Re: [HACKERS] GSoC 2017: Foreign Key Arrays
Dear Alvaro, We just need to rewrite the scope of the patch so I work on the next generation. I do not know what was "out of scope" in the current version /Mark On Tue, 14 Sep 2021, 8:55 pm Alvaro Herrera, wrote: > On 2021-Sep-14, Daniel Gustafsson wrote: > > > Given the above, and that nothing has happened on this thread since this > note, > > I think we should mark this Returned with Feedback and await a new > submission. > > Does that seem reasonable Alvaro? > > It seems reasonable to me. > > -- > Álvaro Herrera Valdivia, Chile — > https://www.EnterpriseDB.com/ >
Re: [HACKERS] GSoC 2017: Foreign Key Arrays
> > Hey Alvaro, Well, if it's true that it's translated to the commutator, then I don't > think any other code changes are needed. Great, I will get a patch ready tomorrow. Hopefully we’ll wrap up the GIN part of the patch soon. /Mark
Re: [HACKERS] GSoC 2017: Foreign Key Arrays
Hello Alvaro, Looking at 0001+0003, I see it claims GIN support for <<@ and @>>, but > actually only the former is implemented fully; the latter is missing a > strategy number in ginarrayproc.c and pg_amop.dat, and also > src/test/regress/sql/gin.sql does not test it. I suspect > ginqueryarrayextract needs to be told about this too. GIN/array_ops requires the left operand to be an array, so only @>> can be used in GIN. However, <<@ is defined as @>> commutative counterpart, so when for example “5 <<@ index” it will be translated to “index @>> index” thus indirectly using the GIN index. We can definitely add tests to “ src/test/regress/sql/gin.sql” to test this. Do you agree? Also what do you mean by “ ginqueryarrayextract needs to be told about this too”? Best Regards, Mark Rofail
Re: [HACKERS] GSoC 2017: Foreign Key Arrays
Hi Zhihing, I think @Andreas ment to mark it as a todo to cleanup later. On Sun, 21 Mar 2021 at 4:49 AM Zhihong Yu wrote: > Hi, > In v11-0004-fk_arrays_elems_edits.patch : > > + riinfo->fk_reftypes[i] == > FKCONSTR_REF_EACH_ELEMENT ? OID_ARRAY_ELEMCONTAINED_OP : > riinfo->pf_eq_oprs[i], // XXX > > Is XXX placeholder for some comment you will fill in later ? > > Cheers > > On Sat, Mar 20, 2021 at 11:42 AM Mark Rofail > wrote: > >> Hey Andreas and Joel! >> >> Thank you so much for your hard work! >> >> 1. I have removed the dependency on count(DISTINCT ...) by using an >>> anti-join instead. The code is much clearer now IMHO. >>> >> It is much cleaner! I >> >> 2. That instead of selecting operators at execution time we save which >>> operators to use in pg_constraint. >> >> This is a clever approach. If you have any updates on this please let me >> know. >> >> I am still reviewing your changes. I have split your changes from my work >> to be able to isolate the changes and review them carefully. And to help >> others review the changes. >> >> Changelist: >> - v11 (compatible with current master 2021, 03, 20, >> commit e835e89a0fd267871e7fbddc39ad00ee3d0cb55c) >> * rebase >> * split andreas and joel's work >> >> >> On Tue, Mar 16, 2021 at 1:27 AM Andreas Karlsson >> wrote: >> >>> On 3/15/21 4:29 PM, Mark Rofail wrote: >>> > Actually, your fix led me in the right way, the issue was how windows >>> > handle pointers. >>> >>> Hi, >>> >>> I have started working on a partially new strategy for the second patch. >>> The ideas are: >>> >>> 1. I have removed the dependency on count(DISTINCT ...) by using an >>> anti-join instead (this was implemented by Joel Jacobson with cleanup >>> and finishing touches by me). The code is much clearer now IMHO. >>> >>> 2. That instead of selecting operators at execution time we save which >>> operators to use in pg_constraint. This is heavily a work in progress in >>> my attached patches. I am not 100% convinced this is the right way to go >>> but I plan to work some on this this weekend to see how ti will work out. >>> >>> Another thing I will look into is you gin patch. While you fixed it for >>> simple scalar types which fit into the Datum type I wonder if we do not >>> also need to copy types which are too large to fit into a Datum but I >>> have not investigated yet which memory context the datum passed to >>> ginqueryarrayextract() is allocated in. >>> >>> Andreas >>> >>>
Re: [HACKERS] GSoC 2017: Foreign Key Arrays
> > > This still fails for CI (windows) and me (linux) > > Can you provide more information about your Linux platform? So I may test > the errors for myself? > Seems the new tests fails every CI. That’s good honestly, that we found this problem. The `arrays` regression test extensively test the new operators. So I think the problem will be in when the operators are combined with the GIN index. The problem is that the tests don’t fail on my linux build. Any idea how to replicate the failed tests so I can debug? /Mark
Re: [HACKERS] GSoC 2017: Foreign Key Arrays
Hello Justin, It doesn't just rebase: it also removes the test which was failing on > windows > CI: > My apologies, I didn’t include it in the changelog since this is not a code change, just wanted to see if any other test would fail on the windows CI I think the SELECT, when it works, is actually doing a seq scan and not > using > the index. On my PC, the index scan is used until an autovacuum/analyze > run, > after which it uses seqscan. I'm not sure how the other CIs all managed > to run > autovacuum between creating a table and running a query on it, though. This is genius! That explains it. I have been racking my brain for two weeks now and you figured it out. I guess you should first run the query with "explain (costs off)" to show > what > plan it's using, and add things like "SET enable_seqscan=off" as needed to > guarantee that everyone will use the same plan, regardless of minor cost > differences and vacuum timing. I think that will solve the test discrepancy. Honestly Justin, hats off! /Mark >
Re: [HACKERS] GSoC 2017: Foreign Key Arrays
Hey Joel, I will now proceed with the review of fk_arrays_elem_v2 as well. > Great work!! /Mark >
Re: [HACKERS] GSoC 2017: Foreign Key Arrays
Hey Joel, test opr_sanity ... FAILED > > AND binary_coercible(p2.opcintype, p1.amoplefttype)); > amopfamily | amopstrategy | amopopr > +--+- > -(0 rows) > + 2745 |5 |6105 > +(1 row) > > -- Operators that are primary members of opclasses must be immutable (else > -- it suggests that the index ordering isn't fixed). Operators that are > This is due using anycompatiblearray for the left operand in @>>. To solve this problem we need to use @>>(anyarray,anyelement) or introduce a new opclass for gin indices. These are the two approaches that come to mind to solve this. Which one is the right way or is there another solution I am not aware of? That’s why I’m asking this on the mailing list, to get the community’s input. > /Mark
Re: [HACKERS] GSoC 2017: Foreign Key Arrays
Hey Joel, + oprresult = DatumGetBool(FunctionCallInvoke(locfcinfo)); > -+ if (oprresult) > ++ if (!locfcinfo->isnull && oprresult) > + return true; > + } > > Is checking !locfcinfo->isnull due to something new in v2, > or what is just a correction for a problem also in v1? > The “!locfcinfo->isnull” is to protect against segmentation faults. I found it was added to the original function which I adapted the “element” version from. /Mark
Re: [HACKERS] GSoC 2017: Foreign Key Arrays
Hey Joel, > Here comes a first review of the anyarray_anyelement_operators-v1.patch. Great, thanks! I’ll start applying your comments today and release a new patch. /Mark
Re: [HACKERS] GSoC 2017: Foreign Key Arrays
Hello Stephen, > Thanks a lot for your persistence, by the way. > +100 > Hopefully we'll get this in during this cycle and perhaps then you'll work > on something else? :D Thank you for your kind words! Yes, hopefully, we'll get this in this time around. I would definitely love to work on something else once this is done. > Great to see a GSoC student continue to work > with the community years after the GSoC they participated in. If you'd > be interested in being a mentor for this upcoming GSoC season, please > let me know! > Hmm, not sure. I don't think I am well acquainted with the codebase to actually mentor someone. Maybe if I get some experience I would be ready for GSoC 2022. Thanks again Stephen and Álvaro /Mark
Re: [HACKERS] GSoC 2017: Foreign Key Arrays
Hello Álvaro,
Well, *I* think it makes sense to do it that way. I said so three years
> ago :-)
> https://postgr.es/m/20180410135917.odjh5coa4cjatz5v@alvherre.pgsql
So this makes a lot of sense, let's do that.
> I wonder if it can usefully get cross-type
> operators, i.e., @>>(bigint[],smallint) in some way? Maybe the
> "anycompatiblearray" thing can be used for that purpose?
It was easy to get @>> and <<@ accept cross-types thanks to your
suggestion, but I opted to having the operators defined as follows to still
be consistent with the GIN index since the index needs the first operant to
be of type "anyarray"
@>>(anyarray, anycompatiblenonearray) and <<@(anycompatiblenonearray,
anyarray)
Thanks a lot for your persistence, by the way.
Thank you for your words of encouragement, it was one of my deepest
regrests to not seeing this though while in GSoC, hopefiully it gets
commited this time around.
We will focus on getting the operator patch through first. Should I create
a separate commitfest ticket? or the current one suffices?
https://commitfest.postgresql.org/32/2966/
Changelog (operator patch):
- v1 (compatible with current master 2021-02-05,
commit c72af5c202067a9ecb0ff8df7370fb1ea8f4)
* add tests and documentation to array operators and gin index
/Mark
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index b7150510ab..3d36e88494 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -17525,6 +17525,34 @@ SELECT NULLIF(value, '(none)') ...
+
+
+anyarray @>> anycompatiblenonarray
+boolean
+
+
+Does the array contain specified element ?
+
+
+ARRAY[1,4,3] @>> 3
+t
+
+
+
+
+
+anycompatiblenonarray <<@ anyarray
+boolean
+
+
+Is the specified element contained in the array?
+
+
+2 <<@ ARRAY[1,7,4,2,6]
+t
+
+
+
anyarray && anyarray
diff --git a/doc/src/sgml/gin.sgml b/doc/src/sgml/gin.sgml
index d68d12d515..a744609a6b 100644
--- a/doc/src/sgml/gin.sgml
+++ b/doc/src/sgml/gin.sgml
@@ -84,7 +84,7 @@
- array_ops
+ array_ops
&& (anyarray,anyarray)
@@ -93,6 +93,12 @@
<@ (anyarray,anyarray)
+
+ @>> (anyarray,anycompatiblenonarray)
+
+
+ <<@ (anycompatiblenonarray,anyarray)
+
= (anyarray,anyarray)
diff --git a/doc/src/sgml/indices.sgml b/doc/src/sgml/indices.sgml
index 623962d1d8..3cfc64cfe1 100644
--- a/doc/src/sgml/indices.sgml
+++ b/doc/src/sgml/indices.sgml
@@ -326,7 +326,7 @@ SELECT * FROM places ORDER BY location <-> point '(101,456)' LIMIT 10;
for arrays, which supports indexed queries using these operators:
-<@ @> = &&
+<@ @> <<@ @>> = &&
(See for the meaning of
diff --git a/src/backend/access/gin/ginarrayproc.c b/src/backend/access/gin/ginarrayproc.c
index bf73e32932..9b91582021 100644
--- a/src/backend/access/gin/ginarrayproc.c
+++ b/src/backend/access/gin/ginarrayproc.c
@@ -24,6 +24,7 @@
#define GinContainsStrategy 2
#define GinContainedStrategy 3
#define GinEqualStrategy 4
+#define GinContainsElemStrategy 5
/*
@@ -79,7 +80,7 @@ Datum
ginqueryarrayextract(PG_FUNCTION_ARGS)
{
/* Make copy of array input to ensure it doesn't disappear while in use */
- ArrayType *array = PG_GETARG_ARRAYTYPE_P_COPY(0);
+ ArrayType *array;
int32 *nkeys = (int32 *) PG_GETARG_POINTER(1);
StrategyNumber strategy = PG_GETARG_UINT16(2);
@@ -94,14 +95,31 @@ ginqueryarrayextract(PG_FUNCTION_ARGS)
bool *nulls;
int nelems;
- get_typlenbyvalalign(ARR_ELEMTYPE(array),
- &elmlen, &elmbyval, &elmalign);
+ if (strategy == GinContainsElemStrategy)
+ {
+ /*
+ * since this function returns a pointer to a
+ * deconstructed array, there is nothing to do if
+ * operand is a single element except to return it
+ * as is and configure the searchmode
+ */
- deconstruct_array(array,
- ARR_ELEMTYPE(array),
- elmlen, elmbyval, elmalign,
- &elems, &nulls, &nelems);
+ nelems = 1;
+ elems = &PG_GETARG_DATUM(0);
+ nulls = &PG_ARGISNULL(0);
+ }
+ else
+ {
+ array = PG_GETARG_ARRAYTYPE_P_COPY(0);
+
+ get_typlenbyvalalign(ARR_ELEMTYPE(array),
+ &elmlen, &elmbyval, &elmalign);
+ deconstruct_array(array,
+ ARR_ELEMTYPE(array),
+ elmlen, elmbyval, elmalign,
+ &elems, &nulls, &nelems);
+ }
*nkeys = nelems;
*nullFlags = nulls;
@@ -126,6 +144,14 @@ ginqueryarrayextract(PG_FUNCTION_ARGS)
else
*searchMode = GIN_SEARCH_MODE_INCLUDE_EMPTY;
break;
+ case GinContainsElemStrategy:
+ /*
+ * only items that match the queried element
+ * are considered candidate
+ */
+
+ *searchMode = GIN_SEARCH_MODE_DEFAULT;
+ break;
default:
elog(ERROR, "ginqueryarrayextract: unknown strategy number: %d",
strategy);
@@ -
Re: [HACKERS] GSoC 2017: Foreign Key Arrays
Hello Álvaro, I disagree -- I think we should get the second patch in, and consider it > a requisite for the other one. I just want to make sure I got your last message right. We should work on adding the <<@ and @>> operators and their GIN logic as a separate patch then submit the FKARRAY as a future patch? So basically reverse the order of the patches. /Mark
Re: [HACKERS] GSoC 2017: Foreign Key Arrays
Hello Joel, This means, as a user, I might not be aware of the vector restriction when > adding the foreign keys > for my existing schema, and think everything is fine, and not realize > there is a problem until > new data arrives. > Please find v16 with the error message implemented. You can find it in the previous message. > As always thank you for your great insight and your help. /Mark
Re: [HACKERS] GSoC 2017: Foreign Key Arrays
> > > Hey Joel, I apologize for my rash response, I did not quite understand your example > at first, it appears the UPDATE statement is neither over the > referencing nor the referenced columns > The v14 fixed the issue where an error would occur by an update statement that didn't target the refrencing or refrenced columns Vectors as refrencing columns are not supported and out of scope of this patch. Try to use arrays. /Mark
Re: [HACKERS] GSoC 2017: Foreign Key Arrays
Hi Joel, As always, great catch! Kindly find the updated patch (v15) below Changelog: - v15 (compatible with current master 2021-01-27, commit e42b3c3bd6a9c6233ac4c8a2e9b040367ba2f97c) * remove "EACH ELEMENT OF" from violation messages * accommodate tests accordingly Keep up the good work testing this patch. /Mark On Wed, Jan 27, 2021 at 5:11 AM Joel Jacobson wrote: > On Tue, Jan 26, 2021, at 12:59, Mark Rofail wrote: > > Please don't hesitate to give your feedback. > > The error message for insert or update violations looks fine: > > UPDATE catalog_clone.pg_extension SET extconfig = ARRAY[12345] WHERE oid = > 10; > ERROR: insert or update on table "pg_extension" violates foreign key > constraint "pg_extension_extconfig_fkey" > DETAIL: Key (EACH ELEMENT OF extconfig)=(12345) is not present in table > "pg_class". > > But when trying to delete a still referenced row, > the column mentioned in the "EACH ELEMENT OF" sentence > is not the array column, but the column in the referenced table: > > DELETE FROM catalog_clone.pg_class WHERE oid = 10; > ERROR: update or delete on table "pg_class" violates foreign key > constraint "pg_extension_extconfig_fkey" on table "pg_extension" > DETAIL: Key (EACH ELEMENT OF oid)=(10) is still referenced from table > "pg_extension". > > I think either the "EACH ELEMENT OF" part of the latter error message > should be dropped, > or the column name for the array column should be used. > > /Joel >
Re: [HACKERS] GSoC 2017: Foreign Key Arrays
Hello Joel, Thank you for your kind words and happy that you benefited from this patch. We simply assert that the update/delete method used is supported currently only "NO ACTION" and "RESTRICT", this can be extended in future patches without rework, just extra logic. Please don't hesitate to give your feedback. I would love some help with some performance comparisons if you are up to it, between Many-to-Many, Foreign Key Arrays and Gin Indexed Foreign Key Arrays. /Mark On Tue, Jan 26, 2021 at 1:51 PM Joel Jacobson wrote: > Hi Mark, > > On Mon, Jan 25, 2021, at 09:14, Joel Jacobson wrote: > > I'll continue testing over the next couple of days and report if I find > any more oddities. > > > I've continued testing by trying to make full use of this feature in the > catalog-diff-tool I'm working on. > > Today I became aware of a SQL feature that I must confess I've never used > before, > that turned out to be useful in my tool, as I then wouldn't need to follow > FKs > to do updates manually, but let the database do them automatically. > > I'm talking about "ON CASCADE UPDATE", which I see is not supported in the > patch. > > In my tool, I could simplify the code for normal FKs by using ON CASCADE > UPDATE, > but will continue to need my hand-written traverse-FKs-recursively code > for Foreign Key Arrays. > > I lived a long SQL life without ever needing ON CASCADE UPDATE, > so I would definitively vote for Foreign Key Arrays to be added even > without support for this. > > Would you say the patch is written in a way which would allow adding > support for ON CASCADE UPDATE > later on mostly by adding code, or would it require a major rewrite? > > I hesitated if I should share this with you, since I'm really grateful for > this feature even without ON CASCADE UPDATE, > but since this was discovered when testing real-life scenario and not some > hypothetical example, > I felt it should be noted that I stumbled upon this during testing. > > Again, thank you so much for working on this. > > /Joel >
Re: [HACKERS] GSoC 2017: Foreign Key Arrays
Hello Joel, UPDATE catalog_clone.pg_index SET indkey = '1 2 12345'::int2vector WHERE > indexrelid = 2837; > ERROR: operator does not exist: int2vector pg_catalog.@> smallint[] > LINE 1: ...WHERE "attrelid" OPERATOR(pg_catalog.=) $1 AND $2 OPERATOR(p... > In your example, you are using the notation '1 2 12345'::int2vector which signifies a vector as you have mentioned, however in this patch we use the array operator @> to help with the indexing, the incompatibility stems from here. I do not think that postgres contains vector operators, correct me if I am wrong. I feel that supporting vectors is our of the scope of this patch, if you have an idea how to support it please let me know. /Mark
Re: [HACKERS] GSoC 2017: Foreign Key Arrays
Hello Joel, On Sun, Jan 24, 2021 at 12:11 PM Joel Jacobson wrote: > HINT: No operator matches the given name and argument types. You might > need to add explicit type casts. > QUERY: SELECT 1 WHERE (SELECT pg_catalog.count(DISTINCT y) FROM > pg_catalog.unnest($1) y) OPERATOR(pg_catalog.=) (SELECT pg_catalog.count(*) > FROM (SELECT 1 FROM ONLY "catalog_clone"."pg_type" x WHERE $1 > OPERATOR(pg_catalog. @>) ARRAY["oid"] FOR KEY SHARE OF x) z) > > It seems to me there is some type conversion between oidvector and oid[] > that isn't working properly? > This seems to be a type casting problem indeed. The coercion part of the patch found in "ruleutils.c:11438-11491" is the culprit, is there a cleaner way to achieve this? I am aware of the problem and will try to fix it, but I assume this is because of "Array-containselem-gin-v1.patch" can you give the v13 patch a try alone? I will work on a fix and send it soon. /Mark
Re: [HACKERS] GSoC 2017: Foreign Key Arrays
Greetings,
I am trying to revive this patch, Foreign Key Arrays. The original proposal
from my GSoC 2017 days can be found here:
https://www.postgresql.org/message-id/CAJvoCut7zELHnBSC8HrM6p-R6q-NiBN1STKhqnK5fPE-9%3DGq3g%40mail.gmail.com
Disclaimer, I am not the original author of this patch, I picked up this
patch in 2017 to migrate the original patch from 2012 and add a GIN index
to make it usable as the performance without a GIN index is not usable
after 100 rows.
The original authors, Tom Lane and Marco Nenciarini, are the ones who did
most of the heavy lifting. The original discussion can be found here:
https://www.postgresql.org/message-id/flat/1343842863.5162.4.camel%40greygoo.devise-it.lan#1343842863.5162.4.ca...@greygoo.devise-it.lan
*In brief, it would be used as follows:*
```sql
CREATE TABLE A ( atest1 int PRIMARY KEY, atest2 text );
CREATE TABLE B ( btest1 int[], btest2 int );
ALTER TABLE B ADD CONSTRAINT FKARRAY FOREIGN KEY (EACH ELEMENT OF
btest1) REFERENCES A;
```
and now table B references table A as follows:
```sql
INSERT INTO B VALUES ('{10,1}', 2);
```
where this row references rows 1 and 10 from A without the need of a
many-to-many table
*Changelog (since the last version, v8):*
- v9 (made compatible with Postgresql 11)
support `DeconstructFkConstraintRow`
support `CloneFkReferencing`
support `generate_operator_clause`
- v10 (made compatible with Postgresql v12)
support `addFkRecurseReferenced` and `addFkRecurseReferencing`
support `CloneFkReferenced` and `CloneFkReferencing`
migrate tests
- v11(make compatible with Postgresql v13)
drop `ConvertTriggerToFK`
drop `select_common_type_2args` in favor of `select_common_type_from_oids`
migrate tests
- v12(made compatible with current master, 2021-01-23,
commit a8ed6bb8f4cf259b95c1bff5da09a8f4c79dca46)
add ELEMENT to `bare_label_keyword`
migrate docs
*Todo:*
- re-add @>> operator which allows comparison of between array and element
and returns true iff the element is within the array
to allow easier select statements and lower overhead of explicitly creating
an array within the SELECT statement.
```diff
- SELECT * FROM B WHERE btest1 @> ARRAY[5];
+ SELECT * FROM B WHERE btest1 @>> 5;
```
I apologize it took so long to get a new version here (years). However,
this is not the first time I tried to migrate the patch, every time a
different issue blocked me from doing so.
Reviews and suggestions are most welcome, @Joel Jacobson
please
review and test as previously agreed.
/Mark
On Tue, Oct 2, 2018 at 7:13 AM Michael Paquier wrote:
> On Sat, Aug 11, 2018 at 05:20:57AM +0200, Mark Rofail wrote:
> > I am still having problems rebasing this patch. I can not figure it out
> on
> > my own.
>
> Okay, it's been a couple of months since this last email, and nothing
> has happened, so I am marking it as returned with feedback.
> --
> Michael
>
Re: Add primary keys to system catalogs
I'll post tomorrow the latest rebased patch with a summary of the issues. Let's continue this in the foreign key array thread, as to not clutter this thread. Regards, Mark Rofail. On Tue, Jan 19, 2021, 11:00 PM Joel Jacobson wrote: > On Tue, Jan 19, 2021, at 18:25, Mark Rofail wrote: > >Dear Joel, > > > >I would love to start working on this again, I tried to revive the patch > again in 2019, however, I faced the same problems as >last time (detailed > in the thread) and I didn't get the support I needed to continue with this > patch. > > > >Are you willing to help rebase and finally publish this patch? > > Willing, sure, not perhaps not able, > if there are complex problems that need a deep understanding of code base, > I'm not sure I will be of much help, but I can for sure do testing and > reviewing. > > /Joel >
Re: Add primary keys to system catalogs
Dear Joel, I would love to start working on this again, I tried to revive the patch again in 2019, however, I faced the same problems as last time (detailed in the thread) and I didn't get the support I needed to continue with this patch. Are you willing to help rebase and finally publish this patch? Best Regards, Mark Rofail On Tue, 19 Jan 2021 at 2:22 PM Joel Jacobson wrote: > On Mon, Jan 18, 2021, at 18:23, Tom Lane wrote: > > I realized that there's a stronger roadblock for > > treating catalog interrelationships as SQL foreign keys. Namely, > > that we always represent no-reference situations with a zero OID, > > whereas it'd have to be NULL to look like a valid foreign-key case. > > Another roadblock is perhaps the lack of foreign keys on arrays, > which would be needed by the following references: > > SELECT * FROM oidjoins WHERE column_type ~ '(vector|\[\])$' ORDER BY 1,2,3; > table_name | column_name | column_type | ref_table_name | > ref_column_name > > --++-++- > pg_constraint| conexclop | oid[] | pg_operator| oid > pg_constraint| conffeqop | oid[] | pg_operator| oid > pg_constraint| confkey| int2[] | pg_attribute | > attnum > pg_constraint| conkey | int2[] | pg_attribute | > attnum > pg_constraint| conpfeqop | oid[] | pg_operator| oid > pg_constraint| conppeqop | oid[] | pg_operator| oid > pg_extension | extconfig | oid[] | pg_class | oid > pg_index | indclass | oidvector | pg_opclass | oid > pg_index | indcollation | oidvector | pg_collation | oid > pg_index | indkey | int2vector | pg_attribute | > attnum > pg_partitioned_table | partattrs | int2vector | pg_attribute | > attnum > pg_partitioned_table | partclass | oidvector | pg_opclass | oid > pg_partitioned_table | partcollation | oidvector | pg_collation | oid > pg_policy| polroles | oid[] | pg_authid | oid > pg_proc | proallargtypes | oid[] | pg_type| oid > pg_proc | proargtypes| oidvector | pg_type| oid > pg_statistic_ext | stxkeys| int2vector | pg_attribute | > attnum > pg_trigger | tgattr | int2vector | pg_attribute | > attnum > (18 rows) > > I see there is an old thread with work in this area, "Re: GSoC 2017: > Foreign Key Arrays": > > > https://www.postgresql.org/message-id/flat/76a8d3d8-a22e-3299-7c4e-6e115dbf3762%40proxel.se#a3ddc34863465ef83adbd26022cdbcc0 > > The last message in the thread is from 2018-10-02: > >On Tue, 2 Oct, 2018 at 05:13:26AM +0200, Michael Paquier wrote: > >>On Sat, Aug 11, 2018 at 05:20:57AM +0200, Mark Rofail wrote: > >> I am still having problems rebasing this patch. I can not figure it out > on > >> my own. > >Okay, it's been a couple of months since this last email, and nothing > >has happened, so I am marking it as returned with feedback. > >-- > >Michael > > Personally, I would absolutely *love* FKs on array columns. > I always feel shameful when adding array columns to tables, > when the elements are PKs in some other table. > It's convenient and often the best design, > but it feels dirty knowing there are no FKs to help detect application > bugs. > > Let's hope the current FKs-on-catalog-discussion can ignite the old > Foreign Key Arrays thread. > > > /Joel >
Re: [HACKERS] GSoC 2017: Foreign Key Arrays
I am still having problems rebasing this patch. I can not figure it out on my own. On Sun, 27 May 2018 at 5:31 pm, Mark Rofail wrote: > issue 1: `pg_constraint.c:564` > I need to check that `conppeqop` is not null and copy it but I don't know > how to check its type since its a char* > > issue 2: `matview.c:768` > I need to pass `fkreftype` but I don't have it in the rest of the functIon > > On Wed, May 16, 2018 at 10:31 AM, Mark Rofail > wrote: > >> I was wondering if anyone knows the proper way to write a benchmarking >> test for the @>> operator. I used the below script in my previous attempt >> https://gist.github.com/markrofail/174ed370a2f2ac24800fde2fc27e2d38 >> The script does the following steps: >> >> 1. Generate Table A with 5 rows >> 2. Generate Table B with n rows such as: >> every row of Table B is an array of IDs referencing rows in Table A. >> >> The tests we ran previously had Table B up to 1 million rows and the >> results can be seen here : >> >> https://www.postgresql.org/message-id/CAJvoCusMuLnYZUbwTBKt%2Bp6bB9GwiTqF95OsQFHXixJj3LkxVQ%40mail.gmail.com >> >> How would we change this so it would be more exhaustive and accurate? >> >> Regards, >> Mark Rofail >> >> >
Re: [HACKERS] GSoC 2017: Foreign Key Arrays
I was wondering if anyone knows the proper way to write a benchmarking test for the @>> operator. I used the below script in my previous attempt https://gist.github.com/markrofail/174ed370a2f2ac24800fde2fc27e2d38 The script does the following steps: 1. Generate Table A with 5 rows 2. Generate Table B with n rows such as: every row of Table B is an array of IDs referencing rows in Table A. The tests we ran previously had Table B up to 1 million rows and the results can be seen here : https://www.postgresql.org/message-id/CAJvoCusMuLnYZUbwTBKt%2Bp6bB9GwiTqF95OsQFHXixJj3LkxVQ%40mail.gmail.com How would we change this so it would be more exhaustive and accurate? Regards, Mark Rofail
Re: [HACKERS] GSoC 2017: Foreign Key Arrays
On Tue, 10 Apr 2018 at 4:17 pm, Alvaro Herrera wrote: > Mark Rofail wrote: > I meant for the GIN operator. (Remember, these are two patches, and each > of them needs its own tests.) Yes, you are right. I have been dealing with the code as a single patch that I almost forgot. True. So my impression from the numbers you posted last time is that > you need to run each measurement case several times, and provide > averages/ stddevs/etc for the resulting numbers, and see about outliers > (maybe throw them away, or maybe they indicate some problem in the test > or in the code); then we can make an informed decision about whether the > variations between the several different scenarios are real improvements > (or pessimizations) or just measurement noise. I'd rather just throw away the previous results and start over with new performance tests. However, like I showed you, it was my first time to write performance tests. If there's something I could use as a reference that would help me so much. > > In particular: it seemed to me that you decided to throw away the idea > of the new GIN operator without sufficient evidence that it was > unnecessary. I have to admit to that. But in my defence @> is also GIN indexable so the only difference in performance between 'anyarray @>> anyelement' and 'anyarray @> ARRAY [anyelement]' is the delay caused by the ARRAY[] operation theoretically. I apologise, however, I needed more evidence to support my claims. Regards On Tue, Apr 10, 2018 at 4:17 PM, Alvaro Herrera wrote: > Mark Rofail wrote: > > On Tue, Apr 10, 2018 at 3:59 PM, Alvaro Herrera > > > wrote: > > > > > > documentation to it and a few extensive tests to ensure it works well); > > > > I think the existing regression tests verify that the patch works as > > expectations, correct? > > I meant for the GIN operator. (Remember, these are two patches, and each > of them needs its own tests.) > > > We need more *exhaustive* tests to test performance, not functionality. > > True. So my impression from the numbers you posted last time is that > you need to run each measurement case several times, and provide > averages/ stddevs/etc for the resulting numbers, and see about outliers > (maybe throw them away, or maybe they indicate some problem in the test > or in the code); then we can make an informed decision about whether the > variations between the several different scenarios are real improvements > (or pessimizations) or just measurement noise. > > In particular: it seemed to me that you decided to throw away the idea > of the new GIN operator without sufficient evidence that it was > unnecessary. > > -- > Álvaro Herrerahttps://www.2ndQuadrant.com/ > PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services >
Re: [HACKERS] GSoC 2017: Foreign Key Arrays
On Tue, Apr 10, 2018 at 3:59 PM, Alvaro Herrera wrote: > > documentation to it and a few extensive tests to ensure it works well); I think the existing regression tests verify that the patch works as expectations, correct? We need more *exhaustive* tests to test performance, not functionality. Regards
Re: [HACKERS] GSoC 2017: Foreign Key Arrays
Hello David, On Tue, Apr 10, 2018 at 3:47 PM, David Steele wrote: > You should produce a new version by then that addresses Alvaro's > concerns and I imagine that will require quite a bit of discussion and > work. I'll get working. I'll produce a patch with two alternate versions, one with and one without the GIN operators. On 3/7/18 5:43 AM, Alvaro Herrera wrote: > so the performance was measured to see if the GIN operator was > worth it, and the numbers are pretty confusing (the test don't seem > terribly exhaustive; some numbers go up, some go down, it doesn't appear > that the tests were run more than once for each case therefore the > numbers are pretty noisy > Any ideas how to perform more exhaustive tests ? On 3/26/18 4:50 PM, Mark Rofail wrote: > > On Wed, Jan 31, 2018 at 1:52 AM, Andreas Karlsson > wrote: > > > > The issue I see is that > > ginqueryarrayextract() needs to make a copy of the search key but to > do > > so it needs to know the type of anyelement (to know if it needs to > > detoast, etc). But there is as far as I can tell no way to check the > > type of anyelement in this context. > > > > If there is any way to obtain a copy of the datum I would be more than > > happy to integrate the GIN operator to the patch. as I said before we need a way to obtain a copy of a datum to comply with the context of ginqueryarrayextract() Best Regards
Re: Re: [HACKERS] GSoC 2017: Foreign Key Arrays
On 3/7/18 5:43 AM, Alvaro Herrera wrote: I searched for the new GIN operator so that I > could brush it up for commit ahead of the rest -- only to find out that > it was eviscerated from the patch between v5 and v5.1. > > The latest version of the patch which contained the new GIN operator is version `*Array-ELEMENT-foreign-key-v6.patch <https://www.postgresql.org/message-id/attachment/58007/Array-ELEMENT-foreign-key-v6.patch>*` which works fine and passed all the regression tests at the time (2018-01-21). We abandoned the GIN operator since it couldn't follow the same logic as the rest of GIN operators use since it operates on a Datum not an array. Not because of any error. just as Andreas said On Wed, Jan 31, 2018 at 1:52 AM, Andreas Karlsson wrote: The issue I see is that > ginqueryarrayextract() needs to make a copy of the search key but to do > so it needs to know the type of anyelement (to know if it needs to > detoast, etc). But there is as far as I can tell no way to check the > type of anyelement in this context. > > If there is any way to obtain a copy of the datum I would be more than happy to integrate the GIN operator to the patch. Best. Mark Rofail
Re: [HACKERS] GSoC 2017: Foreign Key Arrays
On Sat, 3 Feb 2018 at 5:14 pm, Andreas Karlsson wrote: > If I am right my recommendation for getting this patch in is to > initially skip the new operators and go back to the version of the patch > which uses @>. We really need an initial patch to get accepted and postpone anything that won't affect the core mechanics of the patch. So yes, I move towards delaying the introduction of @>> to the patch. A new patch will be ready by tomorrow. Besr Regards, Mark Rofail > >
Re: [HACKERS] GSoC 2017: Foreign Key Arrays
On Wed, Jan 31, 2018 at 1:52 AM, Andreas Karlsson wrote: > I looked some at your anyarray @>> anyelement code and sadly it does not > look like the index code could work. The issue I see is that > ginqueryarrayextract() needs to make a copy of the search key but to do so > it needs to know the type of anyelement (to know if it needs to detoast, > etc). But there is as far as I can tell no way to check the type of > anyelement in this context. > since its a polymorphic function it only passes if the `anyarray` is the same type of the `anyelement` so we are sure they are the same type. Can't we get the type from the anyarray ? the type is already stored in ` arr_type`. Best Regards, Mark Rofail
Re: [HACKERS] GSoC 2017: Foreign Key Arrays
On Mon, Nov 13, 2017 at 2:41 AM, Andreas Karlsson wrote: > I think the code would look cleaner if you generate the following query: > > SELECT fk.x, fk.ys FROM ONLY t2 fk CROSS JOIN LATERAL > pg_catalog.unnest(ys) a2 (v) LEFT OUTER JOIN ONLY t1 pk ON pk.x = fk.x AND > pk.y = a2.v WHERE [...] > > rather than: > > SELECT fk.k1, fk.ak2 FROM (SELECT x k1, pg_catalog.unnest(ys) k2, ys ak2 > FROM ONLY t2) fk LEFT OUTER JOIN ONLY t1 pk ON pk.x = fk.k1 AND pk.y = > fk.k2 WHERE [...] > Andreas has kindly written the SQL query for us. My problem is generating it with C code Best Regards, Mark Rofail
Re: [HACKERS] GSoC 2017: Foreign Key Arrays
According to the last review, there are still two main issues with the
patch.
On Mon, Oct 30, 2017 at 3:24 AM, Andreas Karlsson wrote:
> 1) MATCH FULL does not seem to care about NULLS in arrays. In the example
> below I expected both inserts into the referring table to fail.
>
> CREATE TABLE t (x int, y int, PRIMARY KEY (x, y));
> CREATE TABLE fk (x int, ys int[], FOREIGN KEY (x, EACH ELEMENT OF ys)
> REFERENCES t MATCH FULL);
> INSERT INTO t VALUES (10, 1);
> INSERT INTO fk VALUES (10, '{1,NULL}');
> INSERT INTO fk VALUES (NULL, '{1}');
>
> CREATE TABLE
> CREATE TABLE
> INSERT 0 1
> INSERT 0 1
> ERROR: insert or update on table "fk" violates foreign key constraint
> "fk_x_fkey"
> DETAIL: MATCH FULL does not allow mixing of null and nonnull key values.
>
> I understand that Match full should contain nulls in the results. However,
I don't think that it's semantically correct, so I suggest we don't use
Match full. What would be the consequences of that ?
2) I think the code in RI_Initial_Check() would be cleaner if you used
> "CROSS JOIN LATERAL unnest(col)" rather than having unnest() in the target
> list. This way you would not need to rename all columns and the code paths
> for the array case could look more like the code path for the normal case.
>
I have repeatedly tried to generate the suggested query using C code and I
failed. I would like some help with it
Best Regards,
Mark Rofail
