Re: alter check constraint enforceability - Mailing list pgsql-hackers

On Thu, Dec 4, 2025 at 12:22 PM jian he <jian.universality@gmail.com> wrote:
>
> On Fri, Nov 7, 2025 at 7:29 AM Robert Treat <rob@xzilla.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