On Thu, Dec 4, 2025 at 12:22 PM jian he <[email protected]> wrote:
>
> On Fri, Nov 7, 2025 at 7:29 AM Robert Treat <[email protected]> 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
