alter check constraint enforceability

On Mon, Jun 2, 2025 at 9:57 PM jian he <jian.universality@gmail.com> wrote:

Currently in pg18, we can add not enforced check constraints.
but we can not do ALTER TABLE ALTER CONSTRAINT [NOT] ENFORCED
for check constraint.

The attached patch is implementation of changing enforceability of
check constraint.

hi.
rebase and minor refactoring.

On Fri, Jul 4, 2025 at 8:00 AM jian he <jian.universality@gmail.com> wrote:

On Mon, Jun 2, 2025 at 9:57 PM jian he <jian.universality@gmail.com> wrote:

Currently in pg18, we can add not enforced check constraints.
but we can not do ALTER TABLE ALTER CONSTRAINT [NOT] ENFORCED
for check constraint.

The attached patch is implementation of changing enforceability of
check constraint.

Initial look and testing looks good. There are some odd parts to work
through with partitioned tables and recursion (for example, if you
have a parent unenforced, and a child enforced, setting a parent
enforced and then not enforced will recurse to the child, so you end
up in a different state. that could be surprising, but the alternative
is not obviously more sensicle).

Some minor items below:

+ errhint("Only foreign key, check constraint can change enforceability"));

"Only foreign key and check constraints can change enforceability"

--

+ /*
+ * If we are told not to recurse, there had better not be any child
+ * tables, because we can't changing constraint enforceability on
+ * the parent unless we have chaned enforceability for all child
+ * tables.
+ */

* tables, because we can't change constraint enforceability on
* the parent unless we have changed enforceability for all child

--

+ if (rel->rd_rel->relkind == RELKIND_RELATION &&
+     cmdcon->is_enforced &&
+     !currcon->conenforced)

i think I have convinced myself that this is correct, but maybe I will
ask you if you had any concerns that this needed to also consider
RELKIND_PARTITIONED_TABLE as well?

Robert Treat
https://xzilla.net

On Thu, Aug 7, 2025 at 7:35 AM Robert Treat <rob@xzilla.net> wrote:

+ if (rel->rd_rel->relkind == RELKIND_RELATION &&
+     cmdcon->is_enforced &&
+     !currcon->conenforced)

i think I have convinced myself that this is correct, but maybe I will
ask you if you had any concerns that this needed to also consider
RELKIND_PARTITIONED_TABLE as well?

ATExecAlterCheckConstrEnforceability itself will be recursive to all
the children.
AlterConstrUpdateConstraintEntry is responsible for changing the catalog state.
except the changing the catalog state, if we change the check
constraint from NOT ENFORCED
to ENFORCED, we also need to verify it in phase 3.
that's the purpose of

+ if (rel->rd_rel->relkind == RELKIND_RELATION &&
+     cmdcon->is_enforced &&
+     !currcon->conenforced)

partitioned tables don't have storage, phase3 table scan to verify
check constraint on partitioned table
don't have effect.

