Feedback-ID: ia2694551:Fastmail
Date: Thu, 22 Aug 2024 14:41:50 -0400
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
To: Tender Wang <tndrwang@gmail.com>
Cc: Junwang Zhao <zhjwpku@gmail.com>,
	Jehan-Guillaume de Rorthais <jgdr@dalibo.com>,
 "pgsql-hackers@postgresql.org" <pgsql-hackers@postgresql.org>,
	Alexander Lakhin <exclusion@gmail.com>,
 Baehler Thomas SBB CFF FFS <thomas.baehler2@sbb.ch>,
	Guillaume Lelarge <guillaume@lelarge.info>
Subject: Re: [BUG] Fix DETACH with FK pointing to a partitioned table fails
Message-ID: <202408221841.hecaenywo5nm@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="gxt7bkvuz5n2cmsd"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: 
 <CAHewXNmf6sCai+iYBSrTW8D8NWBaX+zNYV25=iHWM17zbEfF8g@mail.gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/202408221841.hecaenywo5nm%40alvherre.pgsql>
Precedence: bulk


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

On 2024-Aug-22, Tender Wang wrote:

> I apply the v14 patch on branch REL_14_STABLE. I run this thread issue and I
> find below error.
> [...]
> ERROR:  cache lookup failed for constraint 16400
> 
> I haven't look into details to find out where cause above error.

Right, we try to drop the constraint twice.  We can dodge this by
collecting all constraints to drop in the loop and process them in a
single performMultipleDeletions, as in the attached v14-2.

TBH I think it's a bit infuriating that we lose the constraint (which
was explicitly declared) because of ATTACH/DETACH.  So the behavior of
v15 and above is better.

> By the way, I run above SQL sequences on REL_14_STABLE without your
> partch.  I didn't find reporting error, and running oidjoins.sql
> didn't report warnings.  Do I miss something?

I think the action triggers are missing, so if you keep rows in the r_1
table after you've detached them, you can still delete them from the
referenced table p, instead of getting the error that you should get.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"In fact, the basic problem with Perl 5's subroutines is that they're not
crufty enough, so the cruft leaks out into user-defined code instead, by
the Conservation of Cruft Principle."  (Larry Wall, Apocalypse 6)

--gxt7bkvuz5n2cmsd
Content-Type: text/x-diff; charset=utf-8
Content-Disposition: attachment;
	filename="v14-2-0001-drop-constraint-when-detaching.patch"

From 43fc7215c1519e698fe1d58edd46f87da8529186 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?=C3=81lvaro=20Herrera?= <alvherre@alvh.no-ip.org>
Date: Wed, 21 Aug 2024 17:47:03 -0400
Subject: [PATCH v14-2] drop constraint when detaching

---
 src/backend/commands/tablecmds.c | 84 ++++++++++++++++++++++----------
 1 file changed, 58 insertions(+), 26 deletions(-)

diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index c39e6799ca..3a8ffe30eb 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -18129,6 +18129,7 @@ DetachPartitionFinalize(Relation rel, Relation partRel, bool concurrent,
 				new_repl[Natts_pg_class];
 	HeapTuple	tuple,
 				newtuple;
+	ObjectAddresses *dropobjs = NULL;
 
 	if (concurrent)
 	{
@@ -18143,8 +18144,8 @@ DetachPartitionFinalize(Relation rel, Relation partRel, bool concurrent,
 	DropClonedTriggersFromPartition(RelationGetRelid(partRel));
 
 	/*
-	 * Detach any foreign keys that are inherited.  This includes creating
-	 * additional action triggers.
+	 * Detach any foreign keys that are inherited -- or, if they reference
+	 * partitioned tables, drop them.
 	 */
 	fks = copyObject(RelationGetFKeyList(partRel));
 	foreach(cell, fks)
@@ -18152,7 +18153,6 @@ DetachPartitionFinalize(Relation rel, Relation partRel, bool concurrent,
 		ForeignKeyCacheInfo *fk = lfirst(cell);
 		HeapTuple	contup;
 		Form_pg_constraint conform;
-		Constraint *fkconstraint;
 
 		contup = SearchSysCache1(CONSTROID, ObjectIdGetDatum(fk->conoid));
 		if (!HeapTupleIsValid(contup))
@@ -18167,39 +18167,71 @@ DetachPartitionFinalize(Relation rel, Relation partRel, bool concurrent,
 			continue;
 		}
 
-		/* unset conparentid and adjust conislocal, coninhcount, etc. */
+		/* Mark the constraint as independent */
 		ConstraintSetParentConstraint(fk->conoid, InvalidOid, InvalidOid);
 
 		/*
-		 * Make the action triggers on the referenced relation.  When this was
-		 * a partition the action triggers pointed to the parent rel (they
-		 * still do), but now we need separate ones of our own.
+		 * If the constraint references a partitioned table, just drop the
+		 * constraint, because it's more work to preserve the constraint
+		 * correctly.
+		 *
+		 * If it references a plain table, then we can create the action
+		 * triggers and it'll be okay.
 		 */
-		fkconstraint = makeNode(Constraint);
-		fkconstraint->contype = CONSTRAINT_FOREIGN;
-		fkconstraint->conname = pstrdup(NameStr(conform->conname));
-		fkconstraint->deferrable = conform->condeferrable;
-		fkconstraint->initdeferred = conform->condeferred;
-		fkconstraint->location = -1;
-		fkconstraint->pktable = NULL;
-		fkconstraint->fk_attrs = NIL;
-		fkconstraint->pk_attrs = NIL;
-		fkconstraint->fk_matchtype = conform->confmatchtype;
-		fkconstraint->fk_upd_action = conform->confupdtype;
-		fkconstraint->fk_del_action = conform->confdeltype;
-		fkconstraint->old_conpfeqop = NIL;
-		fkconstraint->old_pktable_oid = InvalidOid;
-		fkconstraint->skip_validation = false;
-		fkconstraint->initially_valid = true;
+		if (get_rel_relkind(fk->confrelid) == RELKIND_PARTITIONED_TABLE)
+		{
+			ObjectAddress constraddr;
 
-		createForeignKeyActionTriggers(partRel, conform->confrelid,
-									   fkconstraint, fk->conoid,
-									   conform->conindid);
+			/* make the dependency deletions above visible */
+			CommandCounterIncrement();
+
+			/*
+			 * Remember the constraint and its triggers for later deletion.
+			 */
+			if (dropobjs == NULL)
+				dropobjs = new_object_addresses();
+			ObjectAddressSet(constraddr, ConstraintRelationId, fk->conoid);
+			add_exact_object_address(&constraddr, dropobjs);
+		}
+		else
+		{
+			Constraint *fkconstraint;
+
+			/*
+			 * Make the action triggers on the referenced relation.  When this
+			 * was a partition the action triggers pointed to the parent rel
+			 * (they still do), but now we need separate ones of our own.
+			 */
+			fkconstraint = makeNode(Constraint);
+			fkconstraint->contype = CONSTRAINT_FOREIGN;
+			fkconstraint->conname = pstrdup(NameStr(conform->conname));
+			fkconstraint->deferrable = conform->condeferrable;
+			fkconstraint->initdeferred = conform->condeferred;
+			fkconstraint->location = -1;
+			fkconstraint->pktable = NULL;
+			fkconstraint->fk_attrs = NIL;
+			fkconstraint->pk_attrs = NIL;
+			fkconstraint->fk_matchtype = conform->confmatchtype;
+			fkconstraint->fk_upd_action = conform->confupdtype;
+			fkconstraint->fk_del_action = conform->confdeltype;
+			fkconstraint->old_conpfeqop = NIL;
+			fkconstraint->old_pktable_oid = InvalidOid;
+			fkconstraint->skip_validation = false;
+			fkconstraint->initially_valid = true;
+
+			createForeignKeyActionTriggers(partRel, conform->confrelid,
+										   fkconstraint, fk->conoid,
+										   conform->conindid);
+		}
 
 		ReleaseSysCache(contup);
 	}
 	list_free_deep(fks);
 
+	/* If we collected any constraints for deletion, do so now. */
+	if (dropobjs != NULL)
+		performMultipleDeletions(dropobjs, DROP_CASCADE, 0);
+
 	/*
 	 * Any sub-constraints that are in the referenced-side of a larger
 	 * constraint have to be removed.  This partition is no longer part of the
-- 
2.39.2


--gxt7bkvuz5n2cmsd--