On 2024-Jul-26, Tender Wang wrote: > Junwang Zhao <zhjw...@gmail.com> 于2024年7月26日周五 14:57写道： > > > There is a bug report[0] Tender comments might be the same issue as > > this one, but I tried Alvaro's and mine patch, neither could solve > > that problem, I did not tried Tender's earlier patch thought. I post > > the test script below in case you are interested. > > My earlier patch should handle Alexander reported case. But I did not > do more test. I'm not sure that wether or not has dismatching between > pg_constraint and pg_trigger. > > I aggred with Alvaro said that "requires a much more invasive > solution".
Here's the patch which, as far as I can tell, fixes all the reported problems (other than the one in bug 18541, for which I proposed an unrelated fix in that thread[1]). If you can double-check, I would very much appreciate that. Also, I think the test cases the patch adds reflect the provided examples sufficiently, but if we're still failing to cover some, please let me know. As I understand, this fix needs to be applied all the way back to 12, because the overall functionality is that old. However, in branches 14 and back, the patch doesn't apply cleanly, because of the changes we made in commit f4566345cf40 :-( I'm tempted to fix it in branches 15, 16, 17, master now and potentially backpatch later, to avoid dragging things along further. It's taken long enough already. [1] https://postgr.es/m/202408072222.icgykv5yrql5@alvherre.pgsql -- Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/ "World domination is proceeding according to plan" (Andrew Morton)
>From 4937a327afd51cddfdad7e01f3ccd9493b893bab Mon Sep 17 00:00:00 2001 From: Alvaro Herrera <alvhe...@alvh.no-ip.org> Date: Tue, 6 Aug 2024 16:59:04 -0400 Subject: [PATCH] Rework foreign key mangling during ATTACH/DETACH ... when the reference table is partitioned. It turns out that the catalog representation we chose for foreign keys connecting partitioned tables is suboptimal, particularly in the sense that a standalone table has a different way to represent it when referencing a partitioned table, than when the same table becomes a partition (and vice versa). This difference means we need to spawn additional catalog rows on detach, and remove them on attach. As a very obvious symptom, we were missing action triggers, which means that you could update/delete rows from the referenced partitioned table that still had referencing rows at the other side, and fail to throw the required errors. This means existing FKs might have rows that break relational integrity. Another possible problem is that trying to reattach a table that had been detached would fail indicating that internal triggers cannot be found, which from the user's point of view is nonsensical. We might want to rethink the representation in the future to avoid this messiness, but the code now seems to do what's required to make the constraints operate correctly. co-author : Tender Wang <tndrw...@gmail.com> Reported-by: Guillaume Lelarge <guilla...@lelarge.info> Reported-by: Jehan-Guillaume de Rorthais <j...@dalibo.com> Reported-by: Thomas Baehler (SBB CFF FFS) <thomas.baehl...@sbb.ch> Discussion: https://postgr.es/m/20230420144344.40744130@karst Discussion: https://postgr.es/m/20230705233028.2f554f73@karst Discussion: https://postgr.es/m/gvap278mb02787e7134fd691861635a8bc9...@gvap278mb0278.chep278.prod.outlook.com Discussion: https://postgr.es/m/18541-628a61bc267cd...@postgresql.org verify: 20230707175859.17c91538@karst --- src/backend/commands/tablecmds.c | 315 +++++++++++++++++++--- src/test/regress/expected/foreign_key.out | 59 ++++ src/test/regress/sql/foreign_key.sql | 30 +++ 3 files changed, 373 insertions(+), 31 deletions(-) diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index 1f94f4fdbb..bb40541ba1 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -10641,7 +10641,7 @@ CloneFkReferenced(Relation parentRel, Relation partitionRel) * 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 * AccessShareLock suffices (assumes that dropping the constraint - * acquires AEL). + * acquires AccessExclusiveLock). */ fkRel = table_open(constrForm->conrelid, AccessShareLock); @@ -11147,6 +11147,83 @@ tryAttachPartitionForeignKey(ForeignKeyCacheInfo *fk, TriggerSetParentTrigger(trigrel, updateTriggerOid, parentUpdTrigger, partRelid); + /* + * If the referenced table is partitioned, then the partition we're + * attaching now has extra pg_constraint rows and action triggers that are + * no longer needed. Remove those. + */ + if (get_rel_relkind(fk->confrelid) == RELKIND_PARTITIONED_TABLE) + { + Relation pg_constraint = table_open(ConstraintRelationId, RowShareLock); + ObjectAddresses *objs; + HeapTuple consttup; + + ScanKeyInit(&key, + Anum_pg_constraint_conrelid, + BTEqualStrategyNumber, F_OIDEQ, + ObjectIdGetDatum(fk->conrelid)); + + scan = systable_beginscan(pg_constraint, + ConstraintRelidTypidNameIndexId, + true, NULL, 1, &key); + objs = new_object_addresses(); + while ((consttup = systable_getnext(scan)) != NULL) + { + Form_pg_constraint conform = (Form_pg_constraint) GETSTRUCT(consttup); + + if (conform->conparentid != fk->conoid) + continue; + else + { + ObjectAddress addr; + int n; + SysScanDesc scan2; + ScanKeyData key2; + + ObjectAddressSet(addr, ConstraintRelationId, conform->oid); + add_exact_object_address(&addr, objs); + + /* + * First we must delete the dependency records that bind + * the constraint records together. + */ + n = deleteDependencyRecordsForSpecific(ConstraintRelationId, + conform->oid, + DEPENDENCY_INTERNAL, + ConstraintRelationId, + fk->conoid); + if (n != 1) + elog(WARNING, "oops: found %d instead of 1 deps from %u to %u", + n, conform->oid, fk->conoid); + + /* + * Now search for the triggers for this constraint and set + * them up for deletion too + */ + ScanKeyInit(&key2, + Anum_pg_trigger_tgconstraint, + BTEqualStrategyNumber, F_OIDEQ, + ObjectIdGetDatum(conform->oid)); + scan2 = systable_beginscan(trigrel, TriggerConstraintIndexId, + true, NULL, 1, &key2); + while ((trigtup = systable_getnext(scan2)) != NULL) + { + ObjectAddressSet(addr, TriggerRelationId, + ((Form_pg_trigger) GETSTRUCT(trigtup))->oid); + add_exact_object_address(&addr, objs); + } + systable_endscan(scan2); + } + } + /* make the dependency deletions visible */ + CommandCounterIncrement(); + performMultipleDeletions(objs, DROP_RESTRICT, + PERFORM_DELETION_INTERNAL); + systable_endscan(scan); + + table_close(pg_constraint, RowShareLock); + } + CommandCounterIncrement(); return true; } @@ -19182,8 +19259,10 @@ DetachPartitionFinalize(Relation rel, Relation partRel, bool concurrent, DropClonedTriggersFromPartition(RelationGetRelid(partRel)); /* - * Detach any foreign keys that are inherited. This includes creating - * additional action triggers. + * Detach any foreign keys that are inherited. This requires marking some + * constraints and triggers as no longer having parents, as well as + * creating additional constraint entries and triggers, depending on the + * shape of each FK. */ fks = copyObject(RelationGetFKeyList(partRel)); if (fks != NIL) @@ -19192,10 +19271,15 @@ DetachPartitionFinalize(Relation rel, Relation partRel, bool concurrent, { ForeignKeyCacheInfo *fk = lfirst(cell); HeapTuple contup; + HeapTuple parentConTup; Form_pg_constraint conform; + Form_pg_constraint parentConForm; Constraint *fkconstraint; - Oid insertTriggerOid, - updateTriggerOid; + Oid parentConstrOid; + Oid insertCheckTrigOid, + updateCheckTrigOid, + updateActionTrigOid, + deleteActionTrigOid; contup = SearchSysCache1(CONSTROID, ObjectIdGetDatum(fk->conoid)); if (!HeapTupleIsValid(contup)) @@ -19210,52 +19294,221 @@ DetachPartitionFinalize(Relation rel, Relation partRel, bool concurrent, continue; } - /* unset conparentid and adjust conislocal, coninhcount, etc. */ - ConstraintSetParentConstraint(fk->conoid, InvalidOid, InvalidOid); - /* - * Also, look up the partition's "check" triggers corresponding to the - * constraint being detached and detach them from the parent triggers. + * To create a FK constraint in the to-be-detached partition + * equivalent to what exists in the parent table, what to do depends + * on whether the referenced table is partitioned or not. If the + * referenced table isn't partitioned, then just reparenting the + * pg_constraint row and pg_trigger rows for the existing constraint, + * and creating new action triggers, is sufficient. + * + * If the referenced table is partitioned, then we must create + * additional pg_constraint rows -- one for each partition -- and the + * necessary action triggers for each constraint. */ - GetForeignKeyCheckTriggers(trigrel, - fk->conoid, fk->confrelid, fk->conrelid, - &insertTriggerOid, &updateTriggerOid); - Assert(OidIsValid(insertTriggerOid)); - TriggerSetParentTrigger(trigrel, insertTriggerOid, InvalidOid, - RelationGetRelid(partRel)); - Assert(OidIsValid(updateTriggerOid)); - TriggerSetParentTrigger(trigrel, updateTriggerOid, InvalidOid, - RelationGetRelid(partRel)); + parentConstrOid = conform->conparentid; - /* - * Make the action triggers on the referenced relation. When this was - * a partition the action triggers pointed to the parent rel (they - * still do), but now we need separate ones of our own. - */ + Assert(OidIsValid(conform->conparentid)); + parentConTup = SearchSysCache1(CONSTROID, + ObjectIdGetDatum(parentConstrOid)); + if (!HeapTupleIsValid(parentConTup)) + elog(ERROR, "cache lookup failed for constraint %u", + conform->conparentid); + parentConForm = (Form_pg_constraint) GETSTRUCT(parentConTup); + + /* Create a synthetic node we'll use throughout */ fkconstraint = makeNode(Constraint); fkconstraint->contype = CONSTRAINT_FOREIGN; fkconstraint->conname = pstrdup(NameStr(conform->conname)); fkconstraint->deferrable = conform->condeferrable; fkconstraint->initdeferred = conform->condeferred; - fkconstraint->location = -1; - fkconstraint->pktable = NULL; - fkconstraint->fk_attrs = NIL; - fkconstraint->pk_attrs = NIL; + fkconstraint->skip_validation = true; + fkconstraint->initially_valid = true; + /* a few irrelevant fields omitted here */ + fkconstraint->pktable = NULL; /* not needed */ + fkconstraint->fk_attrs = NIL; /* not needed */ + fkconstraint->pk_attrs = NIL; /* not needed */ fkconstraint->fk_matchtype = conform->confmatchtype; fkconstraint->fk_upd_action = conform->confupdtype; fkconstraint->fk_del_action = conform->confdeltype; fkconstraint->fk_del_set_cols = NIL; fkconstraint->old_conpfeqop = NIL; fkconstraint->old_pktable_oid = InvalidOid; - fkconstraint->skip_validation = false; - fkconstraint->initially_valid = true; + fkconstraint->location = -1; + /* + * The constraint on this table must be marked no longer a child of + * the parent's constraint, as do its check triggers. + */ + ConstraintSetParentConstraint(fk->conoid, InvalidOid, InvalidOid); + + GetForeignKeyCheckTriggers(trigrel, + fk->conoid, fk->confrelid, fk->conrelid, + &insertCheckTrigOid, &updateCheckTrigOid); + Assert(OidIsValid(insertCheckTrigOid)); + TriggerSetParentTrigger(trigrel, insertCheckTrigOid, InvalidOid, + RelationGetRelid(partRel)); + Assert(OidIsValid(updateCheckTrigOid)); + TriggerSetParentTrigger(trigrel, updateCheckTrigOid, InvalidOid, + RelationGetRelid(partRel)); + + /* + * Now create the action triggers in this table that point to the + * referenced table. + */ createForeignKeyActionTriggers(partRel, conform->confrelid, fkconstraint, fk->conoid, conform->conindid, InvalidOid, InvalidOid, - NULL, NULL); + &deleteActionTrigOid, &updateActionTrigOid); + /* + * If the referenced side is partitioned (which we know because our + * parent's constraint points to a different relation than ours) then + * we must, in addition to the above, create pg_constraint rows that + * point to each partition, each with its own action triggers. + */ + if (parentConForm->conrelid != conform->conrelid) + { + List *children; + ListCell *child; + Relation refdRel; + Oid indexOid; + int numfks; + AttrNumber conkey[INDEX_MAX_KEYS]; + AttrNumber confkey[INDEX_MAX_KEYS]; + Oid conpfeqop[INDEX_MAX_KEYS]; + Oid conppeqop[INDEX_MAX_KEYS]; + Oid conffeqop[INDEX_MAX_KEYS]; + int numfkdelsetcols; + AttrNumber confdelsetcols[INDEX_MAX_KEYS]; + List *colnames = NIL; + char *fkaddition; + + DeconstructFkConstraintRow(contup, + &numfks, + conkey, + confkey, + conpfeqop, + conppeqop, + conffeqop, + &numfkdelsetcols, + confdelsetcols); + + /* + * Choose the "name2" part to give to ChooseConstraintName. This + * is common to all constraints we create below. + */ + for (int i = 0; i < numfks; i++) + { + Form_pg_attribute att; + + att = TupleDescAttr(RelationGetDescr(partRel), + conkey[i] - 1); + colnames = lappend(colnames, makeString(NameStr(att->attname))); + } + fkaddition = ChooseForeignKeyConstraintNameAddition(colnames); + + /* + * We're not expanding nor shrinking key space, so AccessShareLock + * is sufficient here given that dropping a constraint requires + * AccessExclusiveLock. + */ + refdRel = table_open(fk->confrelid, AccessShareLock); + + /* + * When the referenced table is partitioned, we need new + * pg_constraint entries that point from our partition to each + * partition of the other table, and also the action triggers on + * each. + */ + children = find_all_inheritors(fk->confrelid, NoLock, NULL); + foreach(child, children) + { + Oid childoid = lfirst_oid(child); + Relation fchild; + AttrNumber mapped_confkey[INDEX_MAX_KEYS]; + AttrMap *attmap; + char *conname; + Oid constrOid; + ObjectAddress address, + referenced; + + /* A constraint for the top rel already exists, so skip it */ + if (childoid == fk->confrelid) + continue; + + fchild = table_open(childoid, AccessShareLock); + + /* + * Map the referenced attnums in case this partition has a + * different column layout. + */ + attmap = build_attrmap_by_name(RelationGetDescr(refdRel), + RelationGetDescr(fchild), + false); + for (int i = 0; i < numfks; i++) + mapped_confkey[i] = attmap->attnums[confkey[i] - 1]; + + conname = ChooseConstraintName(RelationGetRelationName(partRel), + fkaddition, "fkey", + RelationGetNamespace(partRel), NIL); + + /* Create the pg_constraint row to this specific partition */ + indexOid = index_get_partition(fchild, conform->conindid); + constrOid = + CreateConstraintEntry(conname, + RelationGetNamespace(partRel), + CONSTRAINT_FOREIGN, + fkconstraint->deferrable, + fkconstraint->initdeferred, + fkconstraint->initially_valid, + fk->conoid, + RelationGetRelid(partRel), + conkey, + numfks, + numfks, + InvalidOid, + indexOid, + childoid, + mapped_confkey, + conpfeqop, + conppeqop, + conffeqop, + numfks, + fkconstraint->fk_upd_action, + fkconstraint->fk_del_action, + confdelsetcols, + numfkdelsetcols, + fkconstraint->fk_matchtype, + NULL, + NULL, + NULL, + false, + 1, + false, + false); + + /* Give it a dependency on the "real" (parent) constraint */ + ObjectAddressSet(address, ConstraintRelationId, constrOid); + ObjectAddressSet(referenced, ConstraintRelationId, fk->conoid); + recordDependencyOn(&address, &referenced, DEPENDENCY_INTERNAL); + + /* And create the action triggers that go with it */ + createForeignKeyActionTriggers(partRel, childoid, + fkconstraint, + constrOid, indexOid, + deleteActionTrigOid, + updateActionTrigOid, + NULL, NULL); + + table_close(fchild, NoLock); + } + + table_close(refdRel, NoLock); + } + + ReleaseSysCache(parentConTup); ReleaseSysCache(contup); } list_free_deep(fks); diff --git a/src/test/regress/expected/foreign_key.out b/src/test/regress/expected/foreign_key.out index 46764bd9e3..2fc280d3bd 100644 --- a/src/test/regress/expected/foreign_key.out +++ b/src/test/regress/expected/foreign_key.out @@ -2917,3 +2917,62 @@ DETAIL: drop cascades to table fkpart11.pk drop cascades to table fkpart11.fk_parted drop cascades to table fkpart11.fk_another drop cascades to function fkpart11.print_row() +-- When a table is attached as partition to a partitioned table that has +-- a foreign key to another partitioned table, it acquires a clone of the +-- FK. Upon detach, this clone is not removed, but instead becomes an +-- independent FK. If it then attaches to the partitioned table again, +-- the FK from the parent "takes over" ownership of the independent FK rather +-- than creating a separate one. +CREATE SCHEMA fkpart12 + CREATE TABLE fk_p ( id bigint PRIMARY KEY ) PARTITION BY list (id) + CREATE TABLE fk_p_1 PARTITION OF fk_p FOR VALUES IN (1) + CREATE TABLE fk_p_2 PARTITION OF fk_p FOR VALUES IN (2) + CREATE TABLE fk_r_1 ( id bigint PRIMARY KEY, p_id bigint NOT NULL) + CREATE TABLE fk_r_2 ( id bigint PRIMARY KEY, p_id bigint NOT NULL) + CREATE TABLE fk_r ( id bigint PRIMARY KEY, p_id bigint NOT NULL, + FOREIGN KEY (p_id) REFERENCES fk_p (id) + ) PARTITION BY list (id); +SET search_path TO fkpart12; +ALTER TABLE fk_r ATTACH PARTITION fk_r_2 FOR VALUES IN (2); +ALTER TABLE fk_r ATTACH PARTITION fk_r_1 FOR VALUES IN (1); +\d fk_r_1 + Table "fkpart12.fk_r_1" + Column | Type | Collation | Nullable | Default +--------+--------+-----------+----------+--------- + id | bigint | | not null | + p_id | bigint | | not null | +Partition of: fk_r FOR VALUES IN ('1') +Indexes: + "fk_r_1_pkey" PRIMARY KEY, btree (id) +Foreign-key constraints: + TABLE "fk_r" CONSTRAINT "fk_r_p_id_fkey" FOREIGN KEY (p_id) REFERENCES fk_p(id) + +ALTER TABLE fk_r DETACH PARTITION fk_r_1; +\d fk_r_1 + Table "fkpart12.fk_r_1" + Column | Type | Collation | Nullable | Default +--------+--------+-----------+----------+--------- + id | bigint | | not null | + p_id | bigint | | not null | +Indexes: + "fk_r_1_pkey" PRIMARY KEY, btree (id) +Foreign-key constraints: + "fk_r_p_id_fkey" FOREIGN KEY (p_id) REFERENCES fk_p(id) + +ALTER TABLE fk_r ATTACH PARTITION fk_r_1 FOR VALUES IN (1); +\d fk_r_1 + Table "fkpart12.fk_r_1" + Column | Type | Collation | Nullable | Default +--------+--------+-----------+----------+--------- + id | bigint | | not null | + p_id | bigint | | not null | +Partition of: fk_r FOR VALUES IN ('1') +Indexes: + "fk_r_1_pkey" PRIMARY KEY, btree (id) +Foreign-key constraints: + TABLE "fk_r" CONSTRAINT "fk_r_p_id_fkey" FOREIGN KEY (p_id) REFERENCES fk_p(id) + +SET client_min_messages TO warning; +DROP SCHEMA fkpart12 CASCADE; +RESET client_min_messages; +RESET search_path; diff --git a/src/test/regress/sql/foreign_key.sql b/src/test/regress/sql/foreign_key.sql index f5e0938999..359eb6eefe 100644 --- a/src/test/regress/sql/foreign_key.sql +++ b/src/test/regress/sql/foreign_key.sql @@ -2069,3 +2069,33 @@ UPDATE fkpart11.pk SET a = 3 WHERE a = 4; UPDATE fkpart11.pk SET a = 1 WHERE a = 2; DROP SCHEMA fkpart11 CASCADE; + +-- When a table is attached as partition to a partitioned table that has +-- a foreign key to another partitioned table, it acquires a clone of the +-- FK. Upon detach, this clone is not removed, but instead becomes an +-- independent FK. If it then attaches to the partitioned table again, +-- the FK from the parent "takes over" ownership of the independent FK rather +-- than creating a separate one. +CREATE SCHEMA fkpart12 + CREATE TABLE fk_p ( id bigint PRIMARY KEY ) PARTITION BY list (id) + CREATE TABLE fk_p_1 PARTITION OF fk_p FOR VALUES IN (1) + CREATE TABLE fk_p_2 PARTITION OF fk_p FOR VALUES IN (2) + CREATE TABLE fk_r_1 ( id bigint PRIMARY KEY, p_id bigint NOT NULL) + CREATE TABLE fk_r_2 ( id bigint PRIMARY KEY, p_id bigint NOT NULL) + CREATE TABLE fk_r ( id bigint PRIMARY KEY, p_id bigint NOT NULL, + FOREIGN KEY (p_id) REFERENCES fk_p (id) + ) PARTITION BY list (id); +SET search_path TO fkpart12; +ALTER TABLE fk_r ATTACH PARTITION fk_r_2 FOR VALUES IN (2); + +ALTER TABLE fk_r ATTACH PARTITION fk_r_1 FOR VALUES IN (1); +\d fk_r_1 +ALTER TABLE fk_r DETACH PARTITION fk_r_1; +\d fk_r_1 +ALTER TABLE fk_r ATTACH PARTITION fk_r_1 FOR VALUES IN (1); +\d fk_r_1 + +SET client_min_messages TO warning; +DROP SCHEMA fkpart12 CASCADE; +RESET client_min_messages; +RESET search_path; -- 2.39.2
