Обсуждение: alter check constraint enforceability
hi. 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.
Вложения
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?