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


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

Reply via email to