Re: [PATCH] Partial foreign key updates in referential integrity triggers

On Mon, Nov 22, 2021 at 8:04 PM Paul Martinez <hellopfm(at)gmail(dot)com> wrote:

> On Thu, Nov 11, 2021 at 4:34 AM Peter Eisentraut
> <peter(dot)eisentraut(at)enterprisedb(dot)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
>
Hi,
+ If a foreign key with a <literal>SET NULL</literal> or <literal>SET
+ DEFAULT</literal> delete action, which columns should be updated.

which columns should be updated -> the columns that should be updated

+ if (fk_del_set_cols)
+ {
+ int num_delete_cols = 0;

Since num_delete_cols is only used in the else block, I think it can be
moved inside else block.
Or you can store the value inside *num_fk_del_set_cols directly and avoid
num_delete_cols.

Cheers