Date: Mon, 15 Mar 2021 20:04:37 -0300
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
To: Justin Pryzby <pryzby@telsasoft.com>
Cc: Amit Langote <amitlangote09@gmail.com>,
	pgsql-hackers@lists.postgresql.org
Subject: Re: ALTER TABLE .. DETACH PARTITION CONCURRENTLY
Message-ID: <20210315230437.GA20801@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="M9NhX3UHpAaciwkO"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20210226203236.GA30377@alvherre.pgsql>
User-Agent: Mutt/1.10.1 (2018-07-13)
Archived-At: 
 <https://www.postgresql.org/message-id/20210315230437.GA20801%40alvherre.pgsql>
Precedence: bulk


--M9NhX3UHpAaciwkO
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit

On 2021-Feb-26, Alvaro Herrera wrote:

> Hmm, but if we take this approach, then we're still vulnerable to the
> problem that somebody can do DETACH CONCURRENTLY and cancel the wait (or
> crash the server), then mess up the state before doing DETACH FINALIZE:
> when they cancel the wait, the lock will be released.
> 
> I think the right fix is to disallow any action on a partition which is
> pending detach other than DETACH FINALIZE.  (Didn't do that here.)

Here's a fixup patch to do it that way.  I tried running the commands
you showed and one of them immediately dies with the new error message;
I can't cause the bogus constraint to show up anymore.

I'll clean this up for a real submission tomorrow.

-- 
Álvaro Herrera                            39°49'30"S 73°17'W
"The Gord often wonders why people threaten never to come back after they've
been told never to return" (www.actsofgord.com)

--M9NhX3UHpAaciwkO
Content-Type: text/plain; charset=us-ascii
Content-Disposition: attachment; filename="dont.notpatch"

diff --git a/src/backend/catalog/pg_inherits.c b/src/backend/catalog/pg_inherits.c
index 3fd0880ca0..83093f9730 100644
--- a/src/backend/catalog/pg_inherits.c
+++ b/src/backend/catalog/pg_inherits.c
@@ -535,3 +535,39 @@ DeleteInheritsTuple(Oid inhrelid, Oid inhparent, bool detached,
 
 	return found;
 }
+
+bool
+PartitionHasPendingDetach(Oid partoid)
+{
+	Relation	catalogRelation;
+	ScanKeyData key;
+	SysScanDesc scan;
+	HeapTuple	inheritsTuple;
+
+	/*
+	 * Find pg_inherits entries by inhrelid.
+	 */
+	catalogRelation = table_open(InheritsRelationId, RowExclusiveLock);
+	ScanKeyInit(&key,
+				Anum_pg_inherits_inhrelid,
+				BTEqualStrategyNumber, F_OIDEQ,
+				ObjectIdGetDatum(partoid));
+	scan = systable_beginscan(catalogRelation, InheritsRelidSeqnoIndexId,
+							  true, NULL, 1, &key);
+
+	while (HeapTupleIsValid(inheritsTuple = systable_getnext(scan)))
+	{
+		bool	detached;
+
+		detached = ((Form_pg_inherits) GETSTRUCT(inheritsTuple))->inhdetached;
+
+		/* Done */
+		systable_endscan(scan);
+		table_close(catalogRelation, RowExclusiveLock);
+
+		return detached;
+	}
+
+	elog(ERROR, "relation %u is not a partition", partoid);
+	return false;				/* keep compiler quiet */
+}
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 5986b4dd56..5074c0ff2b 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -17106,6 +17106,8 @@ ATExecDetachPartition(List **wqueue, AlteredTableInfo *tab, Relation rel,
 	 */
 	partRel = table_openrv(name, concurrent ? ShareLock :
 						   AccessExclusiveLock);
+	if (PartitionHasPendingDetach(RelationGetRelid(partRel)))
+		elog(ERROR, "hey, don't!");
 
 	/*
 	 * Check inheritance conditions and either delete the pg_inherits row
diff --git a/src/include/catalog/pg_inherits.h b/src/include/catalog/pg_inherits.h
index 155596907c..19243ea9e4 100644
--- a/src/include/catalog/pg_inherits.h
+++ b/src/include/catalog/pg_inherits.h
@@ -61,5 +61,6 @@ extern void StoreSingleInheritance(Oid relationId, Oid parentOid,
 								   int32 seqNumber);
 extern bool DeleteInheritsTuple(Oid inhrelid, Oid inhparent, bool allow_detached,
 								const char *childname);
+extern bool PartitionHasPendingDetach(Oid partoid);
 
 #endif							/* PG_INHERITS_H */

--M9NhX3UHpAaciwkO--