Hi. I did some refactoring based on your v3, I didn't change your comments.
I don't think ATGetEquivalentCheckConstraintOid is necessary. ATCheckCheckConstrHasEnforcedParent, AlterCheckConstrEnforceabilityRecurse is enough for this context. + changing_conids = lappend_oid(list_copy(changing_conids), + currcon->oid); Refactor changing_conids to List ** to allow direct modification, aligning with existing code conventions. CREATE TABLE ... PARTITION OF automatically copies the parent's status, and ALTER TABLE ... ATTACH PARTITION already rejects cases where the parent is enforced but the child is not. If we also reject directly altering a partition's constraint enforcement status, then we need no longer worry about cases where parent being enforced while a child is not. Therefore, invoking ATCheckCheckConstrHasEnforcedParent just once for table partitioning is safe. -- jian https://www.enterprisedb.com/
From 53604bd9b5f9164000ad4b36e859f3df7b606c89 Mon Sep 17 00:00:00 2001 From: jian he <[email protected]> Date: Mon, 1 Jun 2026 21:00:54 +0800 Subject: [PATCH v4 1/1] Prevent inherited CHECK constraints from being weakened Disallow marking an inherited CHECK constraint as NOT ENFORCED when an equivalent parent constraint remains ENFORCED. This prevents ALTER CONSTRAINT from producing a child constraint that is weaker than one of its inherited parent definitions. When recursively altering a CHECK constraint to NOT ENFORCED, collect the equivalent constraints in the affected inheritance subtree and ignore those parent constraints while checking descendants. If a descendant also inherits an equivalent ENFORCED constraint from a parent outside the current ALTER, keep the descendant ENFORCED by merging to the stricter state. Add regression coverage for direct child ALTER, ONLY ALTER, mixed-parent inheritance, and a common-ancestor diamond where all equivalent inherited constraints can be changed together. Author: Chao Li <[email protected]> Reviewed-by: Discussion: https://postgr.es/m/[email protected] --- src/backend/commands/tablecmds.c | 185 +++++++++++++++++++++- src/test/regress/expected/constraints.out | 12 +- src/test/regress/expected/inherit.out | 56 +++++++ src/test/regress/sql/constraints.sql | 5 +- src/test/regress/sql/inherit.sql | 34 ++++ 5 files changed, 282 insertions(+), 10 deletions(-) diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index a1845240a98..75ade053b38 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -437,6 +437,7 @@ static bool ATExecAlterFKConstrEnforceability(List **wqueue, ATAlterConstraint * static bool ATExecAlterCheckConstrEnforceability(List **wqueue, ATAlterConstraint *cmdcon, Relation conrel, HeapTuple contuple, bool recurse, bool recursing, + List **changing_conids, LOCKMODE lockmode); static bool ATExecAlterConstrDeferrability(List **wqueue, ATAlterConstraint *cmdcon, Relation conrel, Relation tgrel, Relation rel, @@ -459,6 +460,7 @@ static void AlterFKConstrEnforceabilityRecurse(List **wqueue, ATAlterConstraint static void AlterCheckConstrEnforceabilityRecurse(List **wqueue, ATAlterConstraint *cmdcon, Relation conrel, Oid conrelid, bool recurse, bool recursing, + List **changing_conids, LOCKMODE lockmode); static void AlterConstrDeferrabilityRecurse(List **wqueue, ATAlterConstraint *cmdcon, Relation conrel, Relation tgrel, Relation rel, @@ -466,6 +468,10 @@ static void AlterConstrDeferrabilityRecurse(List **wqueue, ATAlterConstraint *cm List **otherrelids, LOCKMODE lockmode); static void AlterConstrUpdateConstraintEntry(ATAlterConstraint *cmdcon, Relation conrel, HeapTuple contuple); +static bool ATCheckCheckConstrHasEnforcedParent(Relation conrel, Relation rel, + HeapTuple contuple, + List **changing_conids, + Oid *enforced_parentoid); static ObjectAddress ATExecValidateConstraint(List **wqueue, Relation rel, char *constrName, bool recurse, bool recursing, LOCKMODE lockmode); @@ -477,6 +483,7 @@ static void QueueCheckConstraintValidation(List **wqueue, Relation conrel, Relat static void QueueNNConstraintValidation(List **wqueue, Relation conrel, Relation rel, HeapTuple contuple, bool recurse, bool recursing, LOCKMODE lockmode); +static bool constraints_equivalent(HeapTuple a, HeapTuple b, TupleDesc tupleDesc); static int transformColumnNameList(Oid relId, List *colList, int16 *attnums, Oid *atttypids, Oid *attcollids); static int transformFkeyGetPrimaryKey(Relation pkrel, Oid *indexOid, @@ -12482,9 +12489,13 @@ ATExecAlterConstraintInternal(List **wqueue, ATAlterConstraint *cmdcon, InvalidOid, InvalidOid, InvalidOid, InvalidOid); else if (currcon->contype == CONSTRAINT_CHECK) + { + List *changing_conids = NIL; + changed = ATExecAlterCheckConstrEnforceability(wqueue, cmdcon, conrel, contuple, recurse, false, - lockmode); + &changing_conids, lockmode); + } } else if (cmdcon->alterDeferrability && ATExecAlterConstrDeferrability(wqueue, cmdcon, conrel, tgrel, rel, @@ -12671,7 +12682,9 @@ ATExecAlterFKConstrEnforceability(List **wqueue, ATAlterConstraint *cmdcon, static bool ATExecAlterCheckConstrEnforceability(List **wqueue, ATAlterConstraint *cmdcon, Relation conrel, HeapTuple contuple, - bool recurse, bool recursing, LOCKMODE lockmode) + bool recurse, bool recursing, + List **changing_conids, + LOCKMODE lockmode) { Form_pg_constraint currcon; Relation rel; @@ -12693,6 +12706,66 @@ ATExecAlterCheckConstrEnforceability(List **wqueue, ATAlterConstraint *cmdcon, */ rel = table_open(currcon->conrelid, NoLock); + /* + * When setting a constraint to NOT ENFORCED, check whether any matching + * parent constraint remains ENFORCED and is not part of this ALTER. + * + * For a direct ALTER of an inherited constraint, reject the command, + * because the child cannot be weakened while its parent remains enforced. + * + * During recursion, another parent outside this ALTER may still enforce + * the same constraint. In that case, keep the child constraint ENFORCED + * so that its merged enforceability still reflects the remaining enforced + * parent. + */ + if (!cmdcon->is_enforced) + { + Oid enforced_parentoid = InvalidOid; + + bool enforced_parent_constr = false; + + /* + * For partitioned tables, a one-time verification is needed to ensure + * that a partition's constraint cannot be marked as unenforced if the + * parent's constraint remains enforced. + * + * For traditional table inheritance, if a constraint is unenforced + * but its parent constraint is enforced, both this constraint and its + * descendants must be marked as enforced. + */ + if ((rel->rd_rel->relispartition && !recursing) || + (!rel->rd_rel->relispartition && rel->rd_rel->relkind != RELKIND_PARTITIONED_TABLE)) + { + enforced_parent_constr = ATCheckCheckConstrHasEnforcedParent(conrel, rel, contuple, + changing_conids, + &enforced_parentoid); + + if (enforced_parent_constr) + cmdcon->is_enforced = true; + } + + if (enforced_parent_constr && !recursing) + ereport(ERROR, + errcode(ERRCODE_INVALID_OBJECT_DEFINITION), + errmsg("cannot mark inherited constraint \"%s\" as NOT ENFORCED because " + "matching constraint on parent table \"%s\" is ENFORCED", + NameStr(currcon->conname), + get_rel_name(enforced_parentoid))); + + /* + * Build the set of equivalent CHECK constraints that this command + * will attempt to change before visiting descendants. Each descendant + * is compared to the original target constraint, not only by name. + * The root itself has already been checked above. + */ + *changing_conids = lappend_oid(*changing_conids, currcon->oid); + } + + /* + * Update to the merged enforceability if needed. This may differ from the + * requested enforceability when another matching parent constraint + * remains enforced. + */ if (currcon->conenforced != cmdcon->is_enforced) { AlterConstrUpdateConstraintEntry(cmdcon, conrel, contuple); @@ -12702,7 +12775,8 @@ ATExecAlterCheckConstrEnforceability(List **wqueue, ATAlterConstraint *cmdcon, /* * Note that we must recurse even when trying to change a check constraint * to not enforced if it is already not enforced, in case descendant - * constraints might be enforced and need to be changed to not enforced. + * constraints might be enforced and need to be changed to not enforced, + * unless they still inherit an enforced constraint from another parent. * Conversely, we should do nothing if a constraint is being set to * enforced and is already enforced, as descendant constraints cannot be * different in that case. @@ -12737,6 +12811,7 @@ ATExecAlterCheckConstrEnforceability(List **wqueue, ATAlterConstraint *cmdcon, AlterCheckConstrEnforceabilityRecurse(wqueue, cmdcon, conrel, childoid, false, true, + changing_conids, lockmode); } } @@ -12788,6 +12863,7 @@ static void AlterCheckConstrEnforceabilityRecurse(List **wqueue, ATAlterConstraint *cmdcon, Relation conrel, Oid conrelid, bool recurse, bool recursing, + List **changing_conids, LOCKMODE lockmode) { SysScanDesc pscan; @@ -12816,12 +12892,113 @@ AlterCheckConstrEnforceabilityRecurse(List **wqueue, ATAlterConstraint *cmdcon, errmsg("constraint \"%s\" of relation \"%s\" does not exist", cmdcon->conname, get_rel_name(conrelid))); + if (!cmdcon->is_enforced) + { + Form_pg_constraint currcon; + + currcon = (Form_pg_constraint) GETSTRUCT(childtup); + + if (currcon->conenforced) + *changing_conids = list_append_unique_oid(*changing_conids, + currcon->oid); + } + ATExecAlterCheckConstrEnforceability(wqueue, cmdcon, conrel, childtup, - recurse, recursing, lockmode); + recurse, recursing, changing_conids, + lockmode); systable_endscan(pscan); } +/* + * When setting an inherited CHECK constraint to NOT ENFORCED, look for a + * matching parent constraint that remains ENFORCED and is not part of the same + * ALTER. + */ +static bool +ATCheckCheckConstrHasEnforcedParent(Relation conrel, Relation rel, + HeapTuple contuple, + List **changing_conids, + Oid *enforced_parentoid) +{ + Form_pg_constraint currcon; + Relation inhrel; + SysScanDesc scan; + ScanKeyData skey; + HeapTuple inheritsTuple; + + currcon = (Form_pg_constraint) GETSTRUCT(contuple); + Assert(currcon->contype == CONSTRAINT_CHECK); + + if (currcon->coninhcount <= 0) + return false; + + inhrel = table_open(InheritsRelationId, AccessShareLock); + + ScanKeyInit(&skey, + Anum_pg_inherits_inhrelid, + BTEqualStrategyNumber, F_OIDEQ, + ObjectIdGetDatum(RelationGetRelid(rel))); + scan = systable_beginscan(inhrel, InheritsRelidSeqnoIndexId, + true, NULL, 1, &skey); + + while (HeapTupleIsValid(inheritsTuple = systable_getnext(scan))) + { + Oid parentoid; + SysScanDesc pscan; + ScanKeyData pkey[3]; + HeapTuple parenttup; + + parentoid = ((Form_pg_inherits) GETSTRUCT(inheritsTuple))->inhparent; + + ScanKeyInit(&pkey[0], + Anum_pg_constraint_conrelid, + BTEqualStrategyNumber, F_OIDEQ, + ObjectIdGetDatum(parentoid)); + ScanKeyInit(&pkey[1], + Anum_pg_constraint_contypid, + BTEqualStrategyNumber, F_OIDEQ, + ObjectIdGetDatum(InvalidOid)); + ScanKeyInit(&pkey[2], + Anum_pg_constraint_conname, + BTEqualStrategyNumber, F_NAMEEQ, + NameGetDatum(&currcon->conname)); + + pscan = systable_beginscan(conrel, ConstraintRelidTypidNameIndexId, + true, NULL, 3, pkey); + + while (HeapTupleIsValid(parenttup = systable_getnext(pscan))) + { + Form_pg_constraint parentcon; + + parentcon = (Form_pg_constraint) GETSTRUCT(parenttup); + + if (list_member_oid(*changing_conids, parentcon->oid) || + parentcon->contype != CONSTRAINT_CHECK || + parentcon->connoinherit || + !parentcon->conenforced) + continue; + + if (constraints_equivalent(parenttup, contuple, + RelationGetDescr(conrel))) + { + *enforced_parentoid = parentoid; + systable_endscan(pscan); + systable_endscan(scan); + table_close(inhrel, AccessShareLock); + return true; + } + } + + systable_endscan(pscan); + } + + systable_endscan(scan); + table_close(inhrel, AccessShareLock); + + return false; +} + /* * Returns true if the constraint's deferrability is altered. * diff --git a/src/test/regress/expected/constraints.out b/src/test/regress/expected/constraints.out index e54fec7fb57..b649d10e36b 100644 --- a/src/test/regress/expected/constraints.out +++ b/src/test/regress/expected/constraints.out @@ -446,8 +446,12 @@ alter table parted_ch_2 alter constraint cc_2 enforced; --error ERROR: check constraint "cc_2" of relation "parted_ch_2" is violated by some row delete from parted_ch where a = 16; alter table parted_ch_2 alter constraint cc_2 enforced; -alter table parted_ch_2 alter constraint cc not enforced; -alter table parted_ch_2 alter constraint cc_1 not enforced; +alter table parted_ch_2 alter constraint cc not enforced; -- error +ERROR: cannot mark inherited constraint "cc" as NOT ENFORCED because matching constraint on parent table "parted_ch" is ENFORCED +alter table only parted_ch_2 alter constraint cc not enforced; -- error +ERROR: cannot mark inherited constraint "cc" as NOT ENFORCED because matching constraint on parent table "parted_ch" is ENFORCED +alter table parted_ch_2 alter constraint cc_1 not enforced; -- error +ERROR: cannot mark inherited constraint "cc_1" as NOT ENFORCED because matching constraint on parent table "parted_ch" is ENFORCED alter table parted_ch_2 alter constraint cc_2 not enforced; --check these CHECK constraint status again select * from check_constraint_status; @@ -457,12 +461,12 @@ select * from check_constraint_status; cc | parted_ch_1 | t | t cc | parted_ch_11 | t | t cc | parted_ch_12 | t | t - cc | parted_ch_2 | f | f + cc | parted_ch_2 | t | t cc_1 | parted_ch | t | t cc_1 | parted_ch_1 | t | t cc_1 | parted_ch_11 | t | t cc_1 | parted_ch_12 | t | t - cc_1 | parted_ch_2 | f | f + cc_1 | parted_ch_2 | t | t cc_2 | parted_ch_2 | f | f (11 rows) diff --git a/src/test/regress/expected/inherit.out b/src/test/regress/expected/inherit.out index 3d8e8d8afd2..5b87083a870 100644 --- a/src/test/regress/expected/inherit.out +++ b/src/test/regress/expected/inherit.out @@ -1479,6 +1479,62 @@ NOTICE: drop cascades to 3 other objects DETAIL: drop cascades to table p1_c1 drop cascades to table p1_c2 drop cascades to table p1_c3 +-- an inherited CHECK constraint cannot be NOT ENFORCED under an ENFORCED parent +create table p1(f1 int constraint p1_a_check check (f1 > 0) enforced); +create table p1_c1() inherits(p1); +alter table p1_c1 alter constraint p1_a_check not enforced; -- error +ERROR: cannot mark inherited constraint "p1_a_check" as NOT ENFORCED because matching constraint on parent table "p1" is ENFORCED +alter table p1 alter constraint p1_a_check not enforced; -- ok +alter table p1_c1 alter constraint p1_a_check not enforced; -- ok +drop table p1 cascade; +NOTICE: drop cascades to table p1_c1 +-- recursive NOT ENFORCED merges with ENFORCED constraints from other parents +create table p1(a int constraint p1_a_check check (a > 0) enforced); +create table p2(a int constraint p1_a_check check (a > 0) enforced); +create table p1_c1() inherits (p1, p2); +NOTICE: merging multiple inherited definitions of column "a" +alter table p1 alter constraint p1_a_check not enforced; -- ok +select conname, conenforced, convalidated, conrelid::regclass +from pg_constraint +where conname = 'p1_a_check' and contype = 'c' +order by conrelid::regclass::text collate "C"; + conname | conenforced | convalidated | conrelid +------------+-------------+--------------+---------- + p1_a_check | f | f | p1 + p1_a_check | t | t | p1_c1 + p1_a_check | t | t | p2 +(3 rows) + +alter table p1_c1 alter constraint p1_a_check not enforced; -- error +ERROR: cannot mark inherited constraint "p1_a_check" as NOT ENFORCED because matching constraint on parent table "p2" is ENFORCED +alter table p2 alter constraint p1_a_check not enforced; -- ok +alter table p1_c1 alter constraint p1_a_check not enforced; -- ok. Since two parents are not enforced too. +drop table p1, p2 cascade; +NOTICE: drop cascades to table p1_c1 +-- recursive NOT ENFORCED can change all matching enforced parents together +create table gp(a int constraint gp_a_check check (a > 0) enforced); +create table p1() inherits (gp); +create table p2() inherits (gp); +create table p1_c1() inherits (p1, p2); +NOTICE: merging multiple inherited definitions of column "a" +alter table gp alter constraint gp_a_check not enforced; -- ok +select conname, conenforced, convalidated, conrelid::regclass +from pg_constraint +where conname = 'gp_a_check' and contype = 'c' +order by conrelid::regclass::text collate "C"; + conname | conenforced | convalidated | conrelid +------------+-------------+--------------+---------- + gp_a_check | f | f | gp + gp_a_check | f | f | p1 + gp_a_check | f | f | p1_c1 + gp_a_check | f | f | p2 +(4 rows) + +drop table gp cascade; +NOTICE: drop cascades to 3 other objects +DETAIL: drop cascades to table p1 +drop cascades to table p2 +drop cascades to table p1_c1 --for "no inherit" check constraint, it will not recurse to child table create table p1(f1 int constraint p1_a_check check (f1 > 0) no inherit not enforced); create table p1_c1(f1 int constraint p1_a_check check (f1 > 0) not enforced); diff --git a/src/test/regress/sql/constraints.sql b/src/test/regress/sql/constraints.sql index dc133b124bb..26a9ac78c96 100644 --- a/src/test/regress/sql/constraints.sql +++ b/src/test/regress/sql/constraints.sql @@ -309,8 +309,9 @@ select * from check_constraint_status; alter table parted_ch_2 alter constraint cc_2 enforced; --error delete from parted_ch where a = 16; alter table parted_ch_2 alter constraint cc_2 enforced; -alter table parted_ch_2 alter constraint cc not enforced; -alter table parted_ch_2 alter constraint cc_1 not enforced; +alter table parted_ch_2 alter constraint cc not enforced; -- error +alter table only parted_ch_2 alter constraint cc not enforced; -- error +alter table parted_ch_2 alter constraint cc_1 not enforced; -- error alter table parted_ch_2 alter constraint cc_2 not enforced; --check these CHECK constraint status again diff --git a/src/test/regress/sql/inherit.sql b/src/test/regress/sql/inherit.sql index 8f986904389..679b0b2d969 100644 --- a/src/test/regress/sql/inherit.sql +++ b/src/test/regress/sql/inherit.sql @@ -535,6 +535,40 @@ where conname = 'inh_check_constraint3' and contype = 'c' order by conrelid::regclass::text collate "C"; drop table p1 cascade; +-- an inherited CHECK constraint cannot be NOT ENFORCED under an ENFORCED parent +create table p1(f1 int constraint p1_a_check check (f1 > 0) enforced); +create table p1_c1() inherits(p1); +alter table p1_c1 alter constraint p1_a_check not enforced; -- error +alter table p1 alter constraint p1_a_check not enforced; -- ok +alter table p1_c1 alter constraint p1_a_check not enforced; -- ok +drop table p1 cascade; + +-- recursive NOT ENFORCED merges with ENFORCED constraints from other parents +create table p1(a int constraint p1_a_check check (a > 0) enforced); +create table p2(a int constraint p1_a_check check (a > 0) enforced); +create table p1_c1() inherits (p1, p2); +alter table p1 alter constraint p1_a_check not enforced; -- ok +select conname, conenforced, convalidated, conrelid::regclass +from pg_constraint +where conname = 'p1_a_check' and contype = 'c' +order by conrelid::regclass::text collate "C"; +alter table p1_c1 alter constraint p1_a_check not enforced; -- error +alter table p2 alter constraint p1_a_check not enforced; -- ok +alter table p1_c1 alter constraint p1_a_check not enforced; -- ok. Since two parents are not enforced too. +drop table p1, p2 cascade; + +-- recursive NOT ENFORCED can change all matching enforced parents together +create table gp(a int constraint gp_a_check check (a > 0) enforced); +create table p1() inherits (gp); +create table p2() inherits (gp); +create table p1_c1() inherits (p1, p2); +alter table gp alter constraint gp_a_check not enforced; -- ok +select conname, conenforced, convalidated, conrelid::regclass +from pg_constraint +where conname = 'gp_a_check' and contype = 'c' +order by conrelid::regclass::text collate "C"; +drop table gp cascade; + --for "no inherit" check constraint, it will not recurse to child table create table p1(f1 int constraint p1_a_check check (f1 > 0) no inherit not enforced); create table p1_c1(f1 int constraint p1_a_check check (f1 > 0) not enforced); -- 2.34.1
