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

On Mon, Dec 8, 2025 at 5:58 PM Amul Sul <sulamul@gmail.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/