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

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers