Re: [HACKERS] GSoC 2017: Foreign Key Arrays

The patch looks good to me now other than some smaller issues, mostly in
the documentation. If you fix these I will set it as ready for
committer, and let a committer chime in on the casting logic which we
both find a bit ugly.

== Comments

The only a bit bigger issue I see is the error messages. Can they be
improved?

For example:

CREATE TABLE t1 (a int, b int, PrIMARY KEY (a, b));
CREATE TABLE t2 (a int, bs int8[], FOREIGN KEY (a, EACH ELEMENT OF bs)
REFERENCES t1 (a, b));
INSERT INTO t2 VALUES (0, ARRAY[1, 2]);

Results in:

ERROR: insert or update on table "t2" violates foreign key constraint
"t2_a_fkey"
DETAIL: Key (a, bs)=(0, {1,2}) is not present in table "t1".

Would it be possible to make the DETAIL something like the below
instead? Do you think my suggested error message is clear?

I am imaging something like the below as a patch. Does it look sane? The
test cases need to be updated at least.

diff --git a/src/backend/utils/adt/ri_triggers.c
b/src/backend/utils/adt/ri_triggers.c
index 402bde19d4..9dc7c9812c 100644
--- a/src/backend/utils/adt/ri_triggers.c
+++ b/src/backend/utils/adt/ri_triggers.c
@@ -3041,6 +3041,10 @@ ri_ReportViolation(const RI_ConstraintInfo *riinfo,
appendStringInfoString(&key_names, ", ");
appendStringInfoString(&key_values, ", ");
}
+
+ if (riinfo->fk_reftypes[idx] == FKCONSTR_REF_EACH_ELEMENT)
+ appendStringInfoString(&key_names, "EACH ELEMENT OF ");
+
appendStringInfoString(&key_names, name);
appendStringInfoString(&key_values, val);
}

DETAIL: Key (a, EACH ELEMENT OF bs)=(0, {1,2}) is not present in table
"t1".

- REFERENCES <replaceable class="parameter">reftable</replaceable> [ (
<replaceable class="parameter">refcolumn</replaceable> ) ] [ MATCH FULL
| MATCH PARTIAL | MATCH SIMPLE ]
+ [EACH ELEMENT OF] REFERENCES <replaceable
class="parameter">reftable</replaceable> [ ( <replaceable
class="parameter">refcolumn</replaceable> ) ] [ MATCH FULL | MATCH
PARTIAL | MATCH SIMPLE ]

I think this documentation in doc/src/sgml/ref/create_table.sgml should
be removed since it is no longer correct.

+ <para>
+ Foreign Key Arrays are an extension
+ of PostgreSQL and are not included in the SQL standard.
+ </para>

This pargraph and some of the others you added to
doc/src/sgml/ref/create_table.sgml are strangely line wrapped.

+ <varlistentry>
+ <term><literal>ON DELETE CASCADE</literal></term>
+ <listitem>
+ <para>
+ Same as standard foreign key constraints. Deletes the entire
array.
+ </para>
+ </listitem>
+ </varlistentry>
+ </variablelist>

I thought this was no longer supported.

+ however, this syntax will cast ftest1 to int4 upon RI checks,
thus defeating the
+ purpose of the index.

There is a minor indentation error on these lines.

+
+ <para>
+ Array <literal>ELEMENT</literal> foreign keys and the <literal>ELEMENT
+ REFERENCES</literal> clause are a
<productname>PostgreSQL</productname>
+ extension.
+ </para>

We do not have any ELEMENT REFERENCES clause.

- 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.",
- strVal(list_nth(fkconstraint->fk_attrs, i)),
- strVal(list_nth(fkconstraint->pk_attrs, i)),
- format_type_be(fktype),
- format_type_be(pktype))));
+ 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.",
+ strVal(list_nth(fkconstraint->fk_attrs, i)),
+ strVal(list_nth(fkconstraint->pk_attrs, i)),
+ format_type_be(fktype),
+ format_type_be(pktype))));

It seems like you accidentally change the indentation here,

Andreas