Feedback-ID: ia2694551:Fastmail
Date: Mon, 28 Oct 2024 10:16:17 +0100
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
To: Tender Wang <tndrwang@gmail.com>
Cc: Alexander Lakhin <exclusion@gmail.com>,
	Jehan-Guillaume de Rorthais <jgdr@dalibo.com>,
 "pgsql-hackers@postgresql.org" <pgsql-hackers@postgresql.org>,
	Guillaume Lelarge <guillaume@lelarge.info>
Subject: Re: [BUG] Fix DETACH with FK pointing to a partitioned table fails
Message-ID: <202410280916.7zm2gv25farn@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="venjkcsjyivuur74"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: 
 <CAHewXNk8hn_Kq=3gb583O1U1WU=T+VMRF8NDZfTa35wxcJ+xcA@mail.gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/202410280916.7zm2gv25farn%40alvherre.pgsql>
Precedence: bulk


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

On 2024-Oct-27, Tender Wang wrote:

> BTW, while reviewing the v2 patch, I found the parentConTup in
> foreach(cell, fks) block
> didn't need it anymore. We can remove the related codes.

True.  Done so in this v3.

I noticed another problem here: we're grabbing the wrong lock type on
the referenced rel (AccessShareLock) during detach.  (What's more: we
release it afterwards, which is the wrong thing to do.  We need to keep
such locks until end of transaction).  I didn't try to construct a case
where this would be a problem, but if I change AccessShare to NoLock,
the assertion that says we don't hold _any_ lock on that relation fires,
which means that we're not taking any locks on those rels before this
point.  So this lock strength choice is definitely wrong.  I changed it
to ShareRowExclusive, which is what we're suppose to use when adding a
trigger.  Another option might be to do find_all_inheritors() ahead of
time to grab all the necessary locks, but I didn't try to do that.  I
also added an assertion in addFkRecurseReferenced to verify that we hold
that in all paths, and after this change it doesn't fire anymore with
the regression tests.

I have still not edited the commit message.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/

--venjkcsjyivuur74
Content-Type: text/x-diff; charset=utf-8
Content-Disposition: attachment;
	filename="v3-0001-No-need-to-use-an-attrmap-when-detaching-a-foreig.patch"

From 3f18de0a263e507b79b1f1eb9a9a128cd6292779 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?=C3=81lvaro=20Herrera?= <alvherre@alvh.no-ip.org>
Date: Sat, 26 Oct 2024 23:44:58 +0200
Subject: [PATCH v3] No need to use an attrmap when detaching a foreign key

The reason is that the constraint being created is on the same relation
as the constraint that it spawns from.
---
 src/backend/commands/tablecmds.c          | 25 ++++++++---------------
 src/test/regress/expected/foreign_key.out |  9 +++++---
 src/test/regress/sql/foreign_key.sql      | 10 ++++++---
 3 files changed, 21 insertions(+), 23 deletions(-)

diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index e14bc0c0548..c993abc9792 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -10372,6 +10372,8 @@ addFkRecurseReferenced(Constraint *fkconstraint, Relation rel,
 	Oid			deleteTriggerOid,
 				updateTriggerOid;
 
+	Assert(CheckRelationLockedByMe(pkrel, ShareRowExclusiveLock, true));
+
 	/*
 	 * Create the action triggers that enforce the constraint.
 	 */
@@ -19436,8 +19438,7 @@ DetachPartitionFinalize(Relation rel, Relation partRel, bool concurrent,
 	foreach(cell, fks)
 	{
 		ForeignKeyCacheInfo *fk = lfirst(cell);
-		HeapTuple	contup,
-					parentConTup;
+		HeapTuple	contup;
 		Form_pg_constraint conform;
 		Oid			insertTriggerOid,
 					updateTriggerOid;
@@ -19455,13 +19456,6 @@ DetachPartitionFinalize(Relation rel, Relation partRel, bool concurrent,
 			continue;
 		}
 
-		Assert(OidIsValid(conform->conparentid));
-		parentConTup = SearchSysCache1(CONSTROID,
-									   ObjectIdGetDatum(conform->conparentid));
-		if (!HeapTupleIsValid(parentConTup))
-			elog(ERROR, "cache lookup failed for constraint %u",
-				 conform->conparentid);
-
 		/*
 		 * The constraint on this table must be marked no longer a child of
 		 * the parent's constraint, as do its check triggers.
@@ -19502,7 +19496,6 @@ DetachPartitionFinalize(Relation rel, Relation partRel, bool concurrent,
 			Oid			conffeqop[INDEX_MAX_KEYS];
 			int			numfkdelsetcols;
 			AttrNumber	confdelsetcols[INDEX_MAX_KEYS];
-			AttrMap    *attmap;
 			Relation	refdRel;
 
 			DeconstructFkConstraintRow(contup,
@@ -19535,20 +19528,19 @@ DetachPartitionFinalize(Relation rel, Relation partRel, bool concurrent,
 			fkconstraint->old_pktable_oid = InvalidOid;
 			fkconstraint->location = -1;
 
-			attmap = build_attrmap_by_name(RelationGetDescr(partRel),
-										   RelationGetDescr(rel),
-										   false);
+			/* set up colnames, used to generate the constraint name */
 			for (int i = 0; i < numfks; i++)
 			{
 				Form_pg_attribute att;
 
 				att = TupleDescAttr(RelationGetDescr(partRel),
-									attmap->attnums[conkey[i] - 1] - 1);
+									conkey[i] - 1);
+
 				fkconstraint->fk_attrs = lappend(fkconstraint->fk_attrs,
 												 makeString(NameStr(att->attname)));
 			}
 
-			refdRel = table_open(fk->confrelid, AccessShareLock);
+			refdRel = table_open(fk->confrelid, ShareRowExclusiveLock);
 
 			addFkRecurseReferenced(fkconstraint, partRel,
 								   refdRel,
@@ -19565,11 +19557,10 @@ DetachPartitionFinalize(Relation rel, Relation partRel, bool concurrent,
 								   true,
 								   InvalidOid, InvalidOid,
 								   conform->conperiod);
-			table_close(refdRel, AccessShareLock);
+			table_close(refdRel, NoLock);	/* keep lock till end of xact */
 		}
 
 		ReleaseSysCache(contup);
-		ReleaseSysCache(parentConTup);
 	}
 	list_free_deep(fks);
 	if (trigrel)
diff --git a/src/test/regress/expected/foreign_key.out b/src/test/regress/expected/foreign_key.out
index b73e7dced8f..69994c98e32 100644
--- a/src/test/regress/expected/foreign_key.out
+++ b/src/test/regress/expected/foreign_key.out
@@ -2944,17 +2944,20 @@ CREATE SCHEMA fkpart12
   CREATE TABLE fk_p ( id int, jd int, PRIMARY KEY(id, jd)) PARTITION BY list (id)
   CREATE TABLE fk_p_1 PARTITION OF fk_p FOR VALUES IN (1) PARTITION BY list (jd)
   CREATE TABLE fk_p_1_1 PARTITION OF fk_p_1 FOR VALUES IN (1)
-  CREATE TABLE fk_p_1_2 PARTITION OF fk_p_1 FOR VALUES IN (2)
+  CREATE TABLE fk_p_1_2 (x int, y int, jd int NOT NULL, id int NOT NULL)
   CREATE TABLE fk_p_2 PARTITION OF fk_p FOR VALUES IN (2) PARTITION BY list (jd)
   CREATE TABLE fk_p_2_1 PARTITION OF fk_p_2 FOR VALUES IN (1)
   CREATE TABLE fk_p_2_2 PARTITION OF fk_p_2 FOR VALUES IN (2)
-  CREATE TABLE fk_r_1 ( id int PRIMARY KEY, p_id int NOT NULL, p_jd int NOT NULL)
+  CREATE TABLE fk_r_1 ( p_jd int NOT NULL, x int, id int PRIMARY KEY, p_id int NOT NULL)
   CREATE TABLE fk_r_2 ( id int PRIMARY KEY, p_id int NOT NULL, p_jd int NOT NULL) PARTITION BY list (id)
   CREATE TABLE fk_r_2_1 PARTITION OF fk_r_2 FOR VALUES IN (2, 1)
   CREATE TABLE fk_r   ( id int PRIMARY KEY, p_id int NOT NULL, p_jd int NOT NULL,
        FOREIGN KEY (p_id, p_jd) REFERENCES fk_p (id, jd)
   ) PARTITION BY list (id);
 SET search_path TO fkpart12;
+ALTER TABLE fk_p_1_2 DROP COLUMN x, DROP COLUMN y;
+ALTER TABLE fk_p_1 ATTACH PARTITION fk_p_1_2 FOR VALUES IN (2);
+ALTER TABLE fk_r_1 DROP COLUMN x;
 INSERT INTO fk_p VALUES (1, 1);
 ALTER TABLE fk_r ATTACH PARTITION fk_r_1 FOR VALUES IN (1);
 ALTER TABLE fk_r ATTACH PARTITION fk_r_2 FOR VALUES IN (2);
@@ -2993,7 +2996,7 @@ Foreign-key constraints:
     "fk_r_p_id_p_jd_fkey" FOREIGN KEY (p_id, p_jd) REFERENCES fk_p(id, jd)
 Number of partitions: 1 (Use \d+ to list them.)
 
-INSERT INTO fk_r_1 VALUES (2, 1, 2); -- should fail
+INSERT INTO fk_r_1 (id, p_id, p_jd) VALUES (2, 1, 2); -- should fail
 ERROR:  insert or update on table "fk_r_1" violates foreign key constraint "fk_r_p_id_p_jd_fkey"
 DETAIL:  Key (p_id, p_jd)=(1, 2) is not present in table "fk_p".
 DELETE FROM fk_p; -- should fail
diff --git a/src/test/regress/sql/foreign_key.sql b/src/test/regress/sql/foreign_key.sql
index 9b2a6b6bff7..2e710e419c2 100644
--- a/src/test/regress/sql/foreign_key.sql
+++ b/src/test/regress/sql/foreign_key.sql
@@ -2097,11 +2097,11 @@ CREATE SCHEMA fkpart12
   CREATE TABLE fk_p ( id int, jd int, PRIMARY KEY(id, jd)) PARTITION BY list (id)
   CREATE TABLE fk_p_1 PARTITION OF fk_p FOR VALUES IN (1) PARTITION BY list (jd)
   CREATE TABLE fk_p_1_1 PARTITION OF fk_p_1 FOR VALUES IN (1)
-  CREATE TABLE fk_p_1_2 PARTITION OF fk_p_1 FOR VALUES IN (2)
+  CREATE TABLE fk_p_1_2 (x int, y int, jd int NOT NULL, id int NOT NULL)
   CREATE TABLE fk_p_2 PARTITION OF fk_p FOR VALUES IN (2) PARTITION BY list (jd)
   CREATE TABLE fk_p_2_1 PARTITION OF fk_p_2 FOR VALUES IN (1)
   CREATE TABLE fk_p_2_2 PARTITION OF fk_p_2 FOR VALUES IN (2)
-  CREATE TABLE fk_r_1 ( id int PRIMARY KEY, p_id int NOT NULL, p_jd int NOT NULL)
+  CREATE TABLE fk_r_1 ( p_jd int NOT NULL, x int, id int PRIMARY KEY, p_id int NOT NULL)
   CREATE TABLE fk_r_2 ( id int PRIMARY KEY, p_id int NOT NULL, p_jd int NOT NULL) PARTITION BY list (id)
   CREATE TABLE fk_r_2_1 PARTITION OF fk_r_2 FOR VALUES IN (2, 1)
   CREATE TABLE fk_r   ( id int PRIMARY KEY, p_id int NOT NULL, p_jd int NOT NULL,
@@ -2109,6 +2109,10 @@ CREATE SCHEMA fkpart12
   ) PARTITION BY list (id);
 SET search_path TO fkpart12;
 
+ALTER TABLE fk_p_1_2 DROP COLUMN x, DROP COLUMN y;
+ALTER TABLE fk_p_1 ATTACH PARTITION fk_p_1_2 FOR VALUES IN (2);
+ALTER TABLE fk_r_1 DROP COLUMN x;
+
 INSERT INTO fk_p VALUES (1, 1);
 
 ALTER TABLE fk_r ATTACH PARTITION fk_r_1 FOR VALUES IN (1);
@@ -2124,7 +2128,7 @@ ALTER TABLE fk_r DETACH PARTITION fk_r_2;
 
 \d fk_r_2
 
-INSERT INTO fk_r_1 VALUES (2, 1, 2); -- should fail
+INSERT INTO fk_r_1 (id, p_id, p_jd) VALUES (2, 1, 2); -- should fail
 DELETE FROM fk_p; -- should fail
 
 ALTER TABLE fk_r ATTACH PARTITION fk_r_1 FOR VALUES IN (1);
-- 
2.39.5


--venjkcsjyivuur74--