From bf6fe296e61de92729a445ba68220a26b8282d32 Mon Sep 17 00:00:00 2001 From: Alvaro Herrera Date: Wed, 5 Jan 2022 19:00:13 -0300 Subject: [PATCH 1/2] Create foreign key triggers in partitioned tables too While user-defined triggers defined on a partitioned table have a catalog definition for both it and its partitions, internal triggers used by foreign keys defined on partitioned tables only have a catalog definition for its partitions. This commit fixes that so that partitioned tables get the foreign key triggers too, just like user-defined triggers. Moreover, like user-defined triggers, partitions' internal triggers will now also have their tgparentid set appropriately. This is to allow subsequent commit(s) to make the foreign key related events to be fired in some cases using the parent table triggers instead of those of partitions'. This also changes what tgisinternal means in some cases. Currently, it means either that the trigger is an internal implementation object of a foreign key constraint, or a "child" trigger on a partition cloned from the trigger on the parent. This commit changes it to only mean the former to avoid confusion. As for the latter, it can be told by tgparentid being nonzero, which is now true both for user- defined and foreign key's internal triggers. Author: Amit Langote Reviewed-by: Masahiko Sawada Reviewed-by: Arne Roland Discussion: https://postgr.es/m/CA+HiwqG7LQSK+n8Bki8tWv7piHD=PnZro2y6ysU2-28JS6cfgQ@mail.gmail.com --- src/backend/commands/tablecmds.c | 414 +++++++++++++++++++++---- src/backend/commands/trigger.c | 84 ++++- src/bin/pg_dump/pg_dump.c | 24 +- src/bin/pg_dump/pg_dump.h | 2 +- src/bin/psql/describe.c | 16 +- src/include/commands/trigger.h | 4 + src/test/regress/expected/triggers.out | 2 +- 7 files changed, 463 insertions(+), 83 deletions(-) diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index 6b82d32ba1..0539642817 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -453,12 +453,14 @@ static ObjectAddress addFkRecurseReferenced(List **wqueue, Constraint *fkconstra Relation rel, Relation pkrel, Oid indexOid, Oid parentConstr, int numfks, int16 *pkattnum, int16 *fkattnum, Oid *pfeqoperators, Oid *ppeqoperators, Oid *ffeqoperators, - bool old_check_ok); + bool old_check_ok, + Oid parentDelTrigger, Oid parentUpdTrigger); static void addFkRecurseReferencing(List **wqueue, Constraint *fkconstraint, Relation rel, Relation pkrel, Oid indexOid, Oid parentConstr, int numfks, int16 *pkattnum, int16 *fkattnum, Oid *pfeqoperators, Oid *ppeqoperators, Oid *ffeqoperators, - bool old_check_ok, LOCKMODE lockmode); + bool old_check_ok, LOCKMODE lockmode, + Oid parentInsTrigger, Oid parentUpdTrigger); static void CloneForeignKeyConstraints(List **wqueue, Relation parentRel, Relation partitionRel); static void CloneFkReferenced(Relation parentRel, Relation partitionRel); @@ -466,15 +468,30 @@ static void CloneFkReferencing(List **wqueue, Relation parentRel, Relation partRel); static void createForeignKeyCheckTriggers(Oid myRelOid, Oid refRelOid, Constraint *fkconstraint, Oid constraintOid, - Oid indexOid); + Oid indexOid, + Oid parentInsTrigger, Oid parentUpdTrigger, + Oid *insertTrigOid, Oid *updateTrigOid); static void createForeignKeyActionTriggers(Relation rel, Oid refRelOid, Constraint *fkconstraint, Oid constraintOid, - Oid indexOid); + Oid indexOid, + Oid parentDelTrigger, Oid parentUpdTrigger, + Oid *deleteTrigOid, Oid *updateTrigOid); static bool tryAttachPartitionForeignKey(ForeignKeyCacheInfo *fk, Oid partRelid, Oid parentConstrOid, int numfks, AttrNumber *mapped_conkey, AttrNumber *confkey, - Oid *conpfeqop); + Oid *conpfeqop, + Oid parentInsTrigger, + Oid parentUpdTrigger, + Relation trigrel); +static void GetForeignKeyActionTriggers(Relation trigrel, + Oid conoid, Oid confrelid, Oid conrelid, + Oid *deleteTriggerOid, + Oid *updateTriggerOid); +static void GetForeignKeyCheckTriggers(Relation trigrel, + Oid conoid, Oid confrelid, Oid conrelid, + Oid *insertTriggerOid, + Oid *updateTriggerOid); static void ATExecDropConstraint(Relation rel, const char *constrName, DropBehavior behavior, bool recurse, bool recursing, @@ -8833,7 +8850,8 @@ ATAddForeignKeyConstraint(List **wqueue, AlteredTableInfo *tab, Relation rel, pfeqoperators, ppeqoperators, ffeqoperators, - old_check_ok); + old_check_ok, + InvalidOid, InvalidOid); /* Now handle the referencing side. */ addFkRecurseReferencing(wqueue, fkconstraint, rel, pkrel, @@ -8846,7 +8864,8 @@ ATAddForeignKeyConstraint(List **wqueue, AlteredTableInfo *tab, Relation rel, ppeqoperators, ffeqoperators, old_check_ok, - lockmode); + lockmode, + InvalidOid, InvalidOid); /* * Done. Close pk table, but keep lock until we've committed. @@ -8880,13 +8899,18 @@ ATAddForeignKeyConstraint(List **wqueue, AlteredTableInfo *tab, Relation rel, * 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 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, bool old_check_ok) + Oid *ppeqoperators, Oid *ffeqoperators, + bool old_check_ok, + Oid parentDelTrigger, Oid parentUpdTrigger) { ObjectAddress address; Oid constrOid; @@ -8894,6 +8918,8 @@ 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 @@ -8990,15 +9016,13 @@ addFkRecurseReferenced(List **wqueue, Constraint *fkconstraint, Relation rel, CommandCounterIncrement(); /* - * If the referenced table is a plain relation, create the action triggers - * that enforce the constraint. + * Create the action triggers that enforce the constraint. */ - if (pkrel->rd_rel->relkind == RELKIND_RELATION) - { - createForeignKeyActionTriggers(rel, RelationGetRelid(pkrel), - fkconstraint, - constrOid, indexOid); - } + createForeignKeyActionTriggers(rel, RelationGetRelid(pkrel), + fkconstraint, + constrOid, indexOid, + parentDelTrigger, parentUpdTrigger, + &deleteTriggerOid, &updateTriggerOid); /* * If the referenced table is partitioned, recurse on ourselves to handle @@ -9042,7 +9066,8 @@ addFkRecurseReferenced(List **wqueue, Constraint *fkconstraint, Relation rel, partIndexId, constrOid, numfks, mapped_pkattnum, fkattnum, pfeqoperators, ppeqoperators, ffeqoperators, - old_check_ok); + old_check_ok, + deleteTriggerOid, updateTriggerOid); /* Done -- clean up (but keep the lock) */ table_close(partRel, NoLock); @@ -9085,14 +9110,21 @@ addFkRecurseReferenced(List **wqueue, Constraint *fkconstraint, Relation rel, * old_check_ok signals that this constraint replaces an existing one that * was already validated (thus this one doesn't need validation). * lockmode is the lockmode to acquire on partitions when recursing. + * parentInsTrigger and parentUpdTrigger, when being recursively called on + * a partition, are the OIDs of the parent check triggers for INSERT and + * UPDATE respectively. */ static void addFkRecurseReferencing(List **wqueue, Constraint *fkconstraint, Relation rel, Relation pkrel, Oid indexOid, Oid parentConstr, int numfks, int16 *pkattnum, int16 *fkattnum, Oid *pfeqoperators, Oid *ppeqoperators, Oid *ffeqoperators, - bool old_check_ok, LOCKMODE lockmode) + bool old_check_ok, LOCKMODE lockmode, + Oid parentInsTrigger, Oid parentUpdTrigger) { + Oid insertTriggerOid, + updateTriggerOid; + AssertArg(OidIsValid(parentConstr)); if (rel->rd_rel->relkind == RELKIND_FOREIGN_TABLE) @@ -9101,19 +9133,21 @@ addFkRecurseReferencing(List **wqueue, Constraint *fkconstraint, Relation rel, errmsg("foreign key constraints are not supported on foreign tables"))); /* - * If the referencing relation is a plain table, add the check triggers to - * it and, if necessary, schedule it to be checked in Phase 3. + * Add the check triggers to it and, if necessary, schedule it to be + * checked in Phase 3. * * If the relation is partitioned, drill down to do it to its partitions. */ + createForeignKeyCheckTriggers(RelationGetRelid(rel), + RelationGetRelid(pkrel), + fkconstraint, + parentConstr, + indexOid, + parentInsTrigger, parentUpdTrigger, + &insertTriggerOid, &updateTriggerOid); + if (rel->rd_rel->relkind == RELKIND_RELATION) { - createForeignKeyCheckTriggers(RelationGetRelid(rel), - RelationGetRelid(pkrel), - fkconstraint, - parentConstr, - indexOid); - /* * Tell Phase 3 to check that the constraint is satisfied by existing * rows. We can skip this during table creation, when requested @@ -9142,6 +9176,15 @@ addFkRecurseReferencing(List **wqueue, Constraint *fkconstraint, Relation rel, else if (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE) { PartitionDesc pd = RelationGetPartitionDesc(rel); + Relation trigrel; + + /* + * Triggers of the foreign keys will be manipulated a bunch of times + * in the loop below. To avoid repeatedly opening/closing the trigger + * catalog relation, we open it here and pass it to the subroutines + * called below. + */ + trigrel = table_open(TriggerRelationId, RowExclusiveLock); /* * Recurse to take appropriate action on each partition; either we @@ -9183,7 +9226,10 @@ addFkRecurseReferencing(List **wqueue, Constraint *fkconstraint, Relation rel, numfks, mapped_fkattnum, pkattnum, - pfeqoperators)) + pfeqoperators, + insertTriggerOid, + updateTriggerOid, + trigrel)) { attached = true; break; @@ -9262,10 +9308,14 @@ addFkRecurseReferencing(List **wqueue, Constraint *fkconstraint, Relation rel, ppeqoperators, ffeqoperators, old_check_ok, - lockmode); + lockmode, + insertTriggerOid, + updateTriggerOid); table_close(partition, NoLock); } + + table_close(trigrel, RowExclusiveLock); } } @@ -9321,6 +9371,7 @@ CloneFkReferenced(Relation parentRel, Relation partitionRel) ScanKeyData key[2]; HeapTuple tuple; List *clone = NIL; + Relation trigrel; /* * Search for any constraints where this partition's parent is in the @@ -9350,6 +9401,14 @@ CloneFkReferenced(Relation parentRel, Relation partitionRel) systable_endscan(scan); table_close(pg_constraint, RowShareLock); + /* + * Triggers of the foreign keys will be manipulated a bunch of times in + * the loop below. To avoid repeatedly opening/closing the trigger + * catalog relation, we open it here and pass it to the subroutines called + * below. + */ + trigrel = table_open(TriggerRelationId, RowExclusiveLock); + attmap = build_attrmap_by_name(RelationGetDescr(partitionRel), RelationGetDescr(parentRel)); foreach(cell, clone) @@ -9367,6 +9426,8 @@ CloneFkReferenced(Relation parentRel, Relation partitionRel) Oid conppeqop[INDEX_MAX_KEYS]; Oid conffeqop[INDEX_MAX_KEYS]; Constraint *fkconstraint; + Oid deleteTriggerOid, + updateTriggerOid; tuple = SearchSysCache1(CONSTROID, constrOid); if (!HeapTupleIsValid(tuple)) @@ -9433,6 +9494,16 @@ CloneFkReferenced(Relation parentRel, Relation partitionRel) if (!OidIsValid(partIndexId)) elog(ERROR, "index for %u not found in partition %s", indexOid, RelationGetRelationName(partitionRel)); + + /* + * Get the "action" triggers belonging to the constraint to pass as + * parent OIDs for similar triggers that will be created on the + * partition in addFkRecurseReferenced(). + */ + GetForeignKeyActionTriggers(trigrel, constrOid, + constrForm->confrelid, constrForm->conrelid, + &deleteTriggerOid, &updateTriggerOid); + addFkRecurseReferenced(NULL, fkconstraint, fkRel, @@ -9445,11 +9516,15 @@ CloneFkReferenced(Relation parentRel, Relation partitionRel) conpfeqop, conppeqop, conffeqop, - true); + true, + deleteTriggerOid, + updateTriggerOid); table_close(fkRel, NoLock); ReleaseSysCache(tuple); } + + table_close(trigrel, RowExclusiveLock); } /* @@ -9472,6 +9547,7 @@ CloneFkReferencing(List **wqueue, Relation parentRel, Relation partRel) List *partFKs; List *clone = NIL; ListCell *cell; + Relation trigrel; /* obtain a list of constraints that we need to clone */ foreach(cell, RelationGetFKeyList(parentRel)) @@ -9493,6 +9569,14 @@ CloneFkReferencing(List **wqueue, Relation parentRel, Relation partRel) (errcode(ERRCODE_WRONG_OBJECT_TYPE), errmsg("foreign key constraints are not supported on foreign tables"))); + /* + * Triggers of the foreign keys will be manipulated a bunch of times in + * the loop below. To avoid repeatedly opening/closing the trigger + * catalog relation, we open it here and pass it to the subroutines called + * below. + */ + trigrel = table_open(TriggerRelationId, RowExclusiveLock); + /* * The constraint key may differ, if the columns in the partition are * different. This map is used to convert them. @@ -9522,6 +9606,8 @@ CloneFkReferencing(List **wqueue, Relation parentRel, Relation partRel) ObjectAddress address, referenced; ListCell *cell; + Oid insertTriggerOid, + updateTriggerOid; tuple = SearchSysCache1(CONSTROID, parentConstrOid); if (!HeapTupleIsValid(tuple)) @@ -9550,6 +9636,19 @@ CloneFkReferencing(List **wqueue, Relation parentRel, Relation partRel) for (int i = 0; i < numfks; i++) mapped_conkey[i] = attmap->attnums[conkey[i] - 1]; + /* + * Get the "check" triggers belonging to the constraint to pass as + * parent OIDs for similar triggers that will be created on the + * partition in addFkRecurseReferencing(). They are also passed to + * tryAttachPartitionForeignKey() below to simply assign as parents to + * the partition's existing "check" triggers, that is, if the + * corresponding constraints is deemed attachable to the parent + * constraint. + */ + GetForeignKeyCheckTriggers(trigrel, constrForm->oid, + constrForm->confrelid, constrForm->conrelid, + &insertTriggerOid, &updateTriggerOid); + /* * Before creating a new constraint, see whether any existing FKs are * fit for the purpose. If one is, attach the parent constraint to @@ -9568,7 +9667,10 @@ CloneFkReferencing(List **wqueue, Relation parentRel, Relation partRel) numfks, mapped_conkey, confkey, - conpfeqop)) + conpfeqop, + insertTriggerOid, + updateTriggerOid, + trigrel)) { attached = true; table_close(pkrel, NoLock); @@ -9667,9 +9769,13 @@ CloneFkReferencing(List **wqueue, Relation parentRel, Relation partRel) conppeqop, conffeqop, false, /* no old check exists */ - AccessExclusiveLock); + AccessExclusiveLock, + insertTriggerOid, + updateTriggerOid); table_close(pkrel, NoLock); } + + table_close(trigrel, RowExclusiveLock); } /* @@ -9690,16 +9796,20 @@ tryAttachPartitionForeignKey(ForeignKeyCacheInfo *fk, int numfks, AttrNumber *mapped_conkey, AttrNumber *confkey, - Oid *conpfeqop) + Oid *conpfeqop, + Oid parentInsTrigger, + Oid parentUpdTrigger, + Relation trigrel) { HeapTuple parentConstrTup; Form_pg_constraint parentConstr; HeapTuple partcontup; Form_pg_constraint partConstr; - Relation trigrel; ScanKeyData key; SysScanDesc scan; HeapTuple trigtup; + Oid insertTriggerOid, + updateTriggerOid; parentConstrTup = SearchSysCache1(CONSTROID, ObjectIdGetDatum(parentConstrOid)); @@ -9760,12 +9870,10 @@ tryAttachPartitionForeignKey(ForeignKeyCacheInfo *fk, * in the partition. We identify them because they have our constraint * OID, as well as being on the referenced rel. */ - trigrel = table_open(TriggerRelationId, RowExclusiveLock); ScanKeyInit(&key, Anum_pg_trigger_tgconstraint, BTEqualStrategyNumber, F_OIDEQ, ObjectIdGetDatum(fk->conoid)); - scan = systable_beginscan(trigrel, TriggerConstraintIndexId, true, NULL, 1, &key); while ((trigtup = systable_getnext(scan)) != NULL) @@ -9798,13 +9906,136 @@ tryAttachPartitionForeignKey(ForeignKeyCacheInfo *fk, } systable_endscan(scan); - table_close(trigrel, RowExclusiveLock); ConstraintSetParentConstraint(fk->conoid, parentConstrOid, partRelid); + + /* + * Like the constraint, attach partition's "check" triggers to the + * corresponding parent triggers. + */ + GetForeignKeyCheckTriggers(trigrel, + fk->conoid, fk->confrelid, fk->conrelid, + &insertTriggerOid, &updateTriggerOid); + Assert(OidIsValid(insertTriggerOid) && OidIsValid(parentInsTrigger)); + TriggerSetParentTrigger(trigrel, insertTriggerOid, parentInsTrigger, + partRelid); + Assert(OidIsValid(updateTriggerOid) && OidIsValid(parentUpdTrigger)); + TriggerSetParentTrigger(trigrel, updateTriggerOid, parentUpdTrigger, + partRelid); + CommandCounterIncrement(); return true; } +/* + * GetForeignKeyActionTriggers + * Returns delete and update "action" triggers of the given relation + * belonging to the given constraint + */ +static void +GetForeignKeyActionTriggers(Relation trigrel, + Oid conoid, Oid confrelid, Oid conrelid, + Oid *deleteTriggerOid, + Oid *updateTriggerOid) +{ + ScanKeyData key; + SysScanDesc scan; + HeapTuple trigtup; + + *deleteTriggerOid = *updateTriggerOid = InvalidOid; + ScanKeyInit(&key, + Anum_pg_trigger_tgconstraint, + BTEqualStrategyNumber, F_OIDEQ, + ObjectIdGetDatum(conoid)); + + scan = systable_beginscan(trigrel, TriggerConstraintIndexId, true, + NULL, 1, &key); + while ((trigtup = systable_getnext(scan)) != NULL) + { + Form_pg_trigger trgform = (Form_pg_trigger) GETSTRUCT(trigtup); + + if (trgform->tgconstrrelid != conrelid) + continue; + if (trgform->tgrelid != confrelid) + continue; + if (TRIGGER_FOR_DELETE(trgform->tgtype)) + { + Assert(*deleteTriggerOid == InvalidOid); + *deleteTriggerOid = trgform->oid; + } + else if (TRIGGER_FOR_UPDATE(trgform->tgtype)) + { + Assert(*updateTriggerOid == InvalidOid); + *updateTriggerOid = trgform->oid; + } + if (OidIsValid(*deleteTriggerOid) && OidIsValid(*updateTriggerOid)) + break; + } + + if (!OidIsValid(*deleteTriggerOid)) + elog(ERROR, "could not find ON DELETE action trigger of foreign key constraint %u", + conoid); + if (!OidIsValid(*updateTriggerOid)) + elog(ERROR, "could not find ON UPDATE action trigger of foreign key constraint %u", + conoid); + + systable_endscan(scan); +} + +/* + * GetForeignKeyCheckTriggers + * Returns insert and update "check" triggers of the given relation + * belonging to the given constraint + */ +static void +GetForeignKeyCheckTriggers(Relation trigrel, + Oid conoid, Oid confrelid, Oid conrelid, + Oid *insertTriggerOid, + Oid *updateTriggerOid) +{ + ScanKeyData key; + SysScanDesc scan; + HeapTuple trigtup; + + *insertTriggerOid = *updateTriggerOid = InvalidOid; + ScanKeyInit(&key, + Anum_pg_trigger_tgconstraint, + BTEqualStrategyNumber, F_OIDEQ, + ObjectIdGetDatum(conoid)); + + scan = systable_beginscan(trigrel, TriggerConstraintIndexId, true, + NULL, 1, &key); + while ((trigtup = systable_getnext(scan)) != NULL) + { + Form_pg_trigger trgform = (Form_pg_trigger) GETSTRUCT(trigtup); + + if (trgform->tgconstrrelid != confrelid) + continue; + if (trgform->tgrelid != conrelid) + continue; + if (TRIGGER_FOR_INSERT(trgform->tgtype)) + { + Assert(*insertTriggerOid == InvalidOid); + *insertTriggerOid = trgform->oid; + } + else if (TRIGGER_FOR_UPDATE(trgform->tgtype)) + { + Assert(*updateTriggerOid == InvalidOid); + *updateTriggerOid = trgform->oid; + } + if (OidIsValid(*insertTriggerOid) && OidIsValid(*updateTriggerOid)) + break; + } + + if (!OidIsValid(*insertTriggerOid)) + elog(ERROR, "could not find ON INSERT check triggers of foreign key constraint %u", + conoid); + if (!OidIsValid(*updateTriggerOid)) + elog(ERROR, "could not find ON UPDATE check triggers of foreign key constraint %u", + conoid); + + systable_endscan(scan); +} /* * ALTER TABLE ALTER CONSTRAINT @@ -10721,10 +10952,19 @@ validateForeignKeyConstraint(char *conname, ExecDropSingleTupleTableSlot(slot); } -static void +/* + * CreateFKCheckTrigger + * Creates the insert (on_insert=true) or update "check" trigger that + * implements a given foreign key + * + * Returns the OID of the so created trigger. + */ +static Oid CreateFKCheckTrigger(Oid myRelOid, Oid refRelOid, Constraint *fkconstraint, - Oid constraintOid, Oid indexOid, bool on_insert) + Oid constraintOid, Oid indexOid, Oid parentTrigOid, + bool on_insert) { + ObjectAddress trigAddress; CreateTrigStmt *fk_trigger; /* @@ -10763,23 +11003,32 @@ CreateFKCheckTrigger(Oid myRelOid, Oid refRelOid, Constraint *fkconstraint, fk_trigger->constrrel = NULL; fk_trigger->args = NIL; - (void) CreateTrigger(fk_trigger, NULL, myRelOid, refRelOid, constraintOid, - indexOid, InvalidOid, InvalidOid, NULL, true, false); + trigAddress = CreateTrigger(fk_trigger, NULL, myRelOid, refRelOid, + constraintOid, indexOid, InvalidOid, + parentTrigOid, NULL, true, false); /* Make changes-so-far visible */ CommandCounterIncrement(); + + return trigAddress.objectId; } /* * createForeignKeyActionTriggers * Create the referenced-side "action" triggers that implement a foreign * key. + * + * Returns the OIDs of the so created triggers in *deleteTrigOid and + * *updateTrigOid. */ static void createForeignKeyActionTriggers(Relation rel, Oid refRelOid, Constraint *fkconstraint, - Oid constraintOid, Oid indexOid) + Oid constraintOid, Oid indexOid, + Oid parentDelTrigger, Oid parentUpdTrigger, + Oid *deleteTrigOid, Oid *updateTrigOid) { CreateTrigStmt *fk_trigger; + ObjectAddress trigAddress; /* * Build and execute a CREATE CONSTRAINT TRIGGER statement for the ON @@ -10830,9 +11079,12 @@ createForeignKeyActionTriggers(Relation rel, Oid refRelOid, Constraint *fkconstr } fk_trigger->args = NIL; - (void) CreateTrigger(fk_trigger, NULL, refRelOid, RelationGetRelid(rel), - constraintOid, - indexOid, InvalidOid, InvalidOid, NULL, true, false); + trigAddress = CreateTrigger(fk_trigger, NULL, refRelOid, + RelationGetRelid(rel), + constraintOid, indexOid, InvalidOid, + parentDelTrigger, NULL, true, false); + if (deleteTrigOid) + *deleteTrigOid = trigAddress.objectId; /* Make changes-so-far visible */ CommandCounterIncrement(); @@ -10886,25 +11138,35 @@ createForeignKeyActionTriggers(Relation rel, Oid refRelOid, Constraint *fkconstr } fk_trigger->args = NIL; - (void) CreateTrigger(fk_trigger, NULL, refRelOid, RelationGetRelid(rel), - constraintOid, - indexOid, InvalidOid, InvalidOid, NULL, true, false); + trigAddress = CreateTrigger(fk_trigger, NULL, refRelOid, + RelationGetRelid(rel), + constraintOid, indexOid, InvalidOid, + parentUpdTrigger, NULL, true, false); + if (updateTrigOid) + *updateTrigOid = trigAddress.objectId; } /* * createForeignKeyCheckTriggers * Create the referencing-side "check" triggers that implement a foreign * key. + * + * Returns the OIDs of the so created triggers in *insertTrigOid and + * *updateTrigOid. */ static void createForeignKeyCheckTriggers(Oid myRelOid, Oid refRelOid, Constraint *fkconstraint, Oid constraintOid, - Oid indexOid) + Oid indexOid, + Oid parentInsTrigger, Oid parentUpdTrigger, + Oid *insertTrigOid, Oid *updateTrigOid) { - CreateFKCheckTrigger(myRelOid, refRelOid, fkconstraint, constraintOid, - indexOid, true); - CreateFKCheckTrigger(myRelOid, refRelOid, fkconstraint, constraintOid, - indexOid, false); + *insertTrigOid = CreateFKCheckTrigger(myRelOid, refRelOid, fkconstraint, + constraintOid, indexOid, + parentInsTrigger, true); + *updateTrigOid = CreateFKCheckTrigger(myRelOid, refRelOid, fkconstraint, + constraintOid, indexOid, + parentUpdTrigger, false); } /* @@ -17086,19 +17348,10 @@ CloneRowTriggersToPartition(Relation parent, Relation partition) continue; /* - * Internal triggers require careful examination. Ideally, we don't - * clone them. However, if our parent is itself a partition, there - * might be internal triggers that must not be skipped; for example, - * triggers on our parent that are in turn clones from its parent (our - * grandparent) are marked internal, yet they are to be cloned. - * - * Note we dare not verify that the other trigger belongs to an - * ancestor relation of our parent, because that creates deadlock - * opportunities. + * Don't clone internal triggers, because the constraint cloning code + * will. */ - if (trigForm->tgisinternal && - (!parent->rd_rel->relispartition || - !OidIsValid(trigForm->tgparentid))) + if (trigForm->tgisinternal) continue; /* @@ -17217,6 +17470,7 @@ ATExecDetachPartition(Relation rel, RangeVar *name) List *indexes; List *fks; ListCell *cell; + Relation trigrel = NULL; /* * We must lock the default partition, because detaching this partition @@ -17309,12 +17563,16 @@ ATExecDetachPartition(Relation rel, RangeVar *name) * additional action triggers. */ fks = copyObject(RelationGetFKeyList(partRel)); + if (fks != NIL) + trigrel = table_open(TriggerRelationId, RowExclusiveLock); foreach(cell, fks) { ForeignKeyCacheInfo *fk = lfirst(cell); HeapTuple contup; Form_pg_constraint conform; Constraint *fkconstraint; + Oid insertTriggerOid, + updateTriggerOid; contup = SearchSysCache1(CONSTROID, ObjectIdGetDatum(fk->conoid)); if (!HeapTupleIsValid(contup)) @@ -17332,6 +17590,20 @@ ATExecDetachPartition(Relation rel, RangeVar *name) /* 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. + */ + 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)); + /* * Make the action triggers on the referenced relation. When this was * a partition the action triggers pointed to the parent rel (they @@ -17346,11 +17618,15 @@ ATExecDetachPartition(Relation rel, RangeVar *name) createForeignKeyActionTriggers(partRel, conform->confrelid, fkconstraint, fk->conoid, - conform->conindid); + conform->conindid, + InvalidOid, InvalidOid, + NULL, NULL); ReleaseSysCache(contup); } list_free_deep(fks); + if (trigrel) + table_close(trigrel, RowExclusiveLock); /* * Any sub-constraints that are in the referenced-side of a larger @@ -17441,6 +17717,14 @@ DropClonedTriggersFromPartition(Oid partitionId) if (!OidIsValid(pg_trigger->tgparentid)) continue; + /* + * Ignore internal triggers that are implementation objects of foreign + * keys, because these will be detached when the foreign keys + * themselves are. + */ + if (OidIsValid(pg_trigger->tgconstrrelid)) + continue; + /* * This is ugly, but necessary: remove the dependency markings on the * trigger so that it can be removed. diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c index 788b92c7b8..c9d7702318 100644 --- a/src/backend/commands/trigger.c +++ b/src/backend/commands/trigger.c @@ -125,8 +125,10 @@ static bool before_stmt_triggers_fired(Oid relid, CmdType cmdType); * given, stmt->funcname is ignored. * * parentTriggerOid, if nonzero, is a trigger that begets this one; so that - * if that trigger is dropped, this one should be too. (This is passed as - * Invalid by most callers; it's set here when recursing on a partition.) + * if that trigger is dropped, this one should be too. There are two cases + * when a nonzero value is passed for this: 1) when this function recurses to + * create the trigger on partitions, 2) when creating child foreign key + * triggers; see CreateFKCheckTrigger() and createForeignKeyActionTriggers(). * * If whenClause is passed, it is an already-transformed expression for * WHEN. In this case, we ignore any that may come in stmt->whenClause. @@ -809,7 +811,7 @@ CreateTriggerFiringOn(CreateTrigStmt *stmt, const char *queryString, values[Anum_pg_trigger_tgfoid - 1] = ObjectIdGetDatum(funcoid); values[Anum_pg_trigger_tgtype - 1] = Int16GetDatum(tgtype); values[Anum_pg_trigger_tgenabled - 1] = trigger_fires_when; - values[Anum_pg_trigger_tgisinternal - 1] = BoolGetDatum(isInternal || in_partition); + values[Anum_pg_trigger_tgisinternal - 1] = BoolGetDatum(isInternal); values[Anum_pg_trigger_tgconstrrelid - 1] = ObjectIdGetDatum(constrrelid); values[Anum_pg_trigger_tgconstrindid - 1] = ObjectIdGetDatum(indexOid); values[Anum_pg_trigger_tgconstraint - 1] = ObjectIdGetDatum(constraintOid); @@ -1161,6 +1163,82 @@ CreateTriggerFiringOn(CreateTrigStmt *stmt, const char *queryString, return myself; } +/* + * TriggerSetParentTrigger + * Set a partition's trigger as child of its parent trigger, + * or remove the linkage if parentTrigId is InvalidOid. + * + * This updates the constraint's pg_trigger row to show it as inherited, and + * adds PARTITION dependencies to prevent the trigger from being deleted + * on its own. Alternatively, reverse that. + */ +void +TriggerSetParentTrigger(Relation trigRel, + Oid childTrigId, + Oid parentTrigId, + Oid childTableId) +{ + SysScanDesc tgscan; + ScanKeyData skey[1]; + Form_pg_trigger trigForm; + HeapTuple tuple, + newtup; + ObjectAddress depender; + ObjectAddress referenced; + + /* + * Find the trigger to delete. + */ + ScanKeyInit(&skey[0], + Anum_pg_trigger_oid, + BTEqualStrategyNumber, F_OIDEQ, + ObjectIdGetDatum(childTrigId)); + + tgscan = systable_beginscan(trigRel, TriggerOidIndexId, true, + NULL, 1, skey); + + tuple = systable_getnext(tgscan); + if (!HeapTupleIsValid(tuple)) + elog(ERROR, "could not find tuple for trigger %u", childTrigId); + newtup = heap_copytuple(tuple); + trigForm = (Form_pg_trigger) GETSTRUCT(newtup); + if (OidIsValid(parentTrigId)) + { + /* don't allow setting parent for a constraint that already has one */ + if (OidIsValid(trigForm->tgparentid)) + elog(ERROR, "trigger %u already has a parent trigger", + childTrigId); + + trigForm->tgparentid = parentTrigId; + + CatalogTupleUpdate(trigRel, &tuple->t_self, newtup); + + ObjectAddressSet(depender, TriggerRelationId, childTrigId); + + ObjectAddressSet(referenced, TriggerRelationId, parentTrigId); + recordDependencyOn(&depender, &referenced, DEPENDENCY_PARTITION_PRI); + + ObjectAddressSet(referenced, RelationRelationId, childTableId); + recordDependencyOn(&depender, &referenced, DEPENDENCY_PARTITION_SEC); + } + else + { + trigForm->tgparentid = InvalidOid; + + CatalogTupleUpdate(trigRel, &tuple->t_self, newtup); + + deleteDependencyRecordsForClass(TriggerRelationId, childTrigId, + TriggerRelationId, + DEPENDENCY_PARTITION_PRI); + deleteDependencyRecordsForClass(TriggerRelationId, childTrigId, + RelationRelationId, + DEPENDENCY_PARTITION_SEC); + } + + heap_freetuple(newtup); + systable_endscan(tgscan); +} + /* * Guts of trigger deletion. diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c index 147a860b9b..796901450e 100644 --- a/src/bin/pg_dump/pg_dump.c +++ b/src/bin/pg_dump/pg_dump.c @@ -7844,7 +7844,7 @@ getTriggers(Archive *fout, TableInfo tblinfo[], int numTables) i_tgconstrrelid, i_tgconstrrelname, i_tgenabled, - i_tgisinternal, + i_tgispartition, i_tgdeferrable, i_tginitdeferred, i_tgdef; @@ -7870,18 +7870,20 @@ getTriggers(Archive *fout, TableInfo tblinfo[], int numTables) * could result in non-forward-compatible dumps of WHEN clauses * due to under-parenthesization. * - * NB: We need to see tgisinternal triggers in partitions, in case - * the tgenabled flag has been changed from the parent. + * NB: We need to see partition triggers in case the tgenabled flag + * has been changed from the parent. */ appendPQExpBuffer(query, "SELECT t.tgname, " "t.tgfoid::pg_catalog.regproc AS tgfname, " "pg_catalog.pg_get_triggerdef(t.oid, false) AS tgdef, " - "t.tgenabled, t.tableoid, t.oid, t.tgisinternal " + "t.tgenabled, t.tableoid, t.oid, " + "t.tgparentid <> 0 AS tgispartition\n" "FROM pg_catalog.pg_trigger t " "LEFT JOIN pg_catalog.pg_trigger u ON u.oid = t.tgparentid " "WHERE t.tgrelid = '%u'::pg_catalog.oid " - "AND (NOT t.tgisinternal OR t.tgenabled != u.tgenabled)", + "AND ((NOT t.tgisinternal AND t.tgparentid = 0) " + "OR t.tgenabled != u.tgenabled) ", tbinfo->dobj.catId.oid); } else if (fout->remoteVersion >= 110000) @@ -7984,7 +7986,7 @@ getTriggers(Archive *fout, TableInfo tblinfo[], int numTables) i_tgconstrrelid = PQfnumber(res, "tgconstrrelid"); i_tgconstrrelname = PQfnumber(res, "tgconstrrelname"); i_tgenabled = PQfnumber(res, "tgenabled"); - i_tgisinternal = PQfnumber(res, "tgisinternal"); + i_tgispartition = PQfnumber(res, "tgispartition"); i_tgdeferrable = PQfnumber(res, "tgdeferrable"); i_tginitdeferred = PQfnumber(res, "tginitdeferred"); i_tgdef = PQfnumber(res, "tgdef"); @@ -8004,7 +8006,7 @@ getTriggers(Archive *fout, TableInfo tblinfo[], int numTables) tginfo[j].dobj.namespace = tbinfo->dobj.namespace; tginfo[j].tgtable = tbinfo; tginfo[j].tgenabled = *(PQgetvalue(res, j, i_tgenabled)); - tginfo[j].tgisinternal = *(PQgetvalue(res, j, i_tgisinternal)) == 't'; + tginfo[j].tgispartition = *(PQgetvalue(res, j, i_tgispartition)) == 't'; if (i_tgdef >= 0) { tginfo[j].tgdef = pg_strdup(PQgetvalue(res, j, i_tgdef)); @@ -17753,11 +17755,13 @@ dumpTrigger(Archive *fout, TriggerInfo *tginfo) "pg_catalog.pg_trigger", "TRIGGER", trigidentity->data); - if (tginfo->tgisinternal) + if (tginfo->tgispartition) { + Assert(tbinfo->ispartition); + /* - * Triggers marked internal only appear here because their 'tgenabled' - * flag differs from its parent's. The trigger is created already, so + * Partition triggers only appear here because their 'tgenabled' flag + * differs from its parent's. The trigger is created already, so * remove the CREATE and replace it with an ALTER. (Clear out the * DROP query too, so that pg_dump --create does not cause errors.) */ diff --git a/src/bin/pg_dump/pg_dump.h b/src/bin/pg_dump/pg_dump.h index 7d41064b8a..4db409a328 100644 --- a/src/bin/pg_dump/pg_dump.h +++ b/src/bin/pg_dump/pg_dump.h @@ -415,7 +415,7 @@ typedef struct _triggerInfo Oid tgconstrrelid; char *tgconstrrelname; char tgenabled; - bool tgisinternal; + bool tgispartition; bool tgdeferrable; bool tginitdeferred; char *tgdef; diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c index 384db60878..c16cc1938e 100644 --- a/src/bin/psql/describe.c +++ b/src/bin/psql/describe.c @@ -2969,10 +2969,20 @@ describeOneTableDetails(const char *schemaname, " AND u.tgparentid = 0) AS parent" : "NULL AS parent"), oid); - if (pset.sversion >= 110000) + + /* + * tgisinternal is set true for inherited triggers of partitions in + * servers betweem v11 and v14, though these must still be shown to + * the user. So we use another property that is true for such + * inherited triggers to avoid them being hidden, which is their + * dependendence on another trigger; must still hide the internal + * triggers thar originate from a constraint. + */ + if (pset.sversion >= 110000 && pset.sversion < 150000) appendPQExpBufferStr(&buf, "(NOT t.tgisinternal OR (t.tgisinternal AND t.tgenabled = 'D') \n" - " OR EXISTS (SELECT 1 FROM pg_catalog.pg_depend WHERE objid = t.oid \n" - " AND refclassid = 'pg_catalog.pg_trigger'::pg_catalog.regclass))"); + " OR (EXISTS (SELECT 1 FROM pg_catalog.pg_depend WHERE objid = t.oid \n" + " AND refclassid = 'pg_catalog.pg_trigger'::pg_catalog.regclass) \n" + " AND t.tgconstraint = 0))"); else if (pset.sversion >= 90000) /* display/warn about disabled internal triggers */ appendPQExpBufferStr(&buf, "(NOT t.tgisinternal OR (t.tgisinternal AND t.tgenabled = 'D'))"); diff --git a/src/include/commands/trigger.h b/src/include/commands/trigger.h index 40b8154876..1873ce14da 100644 --- a/src/include/commands/trigger.h +++ b/src/include/commands/trigger.h @@ -168,6 +168,10 @@ extern ObjectAddress CreateTriggerFiringOn(CreateTrigStmt *stmt, const char *que Node *whenClause, bool isInternal, bool in_partition, char trigger_fires_when); +extern void TriggerSetParentTrigger(Relation trigRel, + Oid childTrigId, + Oid parentTrigId, + Oid childTableId); extern void RemoveTriggerById(Oid trigOid); extern Oid get_trigger_oid(Oid relid, const char *name, bool missing_ok); diff --git a/src/test/regress/expected/triggers.out b/src/test/regress/expected/triggers.out index 207e0d3779..397f7763a0 100644 --- a/src/test/regress/expected/triggers.out +++ b/src/test/regress/expected/triggers.out @@ -2105,7 +2105,7 @@ select tgrelid::regclass::text, tgname, tgfoid::regproc, tgenabled, tgisinternal tgrelid | tgname | tgfoid | tgenabled | tgisinternal -----------+--------+-----------------+-----------+-------------- trigpart | trg1 | trigger_nothing | O | f - trigpart1 | trg1 | trigger_nothing | O | t + trigpart1 | trg1 | trigger_nothing | O | f (2 rows) create table trigpart3 (like trigpart); -- 2.24.1