Re: GSoC 2017: Foreign Key Arrays

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

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)))
ereport(ERROR,
(errcode(ERRCODE_DATATYPE_MISMATCH),
- 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]),
format_type_be(pktype))));

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.

Andreas