Re: [HACKERS] GSoC 2017: Foreign Key Arrays - Mailing list pgsql-hackers

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