From b40a418dfb3d7ce23ffa568c8a8d03640ce28435 Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
Date: Tue, 16 Apr 2024 13:44:34 +0200
Subject: [PATCH] Fix add/drop of not-null constraints

---
 src/backend/catalog/heap.c          | 36 +++++++++++++++++++++++++----
 src/backend/catalog/pg_constraint.c | 36 ++++++++++++++++++++---------
 src/backend/commands/tablecmds.c    | 26 ++++++++++++++++++++-
 src/include/catalog/pg_constraint.h |  2 +-
 4 files changed, 83 insertions(+), 17 deletions(-)

diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
index cc31909012..f0278b9c01 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 604280d322..a11483b1b8 100644
--- a/src/backend/catalog/pg_constraint.c
+++ b/src/backend/catalog/pg_constraint.c
@@ -709,36 +709,50 @@ 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
+ * If the constraint does exist and it's inheritable, adjust its
+ * inheritance count (and possibly islocal status) and return 1.
+ * 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 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 we're asked for a NO INHERIT constraint and this relation
+		 * already has an inheritable one, throw an error.  XXX does this ever
+		 * occur, and is this the right behavior?
 		 */
-		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)));
+		else 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 +775,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 027d68e5d2..470b9b79d9 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -12942,6 +12942,30 @@ dropconstraint_internal(Relation rel, HeapTuple constraintTup, DropBehavior beha
 	con = (Form_pg_constraint) GETSTRUCT(constraintTup);
 	constrName = NameStr(con->conname);
 
+	/*
+	 * For not-null constraints only: if we're asked to drop it, and it's both
+	 * a constraint defined locally and inherited, we can simply mark it as no
+	 * longer having a local definition, and no further changes are required.
+	 *
+	 * XXX the reason we don't do this for CHECK constraints is that they have
+	 * historically not behaved this way.
+	 */
+	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,
@@ -20225,7 +20249,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
diff --git a/src/include/catalog/pg_constraint.h b/src/include/catalog/pg_constraint.h
index 8517fdafd3..5c52d71e09 100644
--- a/src/include/catalog/pg_constraint.h
+++ b/src/include/catalog/pg_constraint.h
@@ -261,7 +261,7 @@ extern HeapTuple findNotNullConstraintAttnum(Oid relid, AttrNumber attnum);
 extern HeapTuple findNotNullConstraint(Oid relid, const char *colname);
 extern HeapTuple findDomainNotNullConstraint(Oid typid);
 extern AttrNumber extractNotNullColumn(HeapTuple constrTup);
-extern bool AdjustNotNullInheritance1(Oid relid, AttrNumber attnum, int count,
+extern int	AdjustNotNullInheritance1(Oid relid, AttrNumber attnum, int count,
 									  bool is_no_inherit);
 extern void AdjustNotNullInheritance(Oid relid, Bitmapset *columns, int count);
 extern List *RelationGetNotNullConstraints(Oid relid, bool cooked);
-- 
2.39.2

