From 0200481ef57038f6ed32c07c3aee0c195bb626c5 Mon Sep 17 00:00:00 2001 From: Nikolay Samokhvalov Date: Sat, 18 Jul 2026 15:51:59 +0000 Subject: [PATCH] Fix constraint validation when merging with a NOT ENFORCED constraint When a local ENFORCED check constraint was added to a legacy-inheritance child table having an identically named inherited NOT ENFORCED constraint, MergeWithExistingConstraint() marked the merged constraint as both enforced and validated without checking the table's existing rows. Rows violating the constraint could therefore remain in the table while the catalogs claimed the constraint was valid, and the planner would then trust the false metadata (e.g. for constraint exclusion), silently returning wrong query results. To fix, have MergeWithExistingConstraint() set convalidated according to the new constraint's initially-valid status instead of unconditionally setting it to true, and have ATAddCheckNNConstraint() queue a Phase 3 verification scan of the existing rows when such a merge promotes a NOT ENFORCED constraint to enforced, like ALTER TABLE ... ALTER CONSTRAINT ... ENFORCED does. As a side effect, a constraint added with NOT VALID is no longer silently marked validated when merged this way; it keeps its not-valid state and can be validated later with ALTER TABLE ... VALIDATE CONSTRAINT. Oversight in the introduction of NOT ENFORCED constraints in PostgreSQL 18. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01WKfageiP8Th6TgzZuyCpfQ --- src/backend/catalog/heap.c | 10 +++- src/backend/commands/tablecmds.c | 77 +++++++++++++++++++++++++-- src/test/regress/expected/inherit.out | 52 +++++++++++++++++- src/test/regress/sql/inherit.sql | 33 +++++++++++- 4 files changed, 165 insertions(+), 7 deletions(-) diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c index 88087654de9b7..74a31bf19f75b 100644 --- a/src/backend/catalog/heap.c +++ b/src/backend/catalog/heap.c @@ -2880,12 +2880,20 @@ MergeWithExistingConstraint(Relation rel, const char *ccname, Node *expr, * constraint is not, this should be allowed by marking the child * constraint as enforced. In the reverse case, an error would have * already been thrown before reaching this point. + * + * Since the constraint was not enforced before, its existing rows + * have never been checked against it, so it must not become + * validated as a side effect of the merge. Mark it validated only + * if the new constraint says to be initially valid, in which case + * it is the caller's responsibility to verify the existing rows + * (ATAddCheckNNConstraint queues that work); otherwise it remains + * NOT VALID until ALTER TABLE ... VALIDATE CONSTRAINT is run. */ if (is_enforced && !con->conenforced) { Assert(is_local); con->conenforced = true; - con->convalidated = true; + con->convalidated = is_initially_valid; } CatalogTupleUpdate(conDesc, &tup->t_self, tup); diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index cb93c3e935a1f..a8c6224e65029 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -10053,6 +10053,7 @@ ATAddCheckNNConstraint(List **wqueue, AlteredTableInfo *tab, Relation rel, ListCell *lcon; List *children; ListCell *child; + Oid enforcing_conoid = InvalidOid; ObjectAddress address = InvalidObjectAddress; /* Guard against stack overflow due to overly deep inheritance tree. */ @@ -10063,15 +10064,51 @@ ATAddCheckNNConstraint(List **wqueue, AlteredTableInfo *tab, Relation rel, ATSimplePermissions(AT_AddConstraint, rel, ATT_TABLE | ATT_PARTITIONED_TABLE | ATT_FOREIGN_TABLE); + /* + * If the command names a check constraint and an identically-named NOT + * ENFORCED check constraint already exists on this relation, then (if + * things go well) AddRelationNewConstraints will merge the new + * constraint into the existing one, additionally marking it enforced. + * Existing rows have never been checked against a NOT ENFORCED + * constraint, so unlike ordinary merges this one requires the existing + * rows to be verified when the new constraint is to be valid. Take note + * of the pre-merge state, so that we can queue that work below. + */ + if (constr->contype == CONSTR_CHECK && + constr->conname != NULL && + constr->is_enforced && + constr->initially_valid) + { + Oid conoid; + + conoid = get_relation_constraint_oid(RelationGetRelid(rel), + constr->conname, true); + if (OidIsValid(conoid)) + { + HeapTuple contup; + Form_pg_constraint conform; + + contup = SearchSysCache1(CONSTROID, ObjectIdGetDatum(conoid)); + if (!HeapTupleIsValid(contup)) + elog(ERROR, "cache lookup failed for constraint %u", conoid); + conform = (Form_pg_constraint) GETSTRUCT(contup); + if (conform->contype == CONSTRAINT_CHECK && !conform->conenforced) + enforcing_conoid = conoid; + ReleaseSysCache(contup); + } + } + /* * Call AddRelationNewConstraints to do the work, making sure it works on * a copy of the Constraint so transformExpr can't modify the original. It * returns a list of cooked constraints. * * If the constraint ends up getting merged with a pre-existing one, it's - * omitted from the returned list, which is what we want: we do not need - * to do any validation work. That can only happen at child tables, - * though, since we disallow merging at the top level. + * omitted from the returned list. Normally there is then no validation + * work to do, but if the merge marked a previously NOT ENFORCED + * constraint as enforced, existing rows must be verified; that case is + * handled below. Merging can only happen at child tables, though, since + * we disallow merging at the top level. */ newcons = AddRelationNewConstraints(rel, NIL, list_make1(copyObject(constr)), @@ -10125,6 +10162,40 @@ ATAddCheckNNConstraint(List **wqueue, AlteredTableInfo *tab, Relation rel, /* Advance command counter in case same table is visited multiple times */ CommandCounterIncrement(); + /* + * If the new constraint was merged into a pre-existing NOT ENFORCED + * constraint, the merge marked that constraint enforced and valid, but + * its existing rows have never been checked; tell Phase 3 to verify + * them, just as ALTER TABLE ... ALTER CONSTRAINT ... ENFORCED would do. + * (If the constraint was instead created afresh, the loop above has + * already taken care of this.) + */ + if (newcons == NIL && OidIsValid(enforcing_conoid) && + rel->rd_rel->relkind == RELKIND_RELATION) + { + NewConstraint *newcon; + HeapTuple contup; + Datum val; + char *conbin; + + contup = SearchSysCache1(CONSTROID, ObjectIdGetDatum(enforcing_conoid)); + if (!HeapTupleIsValid(contup)) + elog(ERROR, "cache lookup failed for constraint %u", + enforcing_conoid); + + newcon = palloc0_object(NewConstraint); + newcon->name = pstrdup(constr->conname); + newcon->contype = CONSTR_CHECK; + val = SysCacheGetAttrNotNull(CONSTROID, contup, + Anum_pg_constraint_conbin); + conbin = TextDatumGetCString(val); + newcon->qual = expand_generated_columns_in_expr(stringToNode(conbin), + rel, 1); + ReleaseSysCache(contup); + + tab->constraints = lappend(tab->constraints, newcon); + } + /* * If the constraint got merged with an existing constraint, we're done. * We mustn't recurse to child tables in this case, because they've diff --git a/src/test/regress/expected/inherit.out b/src/test/regress/expected/inherit.out index 0136aa53c96a5..265b7b721857c 100644 --- a/src/test/regress/expected/inherit.out +++ b/src/test/regress/expected/inherit.out @@ -1371,7 +1371,8 @@ NOTICE: merging constraint "inh_check_constraint6" with inherited definition alter table p1_c1 add constraint inh_check_constraint9 check (f1 < 10) not valid enforced; alter table p1 add constraint inh_check_constraint9 check (f1 < 10) not enforced; NOTICE: merging constraint "inh_check_constraint9" with inherited definition --- the not-valid state of the child constraint will be ignored here. +-- the not-valid state of the child constraint is preserved here, so the +-- merged constraint becomes enforced but remains NOT VALID. alter table p1 add constraint inh_check_constraint10 check (f1 < 10) not enforced; alter table p1_c1 add constraint inh_check_constraint10 check (f1 < 10) not valid enforced; NOTICE: merging constraint "inh_check_constraint10" with inherited definition @@ -1411,7 +1412,7 @@ order by 1, 2; p1 | inh_check_constraint8 | t | 0 | t | t p1 | inh_check_constraint9 | t | 0 | f | f p1_c1 | inh_check_constraint1 | t | 1 | t | t - p1_c1 | inh_check_constraint10 | t | 1 | t | t + p1_c1 | inh_check_constraint10 | t | 1 | t | f p1_c1 | inh_check_constraint2 | t | 1 | t | t p1_c1 | inh_check_constraint3 | t | 1 | f | f p1_c1 | inh_check_constraint4 | t | 1 | f | f @@ -1479,6 +1480,53 @@ 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 +-- Existing rows must be verified when merging a local ENFORCED constraint +-- into an inherited NOT ENFORCED one +create table p1(f1 int); +create table p1_c1() inherits(p1); +alter table p1 add constraint inh_check_constraint check (f1 > 0) not enforced; +insert into p1_c1 values(-1); +alter table p1_c1 add constraint inh_check_constraint check (f1 > 0); --error +NOTICE: merging constraint "inh_check_constraint" with inherited definition +ERROR: check constraint "inh_check_constraint" of relation "p1_c1" is violated by some row +-- adding it as NOT VALID skips the verification, and the merged constraint +-- must not be marked validated +alter table p1_c1 add constraint inh_check_constraint check (f1 > 0) not valid; --ok +NOTICE: merging constraint "inh_check_constraint" with inherited definition +select conrelid::regclass, conenforced, convalidated +from pg_constraint where conname = 'inh_check_constraint' +order by conrelid::regclass::text collate "C"; + conrelid | conenforced | convalidated +----------+-------------+-------------- + p1 | f | f + p1_c1 | t | f +(2 rows) + +alter table p1_c1 validate constraint inh_check_constraint; --error +ERROR: check constraint "inh_check_constraint" of relation "p1_c1" is violated by some row +delete from p1_c1 where f1 = -1; +alter table p1_c1 validate constraint inh_check_constraint; --ok +drop table p1 cascade; +NOTICE: drop cascades to table p1_c1 +-- with no violating rows the merge succeeds, and the verification allows the +-- merged constraint to be marked validated +create table p1(f1 int); +create table p1_c1() inherits(p1); +alter table p1 add constraint inh_check_constraint check (f1 > 0) not enforced; +insert into p1_c1 values(1); +alter table p1_c1 add constraint inh_check_constraint check (f1 > 0); --ok +NOTICE: merging constraint "inh_check_constraint" with inherited definition +select conrelid::regclass, conenforced, convalidated +from pg_constraint where conname = 'inh_check_constraint' +order by conrelid::regclass::text collate "C"; + conrelid | conenforced | convalidated +----------+-------------+-------------- + p1 | f | f + p1_c1 | t | t +(2 rows) + +drop table p1 cascade; +NOTICE: drop cascades to table p1_c1 -- 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); diff --git a/src/test/regress/sql/inherit.sql b/src/test/regress/sql/inherit.sql index 072fca13c1351..8eeea8e1f79f3 100644 --- a/src/test/regress/sql/inherit.sql +++ b/src/test/regress/sql/inherit.sql @@ -485,7 +485,8 @@ alter table p1_c1 add constraint inh_check_constraint6 check (f1 < 10) enforced; alter table p1_c1 add constraint inh_check_constraint9 check (f1 < 10) not valid enforced; alter table p1 add constraint inh_check_constraint9 check (f1 < 10) not enforced; --- the not-valid state of the child constraint will be ignored here. +-- the not-valid state of the child constraint is preserved here, so the +-- merged constraint becomes enforced but remains NOT VALID. alter table p1 add constraint inh_check_constraint10 check (f1 < 10) not enforced; alter table p1_c1 add constraint inh_check_constraint10 check (f1 < 10) not valid enforced; @@ -535,6 +536,36 @@ where conname = 'inh_check_constraint3' and contype = 'c' order by conrelid::regclass::text collate "C"; drop table p1 cascade; +-- Existing rows must be verified when merging a local ENFORCED constraint +-- into an inherited NOT ENFORCED one +create table p1(f1 int); +create table p1_c1() inherits(p1); +alter table p1 add constraint inh_check_constraint check (f1 > 0) not enforced; +insert into p1_c1 values(-1); +alter table p1_c1 add constraint inh_check_constraint check (f1 > 0); --error +-- adding it as NOT VALID skips the verification, and the merged constraint +-- must not be marked validated +alter table p1_c1 add constraint inh_check_constraint check (f1 > 0) not valid; --ok +select conrelid::regclass, conenforced, convalidated +from pg_constraint where conname = 'inh_check_constraint' +order by conrelid::regclass::text collate "C"; +alter table p1_c1 validate constraint inh_check_constraint; --error +delete from p1_c1 where f1 = -1; +alter table p1_c1 validate constraint inh_check_constraint; --ok +drop table p1 cascade; + +-- with no violating rows the merge succeeds, and the verification allows the +-- merged constraint to be marked validated +create table p1(f1 int); +create table p1_c1() inherits(p1); +alter table p1 add constraint inh_check_constraint check (f1 > 0) not enforced; +insert into p1_c1 values(1); +alter table p1_c1 add constraint inh_check_constraint check (f1 > 0); --ok +select conrelid::regclass, conenforced, convalidated +from pg_constraint where conname = 'inh_check_constraint' +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);