Re: alter check constraint enforceability

On Mon, Dec 8, 2025 at 5:58 PM Amul Sul <sulamul(at)gmail(dot)com> 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?
> --
>

hi.
I have addressed all your points mentioned above.

> + 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?
> --

moving the prolog to the IF check seems easier.

>
> +static void
> +AlterCheckConstrEnforceabilityRecurse(List **wqueue, ATAlterConstraint *cmdcon,
> + Relation conrel, Oid conrelid,
> + bool recurse, bool recursing,
> + LOCKMODE lockmode)
> +{
>
> Kindly add a prolog comment.
>

/*
* Invokes ATExecAlterCheckConstrEnforceability for each CHECK constraint that
* is a child of the specified constraint.
*
* We rely on the parent and child tables having identical CHECK constraint
* names to retrieve the child's pg_constraint tuple.
*
* The arguments to this function have the same meaning as the arguments to
* ATExecAlterCheckConstrEnforceability.
*/

The above comments are what I came up with.

v5-0001:
AlterConstrEnforceabilityRecurse renamed to AlterFKConstrEnforceabilityRecurse
ATExecAlterConstrEnforceability renamed to ATExecAlterFKConstrEnforceability.
comments slightly adjusted, no other changes.

v5-0002: alter check constraint enforceability

--
jian
https://www.enterprisedb.com/