Sorry for the very late review.

I like this feature and have needed it myself in the past, and the current syntax seems pretty good. One could argue for if the syntax could be generalized to support other things like json and hstore, but I do not think it would be fair to block this patch due to that.

== Limitations of the current design

1) Array element foreign keys can only be specified at the table level (not at columns): I think this limitation is fine. Other PostgreSQL specific features like exclusion contraints can also only be specified at the table level.

2) Lack of support for SET NULL and SET DEFAULT: these do not seem very useful for arrays.

3) Lack of support for specifiying multiple arrays in the foreign key: seems like a good thing to me since it is not obvious what such a thing even would do.

4) That you need to add a cast to the index if you have different types: due to there not being a int4[] <@ int2[] operator you need to add an index on (col::int4[]) to speed up deletes and updates. This one i annoying since EXPLAIN wont give you the query plans for the foreign key queries, but I do not think fixing this should be within the scope of the patch and that having a smaller interger in the referring table is rare.

5) The use of count(DISTINCT) requiring types to support btree equality: this has been discussed a lot up-thread and I think the current state is good enough.

== Functional review

I have played around some with it and things seem to work and the test suite passes, but I noticed a couple of strange behaviors.

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));

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.

2) To me it was not obvious that ON DELETE CASCADE would delete the whole rows rather than delete the members from the array, and this kind of misunderstanding can lead to pretty bad surprises in production. I am leaning towards not supporting CASCADE.

== The @>> operator

A previous version of your patch added the "anyelement <<@ anyarray" operator to avoid having to build arrays, but that part was reverted due to a bug.

I am not expert on the gin code, but as far as I can tell it would be relatively simple to fix that bug. Just allocate an array of Datums of length one where you put the element you are searching for (or maybe a copy of it).

Potential issues with adding the operators:

1) Do we really want to add an operator just for array element foreign keys? I think this is not an issue since it seems like it should be useful in general. I know I have wanted it myself.

2) I am not sure, but the committers might prefer if adding the operators is done in a separate patch.

3) Bikeshedding about operator names. I personally think @>> is clear enough and as far as I know it is not used for anything else.

== Code review

The patch no longer applies to HEAD, but the conflicts are small.

I think we should be more consistent in the naming, both in code and in the documentation. Right now we have "array foreign keys", "element foreign keys", "ELEMENT foreign keys", etc.

+       /*
+ * If this is an array foreign key, we must look up the operators for
+        * the array element type, not the array type itself.
+        */
+       if (fkreftypes[i] != FKCONSTR_REF_PLAIN)

+           if (fkreftypes[i] != FKCONSTR_REF_PLAIN)
+           {
+               old_fktype = get_base_element_type(old_fktype);
+               /* this shouldn't happen ... */
+               if (!OidIsValid(old_fktype))
+                   elog(ERROR, "old foreign key column is not an array");
+           }

+       if (riinfo->fk_reftypes[i] != FKCONSTR_REF_PLAIN)
+       {
+           riinfo->has_array = true;
+           riinfo->ff_eq_oprs[i] = ARRAY_EQ_OP;
+       }

In the three diffs above it would be much cleaner to check for "== FKCONSTR_REF_EACH_ELEMENT" since that better conveys the intent and is safer for adding new types in the future.

+           /* We look through any domain here */
+           fktype = get_base_element_type(fktype);

What does the comment above mean?

        if (!(OidIsValid(pfeqop) && OidIsValid(ffeqop)))
