Обсуждение: 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/