From 55173d2e663fbe32430665ce7bd65a47856dc237 Mon Sep 17 00:00:00 2001 From: Alvaro Herrera Date: Fri, 7 Feb 2020 18:27:18 -0300 Subject: [PATCH] Fix failure to create FKs correctly in partitions MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit On a multi-level partioned table, when adding a partition not directly connected to the root table, foreign key constraints referencing the root were not cloned to the new partition, leading to the FK being possibly inadvertently violated later on. This was caused by fuzzy thinking in CloneFkReferenced (commit f56f8f8da6af): it was skipping constraints marked as having parents on the theory that cloning those would create duplicates; but that's only correct for the top level of the partitioning hierarchy. For levels below that one, such constraints must still be considered and only skipped if later on we see that we'd create duplicates. Apparently, I (Álvaro) wrote the comments right but the code implemented something slightly different. Author: Jehan-Guillaume de Rorthais Discussion: https://postgr.es/m/20200206004948.238352db@firost --- src/backend/commands/tablecmds.c | 28 ++++++++++++++--------- src/test/regress/expected/foreign_key.out | 24 +++++++++++++++++++ src/test/regress/sql/foreign_key.sql | 18 +++++++++++++++ 3 files changed, 59 insertions(+), 11 deletions(-) diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index f5993934736..b7c8d663fc4 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -8962,13 +8962,13 @@ CloneFkReferenced(Relation parentRel, Relation partitionRel) List *clone = NIL; /* - * Search for any constraints where this partition is in the referenced - * side. However, we must ignore any constraint whose parent constraint - * is also going to be cloned, to avoid duplicates. So do it in two - * steps: first construct the list of constraints to clone, then go over - * that list cloning those whose parents are not in the list. (We must - * not rely on the parent being seen first, since the catalog scan could - * return children first.) + * Search for any constraints where this partition's parent is in the + * referenced side. However, we must not clone any constraint whose + * parent constraint is also going to be cloned, to avoid duplicates. So + * do it in two steps: first construct the list of constraints to clone, + * then go over that list cloning those whose parents are not in the list. + * (We must not rely on the parent being seen first, since the catalog + * scan could return children first.) */ pg_constraint = table_open(ConstraintRelationId, RowShareLock); ScanKeyInit(&key[0], @@ -8984,10 +8984,6 @@ CloneFkReferenced(Relation parentRel, Relation partitionRel) { Form_pg_constraint constrForm = (Form_pg_constraint) GETSTRUCT(tuple); - /* Only try to clone the top-level constraint; skip child ones. */ - if (constrForm->conparentid != InvalidOid) - continue; - clone = lappend_oid(clone, constrForm->oid); } systable_endscan(scan); @@ -9016,6 +9012,16 @@ CloneFkReferenced(Relation parentRel, Relation partitionRel) elog(ERROR, "cache lookup failed for constraint %u", constrOid); constrForm = (Form_pg_constraint) GETSTRUCT(tuple); + /* + * As explained above: don't try to clone a constraint for which we're + * going to clone the parent. + */ + if (list_member_oid(clone, constrForm->conparentid)) + { + ReleaseSysCache(tuple); + continue; + } + /* * Because we're only expanding the key space at the referenced side, * we don't need to prevent any operation in the referencing table, so diff --git a/src/test/regress/expected/foreign_key.out b/src/test/regress/expected/foreign_key.out index 9f2dda499ef..9e1d7496014 100644 --- a/src/test/regress/expected/foreign_key.out +++ b/src/test/regress/expected/foreign_key.out @@ -2444,3 +2444,27 @@ DROP SCHEMA fkpart8 CASCADE; NOTICE: drop cascades to 2 other objects DETAIL: drop cascades to table fkpart8.tbl1 drop cascades to table fkpart8.tbl2 +-- ensure FK referencing a multi-level partitioned table are +-- enforce reference to sub-children. +CREATE SCHEMA fkpart9 + CREATE TABLE pk (a INT PRIMARY KEY) PARTITION BY RANGE (a) + CREATE TABLE fk ( + fk_a INT REFERENCES pk(a) ON DELETE CASCADE + ) + CREATE TABLE pk1 PARTITION OF pk FOR VALUES FROM (30) TO (50) PARTITION BY RANGE (a) + CREATE TABLE pk11 PARTITION OF pk1 FOR VALUES FROM (30) TO (40); +INSERT INTO fkpart9.pk VALUES (35); +INSERT INTO fkpart9.fk VALUES (35); +DELETE FROM fkpart9.pk WHERE a=35; +SELECT fk.fk_a, pk.a +FROM fkpart9.fk +LEFT JOIN fkpart9.pk ON fk.fk_a = pk.a +WHERE fk.fk_a=35; + fk_a | a +------+--- +(0 rows) + +DROP SCHEMA fkpart9 CASCADE; +NOTICE: drop cascades to 2 other objects +DETAIL: drop cascades to table fkpart9.pk +drop cascades to table fkpart9.fk diff --git a/src/test/regress/sql/foreign_key.sql b/src/test/regress/sql/foreign_key.sql index 6f94bf88851..b03a6670a21 100644 --- a/src/test/regress/sql/foreign_key.sql +++ b/src/test/regress/sql/foreign_key.sql @@ -1722,3 +1722,21 @@ INSERT INTO fkpart8.tbl2 VALUES(1); ALTER TABLE fkpart8.tbl2 DROP CONSTRAINT tbl2_f1_fkey; COMMIT; DROP SCHEMA fkpart8 CASCADE; + +-- ensure FK referencing a multi-level partitioned table are +-- enforce reference to sub-children. +CREATE SCHEMA fkpart9 + CREATE TABLE pk (a INT PRIMARY KEY) PARTITION BY RANGE (a) + CREATE TABLE fk ( + fk_a INT REFERENCES pk(a) ON DELETE CASCADE + ) + CREATE TABLE pk1 PARTITION OF pk FOR VALUES FROM (30) TO (50) PARTITION BY RANGE (a) + CREATE TABLE pk11 PARTITION OF pk1 FOR VALUES FROM (30) TO (40); +INSERT INTO fkpart9.pk VALUES (35); +INSERT INTO fkpart9.fk VALUES (35); +DELETE FROM fkpart9.pk WHERE a=35; +SELECT fk.fk_a, pk.a +FROM fkpart9.fk +LEFT JOIN fkpart9.pk ON fk.fk_a = pk.a +WHERE fk.fk_a=35; +DROP SCHEMA fkpart9 CASCADE; -- 2.39.5