Fix restore of not-null constraints with inheritance

In tables with primary keys, pg_dump creates tables with primary keys by
initially dumping them with throw-away not-null constraints (marked "no
inherit" so that they don't create problems elsewhere), to later drop
them once the primary key is restored.  Because of a unrelated
consideration, on tables with children we add not-null constraints to
all columns of the primary key when it is created.

If both a table and its child have primary keys, and pg_dump happens to
emit the child table first (and its throw-away not-null) and later its
parent table, the creation of the parent's PK will fail because the
throw-away not-null constraint collides with the permanent not-null
constraint that the PK wants to add, so the dump fails to restore.

We can work around this problem by letting the primary key "take over"
the child's not-null.  This requires no changes to pg_dump, just two
changes to ALTER TABLE: first, the ability to convert a no-inherit
not-null constraint into a regular inheritable one (including recursing
down to children, if there are any); second, the ability to "drop" a
constraint that is defined both directly in the table and inherited from
a parent (which simply means to mark it as no longer having a local
definition).

Secondarily, change ATPrepAddPrimaryKey() to acquire locks all the way
down the inheritance hierarchy, in case we need to recurse when
propagating constraints.

These two changes allow pg_dump to reproduce more cases involving
inheritance from versions 16 and older.

Lastly, make two changes to pg_dump: 1) do not try to drop a not-null
constraint that's marked as inherited; this allows a dump to restore
with no errors if a table with a PK inherits from another which also has
a PK; 2) avoid giving inherited constraints throwaway names, for the
rare cases where such a constraint survives after the restore.

Reported-by: Andrew Bille <andrewbille@gmail.com>
Reported-by: Justin Pryzby <pryzby@telsasoft.com>
Discussion: https://postgr.es/m/CAJnzarwkfRu76_yi3dqVF_WL-MpvT54zMwAxFwJceXdHB76bOA@mail.gmail.com
Discussion: https://postgr.es/m/Zh0aAH7tbZb-9HbC@pryzbyj2023

3 files changed, 120 insertions, 24 deletions

diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
index cc31909012d..f0278b9c017 100644
--- a/src/backend/catalog/heap.c
+++ b/src/backend/catalog/heap.c
@@ -2539,6 +2539,7 @@ AddRelationNewConstraints(Relation rel,
 			CookedConstraint *nncooked;
 			AttrNumber	colnum;
 			char	   *nnname;
+			int			existing;
 
 			/* Determine which column to modify */
 			colnum = get_attnum(RelationGetRelid(rel), strVal(linitial(cdef->keys)));
@@ -2547,12 +2548,39 @@ AddRelationNewConstraints(Relation rel,
 					 strVal(linitial(cdef->keys)), RelationGetRelid(rel));
 
 			/*
-			 * If the column already has a not-null constraint, we need only
-			 * update its catalog status and we're done.
+			 * If the column already has an inheritable not-null constraint,
+			 * we need only adjust its inheritance status and we're done.  If
+			 * the constraint is there but marked NO INHERIT, then it is
+			 * updated in the same way, but we also recurse to the children
+			 * (if any) to add the constraint there as well.
 			 */
-			if (AdjustNotNullInheritance1(RelationGetRelid(rel), colnum,
-										  cdef->inhcount, cdef->is_no_inherit))
+			existing = AdjustNotNullInheritance1(RelationGetRelid(rel), colnum,
+												 cdef->inhcount, cdef->is_no_inherit);
+			if (existing == 1)
+				continue;		/* all done! */
+			else if (existing == -1)
+			{
+				List	   *children;
+
+				children = find_inheritance_children(RelationGetRelid(rel), NoLock);
+				foreach_oid(childoid, children)
+				{
+					Relation	childrel = table_open(childoid, NoLock);
+
+					AddRelationNewConstraints(childrel,
+											  NIL,
+											  list_make1(copyObject(cdef)),
+											  allow_merge,
+											  is_local,
+											  is_internal,
+											  queryString);
+					/* these constraints are not in the return list -- good? */
+
+					table_close(childrel, NoLock);
+				}
+
 				continue;
+			}
 
 			/*
 			 * If a constraint name is specified, check that it isn't already
diff --git a/src/backend/catalog/pg_constraint.c b/src/backend/catalog/pg_constraint.c
index 604280d322e..778b7c381df 100644
--- a/src/backend/catalog/pg_constraint.c
+++ b/src/backend/catalog/pg_constraint.c
@@ -565,8 +565,8 @@ ChooseConstraintName(const char *name1, const char *name2,
 }
 
 /*
- * Find and return the pg_constraint tuple that implements a validated
- * not-null constraint for the given column of the given relation.
+ * Find and return a copy of the pg_constraint tuple that implements a
+ * validated not-null constraint for the given column of the given relation.
  *
  * XXX This would be easier if we had pg_attribute.notnullconstr with the OID
  * of the constraint that implements the not-null constraint for that column.
@@ -709,37 +709,54 @@ extractNotNullColumn(HeapTuple constrTup)
  * AdjustNotNullInheritance1
  *		Adjust inheritance count for a single not-null constraint
  *
- * Adjust inheritance count, and possibly islocal status, for the not-null
- * constraint row of the given column, if it exists, and return true.
- * If no not-null constraint is found for the column, return false.
+ * If no not-null constraint is found for the column, return 0.
+ * Caller can create one.
+ * If the constraint does exist and it's inheritable, adjust its
+ * inheritance count (and possibly islocal status) and return 1.
+ * No further action needs to be taken.
+ * If the constraint exists but is marked NO INHERIT, adjust it as above
+ * and reset connoinherit to false, and return -1.  Caller is
+ * responsible for adding the same constraint to the children, if any.
  */
-bool
+int
 AdjustNotNullInheritance1(Oid relid, AttrNumber attnum, int count,
 						  bool is_no_inherit)
 {
 	HeapTuple	tup;
 
+	Assert(count >= 0);
+
 	tup = findNotNullConstraintAttnum(relid, attnum);
 	if (HeapTupleIsValid(tup))
 	{
 		Relation	pg_constraint;
 		Form_pg_constraint conform;
+		int			retval = 1;
 
 		pg_constraint = table_open(ConstraintRelationId, RowExclusiveLock);
 		conform = (Form_pg_constraint) GETSTRUCT(tup);
 
 		/*
-		 * Don't let the NO INHERIT status change (but don't complain
-		 * unnecessarily.)  In the future it might be useful to let an
-		 * inheritable constraint replace a non-inheritable one, but we'd need
-		 * to recurse to children to get it added there.
+		 * If we're asked for a NO INHERIT constraint and this relation
+		 * already has an inheritable one, throw an error.
 		 */
-		if (is_no_inherit != conform->connoinherit)
+		if (is_no_inherit && !conform->connoinherit)
 			ereport(ERROR,
 					errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
 					errmsg("cannot change NO INHERIT status of inherited NOT NULL constraint \"%s\" on relation \"%s\"",
 						   NameStr(conform->conname), get_rel_name(relid)));
 
+		/*
+		 * If the constraint already exists in this relation but it's marked
+		 * NO INHERIT, we can just remove that flag, and instruct caller to
+		 * recurse to add the constraint to children.
+		 */
+		if (!is_no_inherit && conform->connoinherit)
+		{
+			conform->connoinherit = false;
+			retval = -1;		/* caller must add constraint on child rels */
+		}
+
 		if (count > 0)
 			conform->coninhcount += count;
 
@@ -761,10 +778,10 @@ AdjustNotNullInheritance1(Oid relid, AttrNumber attnum, int count,
 
 		table_close(pg_constraint, RowExclusiveLock);
 
-		return true;
+		return retval;
 	}
 
-	return false;
+	return 0;
 }
 
 /*
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 027d68e5d2a..f72b2dcadfb 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -9336,7 +9336,6 @@ ATPrepAddPrimaryKey(List **wqueue, Relation rel, AlterTableCmd *cmd,
 {
 	List	   *children;
 	List	   *newconstrs = NIL;
-	ListCell   *lc;
 	IndexStmt  *indexstmt;
 
 	/* No work if not creating a primary key */
@@ -9351,11 +9350,19 @@ ATPrepAddPrimaryKey(List **wqueue, Relation rel, AlterTableCmd *cmd,
 		!rel->rd_rel->relhassubclass)
 		return;
 
-	children = find_inheritance_children(RelationGetRelid(rel), lockmode);
+	/*
+	 * Acquire locks all the way down the hierarchy.  The recursion to lower
+	 * levels occurs at execution time as necessary, so we don't need to do it
+	 * here, and we don't need the returned list either.
+	 */
+	(void) find_all_inheritors(RelationGetRelid(rel), lockmode, NULL);
 
-	foreach(lc, indexstmt->indexParams)
+	/*
+	 * Construct the list of constraints that we need to add to each child
+	 * relation.
+	 */
+	foreach_node(IndexElem, elem, indexstmt->indexParams)
 	{
-		IndexElem  *elem = lfirst_node(IndexElem, lc);
 		Constraint *nnconstr;
 
 		Assert(elem->expr == NULL);
@@ -9374,9 +9381,10 @@ ATPrepAddPrimaryKey(List **wqueue, Relation rel, AlterTableCmd *cmd,
 		newconstrs = lappend(newconstrs, nnconstr);
 	}
 
-	foreach(lc, children)
+	/* Finally, add AT subcommands to add each constraint to each child. */
+	children = find_inheritance_children(RelationGetRelid(rel), NoLock);
+	foreach_oid(childrelid, children)
 	{
-		Oid			childrelid = lfirst_oid(lc);
 		Relation	childrel = table_open(childrelid, NoLock);
 		AlterTableCmd *newcmd = makeNode(AlterTableCmd);
 		ListCell   *lc2;
@@ -12942,6 +12950,31 @@ dropconstraint_internal(Relation rel, HeapTuple constraintTup, DropBehavior beha
 	con = (Form_pg_constraint) GETSTRUCT(constraintTup);
 	constrName = NameStr(con->conname);
 
+	/*
+	 * If we're asked to drop a constraint which is both defined locally and
+	 * inherited, we can simply mark it as no longer having a local
+	 * definition, and no further changes are required.
+	 *
+	 * XXX We do this for not-null constraints only, not CHECK, because the
+	 * latter have historically not behaved this way and it might be confusing
+	 * to change the behavior now.
+	 */
+	if (con->contype == CONSTRAINT_NOTNULL &&
+		con->conislocal && con->coninhcount > 0)
+	{
+		HeapTuple	copytup;
+
+		copytup = heap_copytuple(constraintTup);
+		con = (Form_pg_constraint) GETSTRUCT(copytup);
+		con->conislocal = false;
+		CatalogTupleUpdate(conrel, &copytup->t_self, copytup);
+		ObjectAddressSet(conobj, ConstraintRelationId, con->oid);
+
+		CommandCounterIncrement();
+		table_close(conrel, RowExclusiveLock);
+		return conobj;
+	}
+
 	/* Don't allow drop of inherited constraints */
 	if (con->coninhcount > 0 && !recursing)
 		ereport(ERROR,
@@ -16620,7 +16653,25 @@ MergeConstraintsIntoExisting(Relation child_rel, Relation parent_rel)
 						errmsg("too many inheritance parents"));
 			if (child_con->contype == CONSTRAINT_NOTNULL &&
 				child_con->connoinherit)
+			{
+				/*
+				 * If the child has children, it's not possible to turn a NO
+				 * INHERIT constraint into an inheritable one: we would need
+				 * to recurse to create constraints in those children, but
+				 * this is not a good place to do that.
+				 */
+				if (child_rel->rd_rel->relhassubclass)
+					ereport(ERROR,
+							errmsg("cannot add NOT NULL constraint to column \"%s\" of relation \"%s\" with inheritance children",
+								   get_attname(RelationGetRelid(child_rel),
+											   extractNotNullColumn(child_tuple),
+											   false),
+								   RelationGetRelationName(child_rel)),
+							errdetail("Existing constraint \"%s\" is marked NO INHERIT.",
+									  NameStr(child_con->conname)));
+
 				child_con->connoinherit = false;
+			}
 
 			/*
 			 * In case of partitions, an inherited constraint must be
@@ -20225,7 +20276,7 @@ ATExecDetachPartitionFinalize(Relation rel, RangeVar *name)
  * DetachAddConstraintIfNeeded
  *		Subroutine for ATExecDetachPartition.  Create a constraint that
  *		takes the place of the partition constraint, but avoid creating
- *		a dupe if an constraint already exists which implies the needed
+ *		a dupe if a constraint already exists which implies the needed
  *		constraint.
  */
 static void