Feedback-ID: ia2694551:Fastmail
Date: Sat, 26 Oct 2024 23:47:08 +0200
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: <202410262147.ivdk6hbdearn@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="csabzletllir36uk"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <202410250830.puisbzuy4yyj@alvherre.pgsql>
Archived-At: 
 <https://www.postgresql.org/message-id/202410262147.ivdk6hbdearn%40alvherre.pgsql>
Precedence: bulk


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

On 2024-Oct-25, Alvaro Herrera wrote:

> On 2024-Oct-25, Tender Wang wrote:
> 
> > Thanks for reporting. I can reproduce this memory invalid access on HEAD.
> > After debuging codes, I found the root cause.
> >  In DetachPartitionFinalize(), below code:
> > att = TupleDescAttr(RelationGetDescr(partRel),
> >                                attmap->attnums[conkey[i] - 1] - 1);
> > 
> > We should use confkey[i] -1 not conkey[i] - 1;
> 
> Wow, how embarrasing, now that's one _really_ stupid bug and it's
> totally my own.  Thanks for the analysis!  I'll get this patched soon.

Actually, now that I look at this again, I think this proposed fix is
wrong; conkey/confkey confusion is not what the problem is.  Rather, the
problem is that we're applying a tuple conversion map when none should
be applied.  So the fix is to remove "attmap" altogether.  One thing
that didn't appear correct to me was that the patch was changing one
constraint name so that it appeared that the constrained columns were
"id, p_id" but that didn't make sense: they are "p_id, p_jd" instead.
Then I realized that you're wrong that it's the referenced side that
we're processing: what we're doing there is generate the fk_attrs list,
which is the list of constrained columns (not the list of referenced
columns, which is pk_attrs).

I also felt like modifying the fkpart12 test rather than adding a
separate fkpart13, so I did that.

So here's a v2 for this.  (Commit message needs love still, but it's a
bit late here.)

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"[PostgreSQL] is a great group; in my opinion it is THE best open source
development communities in existence anywhere."                (Lamar Owen)

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

From bb2aef5cbfd12bf47d49fdf58f303048f0c50339 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 v2] 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          |  8 +++-----
 src/test/regress/expected/foreign_key.out |  9 ++++++---
 src/test/regress/sql/foreign_key.sql      | 10 +++++++---
 3 files changed, 16 insertions(+), 11 deletions(-)

diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index e14bc0c0548..acb7b002940 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -19502,7 +19502,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,15 +19534,14 @@ 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)));
 			}
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


--csabzletllir36uk--