From 0325d7a5957ba39a0dce90835ab54a08ab8bf762 Mon Sep 17 00:00:00 2001 From: Alvaro Herrera Date: Fri, 18 Jan 2019 14:49:40 -0300 Subject: [PATCH] Fix creation of duplicate foreign keys on partitions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When creating a foreign key in a partitioned table, if some partitions already have equivalent constraints, we wastefully create duplicates of the constraints instead of attaching to the existing ones. That's inconsistent with the de-duplication that is applied when a table is attached as a partition. To fix, reuse the FK-cloning code instead of having a separate code path. Backpatch to Postgres 11. This is a subtle behavior change, but surely a welcome one since there's no use in having duplicate foreign keys. Discovered by Álvaro Herrera while thinking about a different problem reported by Jesper Pedersen (bug #15587). Author: Álvaro Herrera Discussion: https://postgr.es/m/201901151935.zfadrzvyof4k@alvherre.pgsql --- src/backend/commands/tablecmds.c | 43 ++++++++---- src/test/regress/expected/foreign_key.out | 83 +++++++++++++++++++++++ src/test/regress/sql/foreign_key.sql | 38 +++++++++++ 3 files changed, 152 insertions(+), 12 deletions(-) diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index 1e108fd61b..1a1ac698e5 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -7658,31 +7658,50 @@ ATAddForeignKeyConstraint(List **wqueue, AlteredTableInfo *tab, Relation rel, if (recurse && rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE) { PartitionDesc partdesc; + Relation pg_constraint; + List *cloned = NIL; + ListCell *cell; + pg_constraint = heap_open(ConstraintRelationId, RowExclusiveLock); partdesc = RelationGetPartitionDesc(rel); for (i = 0; i < partdesc->nparts; i++) { Oid partitionId = partdesc->oids[i]; Relation partition = heap_open(partitionId, lockmode); - AlteredTableInfo *childtab; - ObjectAddress childAddr; CheckTableNotInUse(partition, "ALTER TABLE"); - /* Find or create work queue entry for this table */ - childtab = ATGetQueueEntry(wqueue, partition); - - childAddr = - ATAddForeignKeyConstraint(wqueue, childtab, partition, - fkconstraint, constrOid, - recurse, true, lockmode); - - /* Record this constraint as dependent on the parent one */ - recordDependencyOn(&childAddr, &address, DEPENDENCY_INTERNAL_AUTO); + CloneFkReferencing(pg_constraint, rel, partition, + list_make1_oid(constrOid), + &cloned); heap_close(partition, NoLock); } + heap_close(pg_constraint, RowExclusiveLock); + + foreach(cell, cloned) + { + ClonedConstraint *cc = (ClonedConstraint *) lfirst(cell); + Relation partition = heap_open(cc->relid, lockmode); + AlteredTableInfo *childtab; + NewConstraint *newcon; + + /* Find or create work queue entry for this partition */ + childtab = ATGetQueueEntry(wqueue, partition); + + newcon = (NewConstraint *) palloc0(sizeof(NewConstraint)); + newcon->name = cc->constraint->conname; + newcon->contype = CONSTR_FOREIGN; + newcon->refrelid = cc->refrelid; + newcon->refindid = cc->conindid; + newcon->conid = cc->conid; + newcon->qual = (Node *) fkconstraint; + + childtab->constraints = lappend(childtab->constraints, newcon); + + heap_close(partition, lockmode); + } } /* diff --git a/src/test/regress/expected/foreign_key.out b/src/test/regress/expected/foreign_key.out index 52d540e896..f85c2ef78a 100644 --- a/src/test/regress/expected/foreign_key.out +++ b/src/test/regress/expected/foreign_key.out @@ -1842,3 +1842,86 @@ INSERT INTO fk_notpartitioned_pk VALUES (1600, 601), (1600, 1601); ALTER TABLE fk_partitioned_fk ATTACH PARTITION fk_partitioned_fk_2 FOR VALUES IN (1600); -- leave these tables around intentionally +-- Test creating a constraint at the parent that already exists in partitions. +-- There should be no duplicated constraints, and attempts to drop the +-- constraint in partitions should raise appropriate errors. +create schema fkpart0 + create table pkey (a int primary key) + create table fk_part (a int) partition by list (a) + create table fk_part_1 partition of fk_part + (foreign key (a) references fkpart0.pkey) for values in (1) + create table fk_part_23 partition of fk_part + (foreign key (a) references fkpart0.pkey) for values in (2, 3) + partition by list (a) + create table fk_part_23_2 partition of fk_part_23 for values in (2); +alter table fkpart0.fk_part add foreign key (a) references fkpart0.pkey; +\d fkpart0.fk_part_1 \\ -- should have only one FK + Table "fkpart0.fk_part_1" + Column | Type | Collation | Nullable | Default +--------+---------+-----------+----------+--------- + a | integer | | | +Partition of: fkpart0.fk_part FOR VALUES IN (1) +Foreign-key constraints: + "fk_part_1_a_fkey" FOREIGN KEY (a) REFERENCES fkpart0.pkey(a) + +alter table fkpart0.fk_part_1 drop constraint fk_part_1_a_fkey; +ERROR: cannot drop inherited constraint "fk_part_1_a_fkey" of relation "fk_part_1" +\d fkpart0.fk_part_23 \\ -- should have only one FK + Partitioned table "fkpart0.fk_part_23" + Column | Type | Collation | Nullable | Default +--------+---------+-----------+----------+--------- + a | integer | | | +Partition of: fkpart0.fk_part FOR VALUES IN (2, 3) +Partition key: LIST (a) +Foreign-key constraints: + "fk_part_23_a_fkey" FOREIGN KEY (a) REFERENCES fkpart0.pkey(a) +Number of partitions: 1 (Use \d+ to list them.) + +\d fkpart0.fk_part_23_2 \\ -- should have only one FK + Table "fkpart0.fk_part_23_2" + Column | Type | Collation | Nullable | Default +--------+---------+-----------+----------+--------- + a | integer | | | +Partition of: fkpart0.fk_part_23 FOR VALUES IN (2) +Foreign-key constraints: + "fk_part_23_a_fkey" FOREIGN KEY (a) REFERENCES fkpart0.pkey(a) + +alter table fkpart0.fk_part_23 drop constraint fk_part_23_a_fkey; +ERROR: cannot drop inherited constraint "fk_part_23_a_fkey" of relation "fk_part_23" +alter table fkpart0.fk_part_23_2 drop constraint fk_part_23_a_fkey; +ERROR: cannot drop inherited constraint "fk_part_23_a_fkey" of relation "fk_part_23_2" +create table fkpart0.fk_part_4 partition of fkpart0.fk_part for values in (4); +\d fkpart0.fk_part_4 + Table "fkpart0.fk_part_4" + Column | Type | Collation | Nullable | Default +--------+---------+-----------+----------+--------- + a | integer | | | +Partition of: fkpart0.fk_part FOR VALUES IN (4) +Foreign-key constraints: + "fk_part_a_fkey" FOREIGN KEY (a) REFERENCES fkpart0.pkey(a) + +alter table fkpart0.fk_part_4 drop constraint fk_part_a_fkey; +ERROR: cannot drop inherited constraint "fk_part_a_fkey" of relation "fk_part_4" +create table fkpart0.fk_part_56 partition of fkpart0.fk_part + for values in (5,6) partition by list (a); +create table fkpart0.fk_part_56_5 partition of fkpart0.fk_part_56 + for values in (5); +\d fkpart0.fk_part_56 + Partitioned table "fkpart0.fk_part_56" + Column | Type | Collation | Nullable | Default +--------+---------+-----------+----------+--------- + a | integer | | | +Partition of: fkpart0.fk_part FOR VALUES IN (5, 6) +Partition key: LIST (a) +Foreign-key constraints: + "fk_part_a_fkey" FOREIGN KEY (a) REFERENCES fkpart0.pkey(a) +Number of partitions: 1 (Use \d+ to list them.) + +alter table fkpart0.fk_part_56 drop constraint fk_part_a_fkey; +ERROR: cannot drop inherited constraint "fk_part_a_fkey" of relation "fk_part_56" +alter table fkpart0.fk_part_56_5 drop constraint fk_part_a_fkey; +ERROR: cannot drop inherited constraint "fk_part_a_fkey" of relation "fk_part_56_5" +\set VERBOSITY terse \\ -- suppress cascade details +drop schema fkpart0 cascade; +NOTICE: drop cascades to 2 other objects +\set VERBOSITY default diff --git a/src/test/regress/sql/foreign_key.sql b/src/test/regress/sql/foreign_key.sql index d1f3d6e525..c3a7cfcc01 100644 --- a/src/test/regress/sql/foreign_key.sql +++ b/src/test/regress/sql/foreign_key.sql @@ -1340,3 +1340,41 @@ ALTER TABLE fk_partitioned_fk ATTACH PARTITION fk_partitioned_fk_2 FOR VALUES IN (1600); -- leave these tables around intentionally + +-- Test creating a constraint at the parent that already exists in partitions. +-- There should be no duplicated constraints, and attempts to drop the +-- constraint in partitions should raise appropriate errors. +create schema fkpart0 + create table pkey (a int primary key) + create table fk_part (a int) partition by list (a) + create table fk_part_1 partition of fk_part + (foreign key (a) references fkpart0.pkey) for values in (1) + create table fk_part_23 partition of fk_part + (foreign key (a) references fkpart0.pkey) for values in (2, 3) + partition by list (a) + create table fk_part_23_2 partition of fk_part_23 for values in (2); + +alter table fkpart0.fk_part add foreign key (a) references fkpart0.pkey; +\d fkpart0.fk_part_1 \\ -- should have only one FK +alter table fkpart0.fk_part_1 drop constraint fk_part_1_a_fkey; + +\d fkpart0.fk_part_23 \\ -- should have only one FK +\d fkpart0.fk_part_23_2 \\ -- should have only one FK +alter table fkpart0.fk_part_23 drop constraint fk_part_23_a_fkey; +alter table fkpart0.fk_part_23_2 drop constraint fk_part_23_a_fkey; + +create table fkpart0.fk_part_4 partition of fkpart0.fk_part for values in (4); +\d fkpart0.fk_part_4 +alter table fkpart0.fk_part_4 drop constraint fk_part_a_fkey; + +create table fkpart0.fk_part_56 partition of fkpart0.fk_part + for values in (5,6) partition by list (a); +create table fkpart0.fk_part_56_5 partition of fkpart0.fk_part_56 + for values in (5); +\d fkpart0.fk_part_56 +alter table fkpart0.fk_part_56 drop constraint fk_part_a_fkey; +alter table fkpart0.fk_part_56_5 drop constraint fk_part_a_fkey; + +\set VERBOSITY terse \\ -- suppress cascade details +drop schema fkpart0 cascade; +\set VERBOSITY default