Re: GSoC 2017: Foreign Key Arrays

On 11/10/2017 01:47 AM, Mark Rofail wrote:
> I am sorry for the late reply

There is no reason for you to be. It did not take you 6 weeks to do a
review. :) Thanks for this new version.

> == Functional review
>
> >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.
>
>
> It seems in your example the only failed case was: INSERT INTO fk VALUES
> (NULL, '{1}');
> which shouldn't work, can you clarify this?

I think that if you use MATH FULL the query should fail if you have a
NULL in the array.

> >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.
>
>
> I would say so too, maybe we should remove ON DELETE CASCADE until we
> have supported all remaining actions.

I am leaning towards this too. I would personally be fine with a first
version without support for CASCADE since it is not obvious to me what
CASCADE should do.

> == The @>> operator
> I would argue that allocating an array of datums and building an array
> would have the same complexity

I am not sure what you mean here. Just because something has the same
complexity does not mean there can't be major performance differences.

> == Code review
>
> >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.
>
> Can you clarify what you mean a bit more?

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 [...]

= New stuff

When applying the patch I got some white space warnings:

Array-ELEMENT-foreign-key-v5.3.patch:1343: space before tab in indent,
indent with spaces.
format_type_be(oprleft), format_type_be(oprright))));
Array-ELEMENT-foreign-key-v5.3.patch:1345: trailing whitespace.

When compiling I got an error:

ri_triggers.c: In function ‘ri_GenerateQual’:
ri_triggers.c:2693:19: error: unknown type name ‘d’
Oid oprcommon;d
^
ri_triggers.c:2700:3: error: conflicting types for ‘oprright’
oprright = get_array_type(operform->oprleft);
^~~~~~~~
ri_triggers.c:2691:9: note: previous declaration of ‘oprright’ was here
Oid oprright;
^~~~~~~~
<builtin>: recipe for target 'ri_triggers.o' failed

When building the documentation I got two warnings:

/usr/bin/osx:catalogs.sgml:2349:17:W: empty end-tag
/usr/bin/osx:catalogs.sgml:2350:17:W: empty end-tag

When running the tests I got a failure in element_foreign_key.

Andreas