From 1d09803a3d1f0240b205ea6c05669f6f70638deb Mon Sep 17 00:00:00 2001 From: Jehan-Guillaume de Rorthais Date: Wed, 25 Sep 2024 15:38:42 +0200 Subject: [PATCH v3-17 2/4] 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 after detach, which means that you could update/delete rows from the referenced partitioned table that still had referencing rows at the other side previously detached, and fail to throw the required errors. This means existing FKs might have rows that break relational integrity. To fix this bug, this patch rework addFkRecurseReferenced() so it can be called from DetachPartitionFinalize(), so we can keep this logic in a single function. The addFkRecurseReferenced() rework leads to create addFkConstraint(), but it seems cleaner as addFkRecurseReferenced() and addFkRecurseReferencing() have the same logic now. Some other code factoring using addFkConstraint() are added in next commits. 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: Alvaro Herrera co-author: Jehan-Guillaume de Rorthais co-author: Tender Wang Reported-by: Guillaume Lelarge Reported-by: Jehan-Guillaume de Rorthais Reported-by: Thomas Baehler (SBB CFF FFS) Discussion: https://postgr.es/m/20230420144344.40744130@karst Discussion: https://postgr.es/m/20230705233028.2f554f73@karst Discussion: https://postgr.es/m/GVAP278MB02787E7134FD691861635A8BC9032@GVAP278MB0278.CHEP278.PROD.OUTLOOK.COM Discussion: https://postgr.es/m/18541-628a61bc267cd2d3@postgresql.org --- src/backend/commands/tablecmds.c | 370 ++++++++++++++++++++++++------- 1 file changed, 289 insertions(+), 81 deletions(-) diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index 9432fd6587..83e0a0652e 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -504,7 +504,13 @@ static ObjectAddress ATAddForeignKeyConstraint(List **wqueue, AlteredTableInfo * Relation rel, Constraint *fkconstraint, bool recurse, bool recursing, LOCKMODE lockmode); -static ObjectAddress addFkRecurseReferenced(List **wqueue, Constraint *fkconstraint, +static ObjectAddress addFkConstraint(Constraint *fkconstraint, Relation rel, + Relation pkrel, Oid indexOid, Oid parentConstr, + int numfks, int16 *pkattnum, int16 *fkattnum, + Oid *pfeqoperators, Oid *ppeqoperators, + Oid *ffeqoperators, int numfkdelsetcols, + int16 *fkdelsetcols); +static void addFkRecurseReferenced(List **wqueue, Constraint *fkconstraint, Relation rel, Relation pkrel, Oid indexOid, Oid parentConstr, int numfks, int16 *pkattnum, int16 *fkattnum, Oid *pfeqoperators, Oid *ppeqoperators, Oid *ffeqoperators, @@ -644,9 +650,11 @@ static void DropClonedTriggersFromPartition(Oid partitionId); static ObjectAddress ATExecDetachPartition(List **wqueue, AlteredTableInfo *tab, Relation rel, RangeVar *name, bool concurrent); -static void DetachPartitionFinalize(Relation rel, Relation partRel, - bool concurrent, Oid defaultPartOid); -static ObjectAddress ATExecDetachPartitionFinalize(Relation rel, RangeVar *name); +static void DetachPartitionFinalize(List **wqueue, Relation rel, + Relation partRel, bool concurrent, + Oid defaultPartOid); +static ObjectAddress ATExecDetachPartitionFinalize(List **wqueue, Relation rel, + RangeVar *name); static ObjectAddress ATExecAttachPartitionIdx(List **wqueue, Relation parentIdx, RangeVar *name); static void validatePartitionedIndex(Relation partedIdx, Relation partedTbl); @@ -5512,7 +5520,7 @@ ATExecCmd(List **wqueue, AlteredTableInfo *tab, ((PartitionCmd *) cmd->def)->concurrent); break; case AT_DetachPartitionFinalize: - address = ATExecDetachPartitionFinalize(rel, ((PartitionCmd *) cmd->def)->name); + address = ATExecDetachPartitionFinalize(wqueue, rel, ((PartitionCmd *) cmd->def)->name); break; default: /* oops */ elog(ERROR, "unrecognized alter table type: %d", @@ -9935,9 +9943,21 @@ ATAddForeignKeyConstraint(List **wqueue, AlteredTableInfo *tab, Relation rel, * Create all the constraint and trigger objects, recursing to partitions * as necessary. First handle the referenced side. */ - address = addFkRecurseReferenced(wqueue, fkconstraint, rel, pkrel, + address = addFkConstraint(fkconstraint, rel, pkrel, + indexOid, + InvalidOid, /* no parent constraint */ + numfks, + pkattnum, + fkattnum, + pfeqoperators, + ppeqoperators, + ffeqoperators, + numfkdelsetcols, + fkdelsetcols); + + addFkRecurseReferenced(wqueue, fkconstraint, rel, pkrel, indexOid, - InvalidOid, /* no parent constraint */ + address.objectId, numfks, pkattnum, fkattnum, @@ -10009,46 +10029,31 @@ validateFkOnDeleteSetColumns(int numfks, const int16 *fkattnums, } /* - * addFkRecurseReferenced - * subroutine for ATAddForeignKeyConstraint; recurses on the referenced - * side of the constraint + * addFkConstraint + * Add a new Foreign Key constraint. * - * Create pg_constraint rows for the referenced side of the constraint, - * referencing the parent of the referencing side; also create action triggers - * on leaf partitions. If the table is partitioned, recurse to handle each - * partition. + * Creates a pg_constraint rows referencing a parent if given. * - * wqueue is the ALTER TABLE work queue; can be NULL when not running as part - * of an ALTER TABLE sequence. - * fkconstraint is the constraint being added. - * rel is the root referencing relation. - * pkrel is the referenced relation; might be a partition, if recursing. - * indexOid is the OID of the index (on pkrel) implementing this constraint. - * parentConstr is the OID of a parent constraint; InvalidOid if this is a - * top-level constraint. - * numfks is the number of columns in the foreign key - * pkattnum is the attnum array of referenced attributes. - * fkattnum is the attnum array of referencing attributes. - * numfkdelsetcols is the number of columns in the ON DELETE SET NULL/DEFAULT - * (...) clause - * fkdelsetcols is the attnum array of the columns in the ON DELETE SET - * NULL/DEFAULT clause - * pf/pp/ffeqoperators are OID array of operators between columns. - * old_check_ok signals that this constraint replaces an existing one that - * was already validated (thus this one doesn't need validation). - * parentDelTrigger and parentUpdTrigger, when being recursively called on - * a partition, are the OIDs of the parent action triggers for DELETE and - * UPDATE respectively. + * * fkconstraint is the constraint being added. + * * rel is the root referencing relation. + * * pkrel is the referenced relation; might be a partition, if recursing. + * * indexOid is the OID of the index (on pkrel) implementing this constraint. + * * parentConstr is the OID of a parent constraint; InvalidOid if this is a + * top-level constraint. + * * numfks is the number of columns in the foreign key. + * * pkattnum is the attnum array of referenced attributes. + * * fkattnum is the attnum array of referencing attributes. + * * pf/pp/ffeqoperators are OID array of operators between columns. + * * numfkdelsetcols is the number of columns in the ON DELETE SET NULL/DEFAULT + * (...) clause. + * * fkdelsetcols is the attnum array of the columns in the ON DELETE SET + * NULL/DEFAULT clause. */ static ObjectAddress -addFkRecurseReferenced(List **wqueue, Constraint *fkconstraint, Relation rel, - Relation pkrel, Oid indexOid, Oid parentConstr, - int numfks, - int16 *pkattnum, int16 *fkattnum, Oid *pfeqoperators, - Oid *ppeqoperators, Oid *ffeqoperators, - int numfkdelsetcols, int16 *fkdelsetcols, - bool old_check_ok, - Oid parentDelTrigger, Oid parentUpdTrigger) +addFkConstraint(Constraint *fkconstraint, Relation rel, Relation pkrel, + Oid indexOid, Oid parentConstr, int numfks, int16 *pkattnum, + int16 *fkattnum, Oid *pfeqoperators, Oid *ppeqoperators, + Oid *ffeqoperators, int numfkdelsetcols, int16 *fkdelsetcols) { ObjectAddress address; Oid constrOid; @@ -10056,8 +10061,6 @@ addFkRecurseReferenced(List **wqueue, Constraint *fkconstraint, Relation rel, bool conislocal; int coninhcount; bool connoinherit; - Oid deleteTriggerOid, - updateTriggerOid; /* * Verify relkind for each referenced partition. At the top level, this @@ -10155,12 +10158,61 @@ addFkRecurseReferenced(List **wqueue, Constraint *fkconstraint, Relation rel, /* make new constraint visible, in case we add more */ CommandCounterIncrement(); + return address; +} + +/* + * addFkRecurseReferenced + * subroutine for ATAddForeignKeyConstraint; recurses on the referenced + * side of the constraint + * + * If the referenced relation is a plain relation, create the necessary check + * triggers that implement the constraint. If the referenced relation is a + * partitioned table, then we create a pg_constraint row referencing the parent + * of the referencing side for it and recurse on this routine for each + * partition. + * + * wqueue is the ALTER TABLE work queue; can be NULL when not running as part + * of an ALTER TABLE sequence. + * fkconstraint is the constraint being added. + * rel is the root referencing relation. + * pkrel is the referenced relation; might be a partition, if recursing. + * indexOid is the OID of the index (on pkrel) implementing this constraint. + * parentConstr is the OID of a parent constraint; InvalidOid if this is a + * top-level constraint. + * numfks is the number of columns in the foreign key + * pkattnum is the attnum array of referenced attributes. + * fkattnum is the attnum array of referencing attributes. + * numfkdelsetcols is the number of columns in the ON DELETE SET NULL/DEFAULT + * (...) clause + * fkdelsetcols is the attnum array of the columns in the ON DELETE SET + * NULL/DEFAULT clause + * pf/pp/ffeqoperators are OID array of operators between columns. + * old_check_ok signals that this constraint replaces an existing one that + * was already validated (thus this one doesn't need validation). + * parentDelTrigger and parentUpdTrigger, when being recursively called on + * a partition, are the OIDs of the parent action triggers for DELETE and + * UPDATE respectively. + */ +static void +addFkRecurseReferenced(List **wqueue, Constraint *fkconstraint, Relation rel, + Relation pkrel, Oid indexOid, Oid parentConstr, + int numfks, + int16 *pkattnum, int16 *fkattnum, Oid *pfeqoperators, + Oid *ppeqoperators, Oid *ffeqoperators, + int numfkdelsetcols, int16 *fkdelsetcols, + bool old_check_ok, + Oid parentDelTrigger, Oid parentUpdTrigger) +{ + Oid deleteTriggerOid, + updateTriggerOid; + /* * Create the action triggers that enforce the constraint. */ createForeignKeyActionTriggers(rel, RelationGetRelid(pkrel), fkconstraint, - constrOid, indexOid, + parentConstr, indexOid, parentDelTrigger, parentUpdTrigger, &deleteTriggerOid, &updateTriggerOid); @@ -10179,6 +10231,7 @@ addFkRecurseReferenced(List **wqueue, Constraint *fkconstraint, Relation rel, AttrMap *map; AttrNumber *mapped_pkattnum; Oid partIndexId; + ObjectAddress address; partRel = table_open(pd->oids[i], ShareRowExclusiveLock); @@ -10203,8 +10256,14 @@ addFkRecurseReferenced(List **wqueue, Constraint *fkconstraint, Relation rel, if (!OidIsValid(partIndexId)) elog(ERROR, "index for %u not found in partition %s", indexOid, RelationGetRelationName(partRel)); + + address = addFkConstraint(fkconstraint, rel, partRel, + partIndexId, parentConstr, numfks, + mapped_pkattnum, fkattnum, + pfeqoperators, ppeqoperators, ffeqoperators, + numfkdelsetcols, fkdelsetcols); addFkRecurseReferenced(wqueue, fkconstraint, rel, partRel, - partIndexId, constrOid, numfks, + partIndexId, address.objectId, numfks, mapped_pkattnum, fkattnum, pfeqoperators, ppeqoperators, ffeqoperators, numfkdelsetcols, fkdelsetcols, @@ -10220,8 +10279,6 @@ addFkRecurseReferenced(List **wqueue, Constraint *fkconstraint, Relation rel, } } } - - return address; } /* @@ -10583,6 +10640,7 @@ CloneFkReferenced(Relation parentRel, Relation partitionRel) int numfkdelsetcols; AttrNumber confdelsetcols[INDEX_MAX_KEYS]; Constraint *fkconstraint; + ObjectAddress address; Oid deleteTriggerOid, updateTriggerOid; @@ -10616,7 +10674,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); @@ -10682,12 +10740,16 @@ CloneFkReferenced(Relation parentRel, Relation partitionRel) constrForm->confrelid, constrForm->conrelid, &deleteTriggerOid, &updateTriggerOid); + address = addFkConstraint(fkconstraint, fkRel, partitionRel, partIndexId, + constrOid, numfks, mapped_confkey, conkey, + conpfeqop, conppeqop, conffeqop, + numfkdelsetcols, confdelsetcols); addFkRecurseReferenced(NULL, fkconstraint, fkRel, partitionRel, partIndexId, - constrOid, + address.objectId, numfks, mapped_confkey, conkey, @@ -11139,6 +11201,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; } @@ -19132,7 +19271,7 @@ ATExecDetachPartition(List **wqueue, AlteredTableInfo *tab, Relation rel, } /* Do the final part of detaching */ - DetachPartitionFinalize(rel, partRel, concurrent, defaultPartOid); + DetachPartitionFinalize(wqueue, rel, partRel, concurrent, defaultPartOid); ObjectAddressSet(address, RelationRelationId, RelationGetRelid(partRel)); @@ -19149,8 +19288,8 @@ ATExecDetachPartition(List **wqueue, AlteredTableInfo *tab, Relation rel, * transaction of the concurrent algorithm fails (crash or abort). */ static void -DetachPartitionFinalize(Relation rel, Relation partRel, bool concurrent, - Oid defaultPartOid) +DetachPartitionFinalize(List **wqueue, Relation rel, Relation partRel, + bool concurrent, Oid defaultPartOid) { Relation classRel; List *fks; @@ -19186,8 +19325,11 @@ 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 parentConstrOid; Oid insertTriggerOid, updateTriggerOid; @@ -19204,7 +19346,20 @@ DetachPartitionFinalize(Relation rel, Relation partRel, bool concurrent, continue; } - /* unset conparentid and adjust conislocal, coninhcount, etc. */ + parentConstrOid = conform->conparentid; + + 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); + + /* + * 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); /* @@ -19222,35 +19377,88 @@ DetachPartitionFinalize(Relation rel, Relation partRel, bool concurrent, RelationGetRelid(partRel)); /* - * 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. + * 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. */ - 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->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; + if (parentConForm->conrelid != conform->conrelid) + { + int numfks; + AttrNumber conkey[INDEX_MAX_KEYS]; + AttrMap *attmap; + 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]; + Relation refdRel; + + DeconstructFkConstraintRow(contup, + &numfks, + conkey, + confkey, + conpfeqop, + conppeqop, + conffeqop, + &numfkdelsetcols, + confdelsetcols); + + /* 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->skip_validation = true; + fkconstraint->initially_valid = true; + /* a few irrelevant fields omitted here */ + fkconstraint->pktable = NULL; + fkconstraint->fk_attrs = NIL; + fkconstraint->pk_attrs = NIL; + 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->location = -1; + + attmap = build_attrmap_by_name(RelationGetDescr(partRel), + RelationGetDescr(rel), + false); + for (int i = 0; i < numfks; i++) + { + Form_pg_attribute att; - createForeignKeyActionTriggers(partRel, conform->confrelid, - fkconstraint, fk->conoid, + att = TupleDescAttr(RelationGetDescr(partRel), + attmap->attnums[conkey[i] - 1] - 1); + fkconstraint->fk_attrs = lappend(fkconstraint->fk_attrs, + makeString(NameStr(att->attname))); + } + + refdRel = table_open(fk->confrelid, AccessShareLock); + + addFkRecurseReferenced(wqueue, fkconstraint, partRel, + refdRel, conform->conindid, - InvalidOid, InvalidOid, - NULL, NULL); + fk->conoid, + numfks, + confkey, + conkey, + conpfeqop, + conppeqop, + conffeqop, + numfkdelsetcols, + confdelsetcols, + true, + InvalidOid, InvalidOid); + table_close(refdRel, AccessShareLock); + } ReleaseSysCache(contup); + ReleaseSysCache(parentConTup); } list_free_deep(fks); if (trigrel) @@ -19397,7 +19605,7 @@ DetachPartitionFinalize(Relation rel, Relation partRel, bool concurrent, * completion; this completes the detaching process. */ static ObjectAddress -ATExecDetachPartitionFinalize(Relation rel, RangeVar *name) +ATExecDetachPartitionFinalize(List **wqueue, Relation rel, RangeVar *name) { Relation partRel; ObjectAddress address; @@ -19415,7 +19623,7 @@ ATExecDetachPartitionFinalize(Relation rel, RangeVar *name) */ WaitForOlderSnapshots(snap->xmin, false); - DetachPartitionFinalize(rel, partRel, true, InvalidOid); + DetachPartitionFinalize(wqueue, rel, partRel, true, InvalidOid); ObjectAddressSet(address, RelationRelationId, RelationGetRelid(partRel)); -- 2.46.0