On Mon, Nov 22, 2021 at 8:04 PM Paul Martinez <hellopfm@gmail.com> wrote:
On Thu, Nov 11, 2021 at 4:34 AM Peter Eisentraut <peter.eisentraut@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
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.