also partitioned table check constraint (name, definition
(pg_constraint.conbin) must match with partition
otherwise partition can be attached to the partitioned table.
so here you don't need to consider RELKIND_PARTITIONED_TABLE.

On Mon, 11 Aug 2025 at 14:53, jian he <jian.universality@gmail.com> wrote:

On Thu, Aug 7, 2025 at 7:35 AM Robert Treat <rob@xzilla.net> wrote:

+ if (rel->rd_rel->relkind == RELKIND_RELATION &&
+     cmdcon->is_enforced &&
+     !currcon->conenforced)

i think I have convinced myself that this is correct, but maybe I will
ask you if you had any concerns that this needed to also consider
RELKIND_PARTITIONED_TABLE as well?

ATExecAlterCheckConstrEnforceability itself will be recursive to all
the children.
AlterConstrUpdateConstraintEntry is responsible for changing the catalog state.
except the changing the catalog state, if we change the check
constraint from NOT ENFORCED
to ENFORCED, we also need to verify it in phase 3.
that's the purpose of

+ if (rel->rd_rel->relkind == RELKIND_RELATION &&
+     cmdcon->is_enforced &&
+     !currcon->conenforced)

partitioned tables don't have storage, phase3 table scan to verify
check constraint on partitioned table
don't have effect.

also partitioned table check constraint (name, definition
(pg_constraint.conbin) must match with partition
otherwise partition can be attached to the partitioned table.
so here you don't need to consider RELKIND_PARTITIONED_TABLE.

Hi!
I looked at v3.

Should we rename `ATExecAlterConstrEnforceability` to
`ATExecAlterFKConstrEnforceability `?

--
Best regards,
Kirill Reshke

On Mon, Aug 11, 2025 at 10:00 AM Kirill Reshke <reshkekirill@gmail.com> wrote:

On Mon, 11 Aug 2025 at 14:53, jian he <jian.universality@gmail.com> wrote:

On Thu, Aug 7, 2025 at 7:35 AM Robert Treat <rob@xzilla.net> wrote:

+ if (rel->rd_rel->relkind == RELKIND_RELATION &&
+     cmdcon->is_enforced &&
+     !currcon->conenforced)

i think I have convinced myself that this is correct, but maybe I will
ask you if you had any concerns that this needed to also consider
RELKIND_PARTITIONED_TABLE as well?

ATExecAlterCheckConstrEnforceability itself will be recursive to all
the children.
AlterConstrUpdateConstraintEntry is responsible for changing the catalog state.
except the changing the catalog state, if we change the check
constraint from NOT ENFORCED
to ENFORCED, we also need to verify it in phase 3.
that's the purpose of

+ if (rel->rd_rel->relkind == RELKIND_RELATION &&
+     cmdcon->is_enforced &&
+     !currcon->conenforced)

partitioned tables don't have storage, phase3 table scan to verify
check constraint on partitioned table
don't have effect.

also partitioned table check constraint (name, definition
(pg_constraint.conbin) must match with partition
otherwise partition can be attached to the partitioned table.
so here you don't need to consider RELKIND_PARTITIONED_TABLE.

Hi!
I looked at v3.

Should we rename `ATExecAlterConstrEnforceability` to
`ATExecAlterFKConstrEnforceability `?

+1

Robert Treat
https://xzilla.net

On Fri, Nov 7, 2025 at 7:29 AM Robert Treat <rob@xzilla.net> wrote:

Hi!
I looked at v3.

Should we rename `ATExecAlterConstrEnforceability` to
`ATExecAlterFKConstrEnforceability `?

+1

Robert Treat
https://xzilla.net

hi.

AlterConstrEnforceabilityRecurse renamed to
AlterFKConstrEnforceabilityRecurse

ATExecAlterConstrEnforceability renamed to
ATExecAlterFKConstrEnforceability.

There seem to be no tests for cases where a partitioned table’s check constraint
is not enforced, but the partition’s constraint is enforced. I’ve added tests
for this case.

ATExecAlterCheckConstrEnforceability
``rel = table_open(currcon->conrelid, NoLock);``

NoLock is ok, because parent is already locked, obviously,
``find_all_inheritors(RelationGetRelid(rel), lockmode, NULL); ``
will lock all the children with lockmode.

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

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

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/

On Fri, Dec 12, 2025 at 1:25 PM jian he <jian.universality@gmail.com> wrote:

On Mon, Dec 8, 2025 at 5:58 PM Amul Sul <sulamul@gmail.com> wrote:

[....]

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

Looks good to me, thanks !

v5-0002: alter check constraint enforceability

The patch also looks good, but I have a minor comment for the test --
you created the check_constraint_status view, which is not dropped, it
should be dropped at the end. Also, instead of a view, I think you
could use the \set psql-meta-command; for example, see the
init_range_parted or show_data tests in update.sql

Also, run pgindent on both patches.

Regards,
Amul

On Mon, Dec 15, 2025 at 7:49 PM Amul Sul <sulamul@gmail.com> wrote:

v5-0002: alter check constraint enforceability

The patch also looks good, but I have a minor comment for the test --
you created the check_constraint_status view, which is not dropped, it
should be dropped at the end. Also, instead of a view, I think you
could use the \set psql-meta-command; for example, see the
init_range_parted or show_data tests in update.sql

Also, run pgindent on both patches.

i have tried using \set, but it seems to require the query within a single line.
since the view check_constraint_status definition is quite longer, \set would
make it less readable, so I choose to use view.

previously I use
+ newcon = (NewConstraint *) palloc0(sizeof(NewConstraint));
now use
+        newcon = palloc0_object(NewConstraint);

v6-0001, v6-0002 both indented properly via pgindent, also polished the commit
messages.

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

On Tue, Dec 16, 2025 at 10:26 AM jian he <jian.universality@gmail.com> wrote:

On Mon, Dec 15, 2025 at 7:49 PM Amul Sul <sulamul@gmail.com> wrote:

[...]

v6-0001, v6-0002 both indented properly via pgindent, also polished the commit
messages.

Thanks. I don't have any other comments. The patch is now ready for
committer review. I have updated the status of the CommitFest entry
accordingly.

Regards,
Amul

On Mon, Dec 15, 2025 at 7:49 PM Amul Sul <sulamul@gmail.com> wrote:

v5-0002: alter check constraint enforceability

The patch also looks good, but I have a minor comment for the test --
you created the check_constraint_status view, which is not dropped, it
should be dropped at the end. Also, instead of a view, I think you
could use the \set psql-meta-command; for example, see the
init_range_parted or show_data tests in update.sql

Also, run pgindent on both patches.

i have tried using \set, but it seems to require the query within a single line.
since the view check_constraint_status definition is quite longer, \set would
make it less readable, so I choose to use view.

previously I use
+ newcon = (NewConstraint *) palloc0(sizeof(NewConstraint));
now use
+        newcon = palloc0_object(NewConstraint);

v6-0001, v6-0002 both indented properly via pgindent, also polished the commit
messages.

I have some questions/comments (no change on status, I didn't test)

- FK are constraint triggers, does it makes sense to align
"ATExecAlterFKConstrEnforceability" renaming with other functions like
"AlterConstrTriggerDeferrability" ?

- I also wonder if it makes sense to manage NOT NULL together with
CHECK, like in ATAddCheckNNConstraint.... ?

--
Cédric Villemain +33 6 20 30 22 52
https://www.Data-Bene.io
PostgreSQL Support, Expertise, Training, R&D

On Mon, Feb 9, 2026 at 5:55 PM Cédric Villemain
<cedric.villemain@data-bene.io> wrote:

I have some questions/comments (no change on status, I didn't test)

- FK are constraint triggers, does it makes sense to align
"ATExecAlterFKConstrEnforceability" renaming with other functions like
"AlterConstrTriggerDeferrability" ?

do you mean changing
ATExecAlterConstrDeferrability
to
ATExecAlterFKConstrDeferrability
?

If so, it makes sense and also improves readability, IMO.
However, since only FK supports changing deferrability—and we are not
modifying deferrability here,
the incentive for this renaming change is kind of lower.

- I also wonder if it makes sense to manage NOT NULL together with
CHECK, like in ATAddCheckNNConstraint.... ?

See ATExecAlterConstraintInternal.
We do not support changing enforceability of NOT NULL, since NOT NULL
NOT ENFORCED is not supported.
I do have a patch for NOT NULL NOT ENFORCED,
https://commitfest.postgresql.org/patch/6029

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

On Mon, Feb 9, 2026 at 5:55 PM Cédric Villemain
<cedric.villemain@data-bene.io> wrote:

I have some questions/comments (no change on status, I didn't test)

- FK are constraint triggers, does it makes sense to align
"ATExecAlterFKConstrEnforceability" renaming with other functions like
"AlterConstrTriggerDeferrability" ?

do you mean changing
ATExecAlterConstrDeferrability
to
ATExecAlterFKConstrDeferrability
?

I mean to keep the ConstrTrigger part in the name, in the idea that it's
the same feature (FK are "just" specialized constraints triggers IIUC).

If so, it makes sense and also improves readability, IMO.
However, since only FK supports changing deferrability—and we are not
modifying deferrability here,
the incentive for this renaming change is kind of lower.

sure.

- I also wonder if it makes sense to manage NOT NULL together with
CHECK, like in ATAddCheckNNConstraint.... ?

See ATExecAlterConstraintInternal.
We do not support changing enforceability of NOT NULL, since NOT NULL
NOT ENFORCED is not supported.
I do have a patch for NOT NULL NOT ENFORCED,
https://commitfest.postgresql.org/patch/6029

Ah great, tks.

I'll check your CF entries instead of adding noise here (I was wondering
if already had a patch to cleanup processCASbits() in gram.y).

--
Cédric Villemain +33 6 20 30 22 52
https://www.Data-Bene.io
PostgreSQL Support, Expertise, Training, R&D

Hello

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

Isn't this a behavior change?

With this change, "changed" is set to true regardless of the return
code of ATExecAlterFKConstrEnforceability (and
ATExecAlterCheckConstrEnforceability). Previously we only set it true
if ATExecAlterFKConstrEnforceability returned true (which means it
actually changed something).

I don't think this is visible anywhere outside the code, but wouldn't
it be better to keep the flag as it was previously?

On Wed, Feb 11, 2026 at 3:37 AM Zsolt Parragi <zsolt.parragi@percona.com> wrote:

Hello

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

Isn't this a behavior change?

Hi.
You are right, I missed this minor detail.
Now, I changed it to

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

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

On 2026-02-21 Sa 9:42 AM, jian he wrote:

On Wed, Feb 11, 2026 at 3:37 AM Zsolt Parragi <zsolt.parragi@percona.com> wrote:

Hello

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

Isn't this a behavior change?

Hi.
You are right, I missed this minor detail.
Now, I changed it to

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

pushed with minor tweaks.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com