-                    errmsg("foreign key constraint \"%s\" "
-                           "cannot be implemented",
-                           fkconstraint->conname),
-                    errdetail("Key columns \"%s\" and \"%s\" "
-                              "are of incompatible types: %s and %s.",
+ errmsg("foreign key constraint \"%s\" cannot be implemented",
+                      fkconstraint->conname),
+ errdetail("Key columns \"%s\" and \"%s\" are of incompatible types: %s and %s.",
                               strVal(list_nth(fkconstraint->fk_attrs, i)),
                               strVal(list_nth(fkconstraint->pk_attrs, i)),
-                              format_type_be(fktype),
+                              format_type_be(fktypoid[i]),

The above diff looks like pointless code churn which possibly introduces a bug in how it changed fktype to fktypoid[i].

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.

== Minor things in the code

+           {
+               pk_attrs = lappend(pk_attrs, arg);
+               fk_reftypes = lappend_int(fk_reftypes, FKCONSTR_REF_PLAIN);
+           }

This is incorrectly indented.

I suggest that you should only allocate countbuf in RI_FKey_check() if has_array is set.

I think the code would be more readable if both branches of ri_GenerateQual() used the same pattern for whitespace so the differences are easier to spot.

You can use DROP TABLE IF EXISTS to silence the "ERROR: table "fktableforarray" does not exist" errors.

I am not sure that you need to test all combinations of ON UPDATE and ON DELETE. I think it should be enough to test one of each ON UPDATE and one of each ON DELETE should be enough.

+-- Create the foreign table

It is probably a bad idea to call the referencing table the foreign table.

+-- Repeat a similar test using INT4 keys coerced from INT2

This comment is repeated twice in the test suite.

== Documentation

Remove the ELEMENT REFERENCES form from doc/src/sgml/ref/create_table.sgml since we no longer support it.

- FOREIGN KEY ( <replaceable class="parameter">column_name</replaceable> [, ... ] ) REFERENCES <replaceable class="parameter">reftable</replaceable> [ ( <replaceable class="parameter">refcolumn</replaceable> [, ... ] ) ] + FOREIGN KEY ( [ELEMENT] <replaceable class="parameter">column_name</replaceable> [, ... ] ) REFERENCES <replaceable class="parameter">reftable</replaceable> [ ( <replaceable class="parameter">refcolumn</replaceable> [, ... ] ) ]

Change this to "FOREIGN KEY ( [EACH ELEMENT OF] ...".

- <term><literal>FOREIGN KEY ( <replaceable class="parameter">column_name</replaceable> [, ... ] ) + <term><literal>FOREIGN KEY ( [ELEMENT] <replaceable class="parameter">column_name</replaceable> [, ... ] )

Change this to "... FOREIGN KEY ( [EACH ELEMENT OF] ...".

+        <literal>ELEMENT</literal> keyword and <replaceable

Change this to "... <literal>EACH ELEMENT OF</literal> ...". Maybe you should also fix other instances of ELEMENT in the same paragraph but these are less obvious.

You should remove the "ELEMENT REFERENCES" section of doc/src/sgml/ref/create_table.sgml, and move any still relevant bits elsewhere since we do not support this syntax.

The sql-createtable-foreign-key-arrays section need to be updated to remove references to "ELEMENT REFERENCES".

Your indentation in doc/src/sgml/catalogs.sgml is two spaces but the rest of the file looks like it uses 1 space indentation. Additionally you seem to have accidentally reindented something which was correctly indented.

Same with the idnentation in doc/src/sgml/ddl.sgml and doc/src/sgml/ref/create_table.sgml.

-   <varlistentry>
+  <varlistentry id="sql-createtable-foreign-key" xreflabel="FOREIGN KEY">

You have accidentally reindented this in doc/src/sgml/ref/create_table.sgml.

The paragraph in doc/src/sgml/ref/create_table.sgml which starts with "In case the column name" seems to actually be multiple paragraphs. Is that intentional or a mistake?

The documentation in doc/src/sgml/ddl.sgml mentions that "it must be written in table constraint form" for when you have multiple columns, but I feel this is just redundant and confusing since this is always true, both for array foreign keys and for when you have multiple columns.

The documentation in doc/src/sgml/ddl.sgml should mention that we only support one array reference per foreign key.

Maybe the documentation in doc/src/sgml/ddl.sgml should mention that we only support the table constraint form.


Sent via pgsql-hackers mailing list (
To make changes to your subscription:

Reply via email to