Feedback-ID: ia2694551:Fastmail
Date: Wed, 24 Apr 2024 19:36:17 +0200
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
To: Alexander Lakhin <exclusion@gmail.com>
Cc: Dean Rasheed <dean.a.rasheed@gmail.com>,
	Andrew Bille <andrewbille@gmail.com>,
 Peter Eisentraut <peter@eisentraut.org>,
	Robert Haas <robertmhaas@gmail.com>, pgsql-hackers@lists.postgresql.org
Subject: Re: cataloguing NOT NULL constraints
Message-ID: <202404241736.jf2zq2qmrgov@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="pq2l62po3vnphnkg"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <202404221022.nljgb5dwzppl@alvherre.pgsql>
Archived-At: 
 <https://www.postgresql.org/message-id/202404241736.jf2zq2qmrgov%40alvherre.pgsql>
Precedence: bulk


--pq2l62po3vnphnkg
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit

On 2024-Apr-22, Alvaro Herrera wrote:

> > On d9f686a72~1 this script results in:
> > ERROR:  cannot change NO INHERIT status of inherited NOT NULL constraint "t_a_not_null" on relation "t"
> 
> Right.  Now I'm beginning to wonder if allowing ADD CONSTRAINT to mutate
> a pre-existing NO INHERIT constraint into a inheritable constraint
> (while accepting a constraint name in the command that we don't heed) is
> really what we want.  Maybe we should throw some error when the affected
> constraint is the topmost one, and only accept the inheritance status
> change when we're recursing.

So I added a restriction that we only accept such a change when
recursively adding a constraint, or during binary upgrade.  This should
limit the damage: you're no longer able to change an existing constraint
from NO INHERIT to YES INHERIT merely by doing another ALTER TABLE ADD
CONSTRAINT.

One thing that has me a little nervous about this whole business is
whether we're set up to error out where some child table down the
hierarchy has nulls, and we add a not-null constraint to it but fail to
do a verification scan.  I tried a couple of cases and AFAICS it works
correctly, but maybe there are other cases I haven't thought about where
it doesn't.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"You're _really_ hosed if the person doing the hiring doesn't understand
relational systems: you end up with a whole raft of programmers, none of
whom has had a Date with the clue stick."              (Andrew Sullivan)
https://postgr.es/m/20050809113420.GD2768@phlogiston.dyndns.org

--pq2l62po3vnphnkg
Content-Type: text/x-diff; charset=utf-8
Content-Disposition: attachment;
	filename="v2-0001-Acquire-locks-on-children-before-recursing.patch"

From 238bc09bed57dcd0e4887615f3c57a580eb26d9e Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
Date: Mon, 22 Apr 2024 11:32:04 +0200
Subject: [PATCH v2 1/2] Acquire locks on children before recursing

ALTER TABLE ADD CONSTRAINT was missing this, as evidenced by assertion
failures.  While at it, create a small routine to encapsulate the weird
find_all_inheritors() call.

Reported-by: Alexander Lakhin <exclusion@gmail.com>
Discussion: https://postgr.es/m/5b4cd32f-1d5b-c080-c688-31736bbcd739@gmail.com
---
 src/backend/commands/tablecmds.c | 40 +++++++++++++++++++++-----------
 1 file changed, 27 insertions(+), 13 deletions(-)

diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 3556240c8e..9058a0de7a 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -427,6 +427,7 @@ static void ATSimplePermissions(AlterTableType cmdtype, Relation rel, int allowe
 static void ATSimpleRecursion(List **wqueue, Relation rel,
 							  AlterTableCmd *cmd, bool recurse, LOCKMODE lockmode,
 							  AlterTableUtilityContext *context);
+static void ATLockAllDescendants(Oid relid, LOCKMODE lockmode);
 static void ATCheckPartitionsNotInUse(Relation rel, LOCKMODE lockmode);
 static void ATTypedTableRecursion(List **wqueue, Relation rel, AlterTableCmd *cmd,
 								  LOCKMODE lockmode,
@@ -1621,9 +1622,7 @@ RemoveRelations(DropStmt *drop)
 		 * will lock those objects in the other order.
 		 */
 		if (state.actual_relkind == RELKIND_PARTITIONED_INDEX)
-			(void) find_all_inheritors(state.heapOid,
-									   state.heap_lockmode,
-									   NULL);
+			ATLockAllDescendants(state.heapOid, state.heap_lockmode);
 
 		/* OK, we're ready to delete this one */
 		obj.classId = RelationRelationId;
@@ -4979,10 +4978,12 @@ ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd,
 			break;
 		case AT_AddConstraint:	/* ADD CONSTRAINT */
 			ATSimplePermissions(cmd->subtype, rel, ATT_TABLE | ATT_FOREIGN_TABLE);
-			/* Recursion occurs during execution phase */
-			/* No command-specific prep needed except saving recurse flag */
 			if (recurse)
+			{
+				/* if recursing, set flag and lock all descendants */
 				cmd->recurse = true;
+				ATLockAllDescendants(RelationGetRelid(rel), lockmode);
+			}
 			pass = AT_PASS_ADD_CONSTR;
 			break;
 		case AT_AddIndexConstraint: /* ADD CONSTRAINT USING INDEX */
@@ -6721,6 +6722,21 @@ ATSimpleRecursion(List **wqueue, Relation rel,
 	}
 }
 
+/*
+ * ATLockAllDescendants
+ *
+ * Acquire lock on all descendant relations of the given relation.
+ */
+static void
+ATLockAllDescendants(Oid relid, LOCKMODE lockmode)
+{
+	/*
+	 * This is only used in DDL code, so it doesn't matter that we leak the
+	 * list storage; it'll be gone soon enough.
+	 */
+	(void) find_all_inheritors(relid, lockmode, NULL);
+}
+
 /*
  * Obtain list of partitions of the given table, locking them all at the given
  * lockmode and ensuring that they all pass CheckTableNotInUse.
@@ -9370,10 +9386,9 @@ ATPrepAddPrimaryKey(List **wqueue, Relation rel, AlterTableCmd *cmd,
 
 	/*
 	 * 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.
+	 * levels occurs at execution time as necessary.
 	 */
-	(void) find_all_inheritors(RelationGetRelid(rel), lockmode, NULL);
+	ATLockAllDescendants(RelationGetRelid(rel), lockmode);
 
 	/*
 	 * Construct the list of constraints that we need to add to each child
@@ -9819,10 +9834,10 @@ ATAddCheckNNConstraint(List **wqueue, AlteredTableInfo *tab, Relation rel,
 	/*
 	 * Propagate to children as appropriate.  Unlike most other ALTER
 	 * routines, we have to do this one level of recursion at a time; we can't
-	 * use find_all_inheritors to do it in one pass.
+	 * use find_all_inheritors to do it in one pass.  We have all locks
+	 * already, however.
 	 */
-	children =
-		find_inheritance_children(RelationGetRelid(rel), lockmode);
+	children = find_inheritance_children(RelationGetRelid(rel), NoLock);
 
 	/*
 	 * Check if ONLY was specified with ALTER TABLE.  If so, allow the
@@ -11258,8 +11273,7 @@ CloneFkReferencing(List **wqueue, Relation parentRel, Relation partRel)
 		 */
 		pkrel = table_open(constrForm->confrelid, ShareRowExclusiveLock);
 		if (pkrel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
-			(void) find_all_inheritors(RelationGetRelid(pkrel),
-									   ShareRowExclusiveLock, NULL);
+			ATLockAllDescendants(RelationGetRelid(pkrel), ShareRowExclusiveLock);
 
 		DeconstructFkConstraintRow(tuple, &numfks, conkey, confkey,
 								   conpfeqop, conppeqop, conffeqop,
-- 
2.39.2


--pq2l62po3vnphnkg
Content-Type: text/x-diff; charset=utf-8
Content-Disposition: attachment;
	filename="v2-0002-Disallow-changing-NO-INHERIT-property-of-a-constr.patch"

From 2773755db4d6df08c14c0854dcf7f3d811fd8ebb Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
Date: Wed, 24 Apr 2024 16:54:39 +0200
Subject: [PATCH v2 2/2] Disallow changing NO INHERIT property of a constraint

... unless it happens during binary upgrade, or to make an existing
constraint into an inherited one of a constraint in a parent relation.
---
 src/backend/catalog/heap.c            | 20 +++++++++++++++-----
 src/backend/catalog/pg_constraint.c   | 15 +++++++++++----
 src/include/catalog/pg_constraint.h   |  2 +-
 src/test/regress/expected/inherit.out |  2 +-
 4 files changed, 28 insertions(+), 11 deletions(-)

diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
index f0278b9c01..136cc42a92 100644
--- a/src/backend/catalog/heap.c
+++ b/src/backend/catalog/heap.c
@@ -2549,13 +2549,23 @@ AddRelationNewConstraints(Relation rel,
 
 			/*
 			 * 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.
+			 * we need only adjust its coninhcount and we're done.  In certain
+			 * cases (see below), if the constraint is there but marked NO
+			 * INHERIT, then we mark it as no longer such and coninhcount
+			 * updated, plus we must also recurse to the children (if any) to
+			 * add the constraint there.
+			 *
+			 * We only allow the inheritability status to change during binary
+			 * upgrade (where it's used to add the not-null constraints for
+			 * children of tables with primary keys), or when we're recursing
+			 * processing a table down an inheritance hierarchy; directly
+			 * allowing a constraint to change from NO INHERIT to INHERIT
+			 * during ALTER TABLE ADD CONSTRAINT would be far too surprising
+			 * behavior.
 			 */
 			existing = AdjustNotNullInheritance1(RelationGetRelid(rel), colnum,
-												 cdef->inhcount, cdef->is_no_inherit);
+												 cdef->inhcount, cdef->is_no_inherit,
+												 IsBinaryUpgrade || allow_merge);
 			if (existing == 1)
 				continue;		/* all done! */
 			else if (existing == -1)
diff --git a/src/backend/catalog/pg_constraint.c b/src/backend/catalog/pg_constraint.c
index aaf3537d3f..6b8496e085 100644
--- a/src/backend/catalog/pg_constraint.c
+++ b/src/backend/catalog/pg_constraint.c
@@ -721,7 +721,7 @@ extractNotNullColumn(HeapTuple constrTup)
  */
 int
 AdjustNotNullInheritance1(Oid relid, AttrNumber attnum, int count,
-						  bool is_no_inherit)
+						  bool is_no_inherit, bool allow_noinherit_change)
 {
 	HeapTuple	tup;
 
@@ -744,16 +744,23 @@ AdjustNotNullInheritance1(Oid relid, AttrNumber attnum, int count,
 		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\"",
+					errmsg("cannot change NO INHERIT status of 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.
+		 * NO INHERIT, we can just remove that flag (provided caller allows
+		 * such a change), and instruct caller to recurse to add the
+		 * constraint to children.
 		 */
 		if (!is_no_inherit && conform->connoinherit)
 		{
+			if (!allow_noinherit_change)
+				ereport(ERROR,
+						errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+						errmsg("cannot change NO INHERIT status of NOT NULL constraint \"%s\" in relation \"%s\"",
+							   NameStr(conform->conname), get_rel_name(relid)));
+
 			conform->connoinherit = false;
 			retval = -1;		/* caller must add constraint on child rels */
 		}
diff --git a/src/include/catalog/pg_constraint.h b/src/include/catalog/pg_constraint.h
index 5c52d71e09..68bf55fdf7 100644
--- a/src/include/catalog/pg_constraint.h
+++ b/src/include/catalog/pg_constraint.h
@@ -262,7 +262,7 @@ extern HeapTuple findNotNullConstraint(Oid relid, const char *colname);
 extern HeapTuple findDomainNotNullConstraint(Oid typid);
 extern AttrNumber extractNotNullColumn(HeapTuple constrTup);
 extern int	AdjustNotNullInheritance1(Oid relid, AttrNumber attnum, int count,
-									  bool is_no_inherit);
+									  bool is_no_inherit, bool allow_noinherit_change);
 extern void AdjustNotNullInheritance(Oid relid, Bitmapset *columns, int count);
 extern List *RelationGetNotNullConstraints(Oid relid, bool cooked);
 
diff --git a/src/test/regress/expected/inherit.out b/src/test/regress/expected/inherit.out
index 5a5c23fc3b..203ac6c52e 100644
--- a/src/test/regress/expected/inherit.out
+++ b/src/test/regress/expected/inherit.out
@@ -2126,7 +2126,7 @@ ERROR:  cannot define not-null constraint on column "a2" with NO INHERIT
 DETAIL:  The column has an inherited not-null constraint.
 -- change NO INHERIT status of inherited constraint: no dice, it's inherited
 alter table cc2 add not null a2 no inherit;
-ERROR:  cannot change NO INHERIT status of inherited NOT NULL constraint "nn" on relation "cc2"
+ERROR:  cannot change NO INHERIT status of NOT NULL constraint "nn" on relation "cc2"
 -- remove constraint from cc2: no dice, it's inherited
 alter table cc2 alter column a2 drop not null;
 ERROR:  cannot drop inherited constraint "nn" of relation "cc2"
-- 
2.39.2


--pq2l62po3vnphnkg--