Re: alter check constraint enforceability

On Thu, Dec 4, 2025 at 12:22 PM jian he <jian(dot)universality(at)gmail(dot)com> wrote:
>
> On Fri, Nov 7, 2025 at 7:29 AM Robert Treat <rob(at)xzilla(dot)net> wrote:
> >

The v4 patch is quite good. Here are a few comments/suggestions for
the cosmetic fixes:

+ created. Currently <literal>FOREIGN KEY</literal> and
+ <literal>CHECK</literal> constraints may be altered in this
fashion, but see below.

Although documents may not strictly follow an 80-column length
restriction all the places, it is better to adhere to it as much as possible.
--

+ errhint("Only foreign key and check constraints can
change enforceability"));

Missing a full stop (.) at the end.
--

+ /*
+ * parent relation already locked by called, children will be locked by
+ * find_all_inheritors. So NoLock is fine here.
+ */
+ rel = table_open(currcon->conrelid, NoLock);
+ if (currcon->conenforced != cmdcon->is_enforced)
+ {

Add a newline between these. Also, start comment with capital letter:
s/parent/Parent
--

-static bool ATExecAlterConstrEnforceability(List **wqueue,
...
+static bool ATExecAlterFKConstrEnforceability(List **wqueue,

I suggest the renaming patch be separated.
--

- * Currently only works for Foreign Key and not null constraints.
+ * Currently works for Foreign Key, CHECK, and not null constraints.

Not consistent naming format, should be: s/CHECK/Check.
--

+ if (cmdcon->alterEnforceability)
+ {
+ if (currcon->contype == CONSTRAINT_FOREIGN)
+ {
+ ATExecAlterFKConstrEnforceability(wqueue, cmdcon, conrel, tgrel,
+ currcon->conrelid,
currcon->confrelid,
+ contuple, lockmode, InvalidOid,
+ InvalidOid, InvalidOid,
InvalidOid);
+ changed = true;
+ }
+ else if (currcon->contype == CONSTRAINT_CHECK)
+ {
+ ATExecAlterCheckConstrEnforceability(wqueue, cmdcon,
conrel, contuple,
+ recurse, false, lockmode);
+ changed = true;
+ }
+ }

Don't need inner curly braces; set changed = true; once for both.
--

+ * conrel is the pg_constraint catalog relation.

Not sure why we need to mention conrel here only?
--

+ if (!cmdcon->is_enforced || changed)
+ {

The reason for recursing for the non-enforced constraint (like the FK
constraint) is mentioned in the function prolog. However, since two
conditions are involved here, I was initially confused about the
change. Could you please add a short comment explaining why we enter
for the not-enforced constraint irrespective of whether it was changed
or not, or simply move the relevant note from the prolog here?
--

+static void
+AlterCheckConstrEnforceabilityRecurse(List **wqueue, ATAlterConstraint *cmdcon,
+ Relation conrel, Oid conrelid,
+ bool recurse, bool recursing,
+ LOCKMODE lockmode)
+{

Kindly add a prolog comment.

Regards,
Amul