Fix constraint validation when merging with a NOT ENFORCED constraint

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);

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

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);

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);