On Thu, Nov 11, 2021 at 4:34 AM Peter Eisentraut <peter.eisentr...@enterprisedb.com> wrote: > > I have reviewed your patch > referential-actions-on-delete-only-set-cols-v3.patch. Attached are two > patches that go on top of yours that contain my recommended changes. > > Basically, this all looks pretty good to me. My changes are mostly > stylistic.
Thank you! I really, really appreciate the thorough review and the comments and corrections. > Some notes of substance: > > - The omission of the referential actions details from the CREATE TABLE > reference page surprised me. I have committed that separately (without > the column support, of course). So you should rebase your patch on top > of that. Note that ALTER TABLE would now also need to be updated by > your patch. Done. > - I recommend setting pg_constraint.confdelsetcols to null for the > default behavior of setting all columns, instead of an empty array the > way you have done it. I have noted the places in the code that need to > be changed for that. Done. > - The outfuncs.c support shouldn't be included in the final patch. > There is nothing wrong it, but I don't think it should be part of this > patch to add piecemeal support like that. I have included a few changes > there anyway for completeness. Got it. I've reverted the changes in that file. > - In ri_triggers.c, I follow your renaming of the constants, but > RI_PLAN_ONTRIGGER_RESTRICT seems a little weird. Maybe do _ONBOTH_, or > else reverse the order, like RI_PLAN_SETNULL_ONDELETE, which would then > allow RI_PLAN_RESTRICT. I've reversed the order, so it's now RI_PLAN_<action>_<trigger>, and renamed the RESTRICT one to just RI_PLAN_RESTRICT. I've attached an updated patch, including your changes and the additional changes mentioned above. - Paul
referential-actions-on-delete-only-set-cols-v4.patch
Description: Binary data
