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
