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/
>From 3f18de0a263e507b79b1f1eb9a9a128cd6292779 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?=C3=81lvaro=20Herrera?= <alvhe...@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
