Feedback-ID: ia2694551:Fastmail
Date: Mon, 22 Apr 2024 12:22:23 +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: <202404221022.nljgb5dwzppl@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="wwicd52hioo5unt5"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <5b4cd32f-1d5b-c080-c688-31736bbcd739@gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/202404221022.nljgb5dwzppl%40alvherre.pgsql>
Precedence: bulk


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

Hi Alexander,

On 2024-Apr-18, Alexander Lakhin wrote:

> 18.04.2024 16:39, Alvaro Herrera wrote:
> > I have pushed a fix which should hopefully fix this problem
> > (d9f686a72e).  Please give this a look.  Thanks for reporting the issue.
> 
> Please look at an assertion failure, introduced with d9f686a72:
> CREATE TABLE t(a int, NOT NULL a NO INHERIT);
> CREATE TABLE t2() INHERITS (t);
> 
> ALTER TABLE t ADD CONSTRAINT nna NOT NULL a;
> TRAP: failed Assert("lockmode != NoLock || IsBootstrapProcessingMode() ||
> CheckRelationLockedByMe(r, AccessShareLock, true)"), File: "relation.c",
> Line: 67, PID: 2980258

Ah, of course -- we're missing acquiring locks during the prep phase for
the recursive case of ADD CONSTRAINT.  So we just need to add
find_all_inheritors() to do so in the AT_AddConstraint case in
ATPrepCmd().  However these naked find_all_inheritors() call look a bit
ugly to me, so I couldn't resist the temptation of adding a static
function ATLockAllDescendants to clean it up a bit.  I'll also add your
script to the tests and push shortly.

> 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.

Also I just noticed that in 9b581c534186 (which introduced this error
message) I used ERRCODE_DATATYPE_MISMATCH ... Is that really appropriate
here?

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"Once again, thank you and all of the developers for your hard work on
PostgreSQL.  This is by far the most pleasant management experience of
any database I've worked on."                             (Dan Harris)
http://archives.postgresql.org/pgsql-performance/2006-04/msg00247.php

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

From 35b485b72d0675d631cde9f95e65d9c0db9254b8 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] Acquire locks on children before recursing

ALTER TABLE ADD CONSTRAINT was missing this, as evidenced by assertion
failures.

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

diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 3556240c8e..8941181912 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,15 @@ 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)
+			{
+				/*
+				 * Make note for execution phase about need for recursion;
+				 * also acquire lock on all descendants.
+				 */
+				ATLockAllDescendants(RelationGetRelid(rel), lockmode);
 				cmd->recurse = true;
+			}
 			pass = AT_PASS_ADD_CONSTR;
 			break;
 		case AT_AddIndexConstraint: /* ADD CONSTRAINT USING INDEX */
@@ -6721,6 +6725,17 @@ ATSimpleRecursion(List **wqueue, Relation rel,
 	}
 }
 
+/*
+ * ATLockAllDescendants
+ *
+ * Acquire lock on all descendants of the given relation.
+ */
+static void
+ATLockAllDescendants(Oid relid, LOCKMODE lockmode)
+{
+	(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 +9385,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
@@ -11258,8 +11272,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


--wwicd52hioo5unt5--