From e02f2a1e4eb0975e984af96af09ca50cbae81e26 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 | 101 +++++- 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 | 4 +- 7 files changed, 473 insertions(+), 92 deletions(-) diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index 4729a895e8..07051a5b75 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -482,12 +482,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); @@ -495,15 +497,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, @@ -9227,7 +9244,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, @@ -9240,7 +9258,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. @@ -9274,13 +9293,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; @@ -9288,6 +9312,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 @@ -9384,15 +9410,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 @@ -9436,7 +9460,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); @@ -9479,14 +9504,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) @@ -9495,19 +9527,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 @@ -9536,6 +9570,15 @@ addFkRecurseReferencing(List **wqueue, Constraint *fkconstraint, Relation rel, else if (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE) { PartitionDesc pd = RelationGetPartitionDesc(rel, true); + 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 @@ -9577,7 +9620,10 @@ addFkRecurseReferencing(List **wqueue, Constraint *fkconstraint, Relation rel, numfks, mapped_fkattnum, pkattnum, - pfeqoperators)) + pfeqoperators, + insertTriggerOid, + updateTriggerOid, + trigrel)) { attached = true; break; @@ -9656,10 +9702,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); } } @@ -9715,6 +9765,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 @@ -9744,6 +9795,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) @@ -9761,6 +9820,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)) @@ -9827,6 +9888,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, @@ -9839,11 +9910,15 @@ CloneFkReferenced(Relation parentRel, Relation partitionRel) conpfeqop, conppeqop, conffeqop, - true); + true, + deleteTriggerOid, + updateTriggerOid); table_close(fkRel, NoLock); ReleaseSysCache(tuple); } + + table_close(trigrel, RowExclusiveLock); } /* @@ -9866,6 +9941,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)) @@ -9887,6 +9963,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. @@ -9916,6 +10000,8 @@ CloneFkReferencing(List **wqueue, Relation parentRel, Relation partRel) ObjectAddress address, referenced; ListCell *cell; + Oid insertTriggerOid, + updateTriggerOid; tuple = SearchSysCache1(CONSTROID, parentConstrOid); if (!HeapTupleIsValid(tuple)) @@ -9944,6 +10030,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 @@ -9962,7 +10061,10 @@ CloneFkReferencing(List **wqueue, Relation parentRel, Relation partRel) numfks, mapped_conkey, confkey, - conpfeqop)) + conpfeqop, + insertTriggerOid, + updateTriggerOid, + trigrel)) { attached = true; table_close(pkrel, NoLock); @@ -10061,9 +10163,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); } /* @@ -10084,16 +10190,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)); @@ -10154,12 +10264,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) @@ -10192,13 +10300,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 @@ -11115,10 +11346,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; /* @@ -11158,23 +11398,32 @@ CreateFKCheckTrigger(Oid myRelOid, Oid refRelOid, Constraint *fkconstraint, fk_trigger->initdeferred = fkconstraint->initdeferred; fk_trigger->constrrel = NULL; - (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 @@ -11226,9 +11475,12 @@ createForeignKeyActionTriggers(Relation rel, Oid refRelOid, Constraint *fkconstr break; } - (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(); @@ -11283,25 +11535,35 @@ createForeignKeyActionTriggers(Relation rel, Oid refRelOid, Constraint *fkconstr break; } - (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); } /* @@ -17596,19 +17858,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; /* @@ -17910,6 +18163,7 @@ DetachPartitionFinalize(Relation rel, Relation partRel, bool concurrent, new_repl[Natts_pg_class]; HeapTuple tuple, newtuple; + Relation trigrel = NULL; if (concurrent) { @@ -17928,12 +18182,16 @@ DetachPartitionFinalize(Relation rel, Relation partRel, bool concurrent, * 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)) @@ -17951,6 +18209,20 @@ DetachPartitionFinalize(Relation rel, Relation partRel, bool concurrent, /* 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 @@ -17965,11 +18237,15 @@ DetachPartitionFinalize(Relation rel, Relation partRel, bool concurrent, 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 @@ -18194,6 +18470,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 40441fdb4c..7b12ee921e 100644 --- a/src/backend/commands/trigger.c +++ b/src/backend/commands/trigger.c @@ -126,8 +126,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. @@ -196,6 +198,7 @@ CreateTriggerFiringOn(CreateTrigStmt *stmt, const char *queryString, bool trigger_exists = false; Oid existing_constraint_oid = InvalidOid; bool existing_isInternal = false; + bool existing_isClone = false; if (OidIsValid(relOid)) rel = table_open(relOid, ShareRowExclusiveLock); @@ -734,6 +737,7 @@ CreateTriggerFiringOn(CreateTrigStmt *stmt, const char *queryString, trigoid = oldtrigger->oid; existing_constraint_oid = oldtrigger->tgconstraint; existing_isInternal = oldtrigger->tgisinternal; + existing_isClone = OidIsValid(oldtrigger->tgparentid); trigger_exists = true; /* copy the tuple to use in CatalogTupleUpdate() */ tuple = heap_copytuple(tuple); @@ -760,17 +764,16 @@ CreateTriggerFiringOn(CreateTrigStmt *stmt, const char *queryString, stmt->trigname, RelationGetRelationName(rel)))); /* - * An internal trigger cannot be replaced by a user-defined trigger. - * However, skip this test when in_partition, because then we're - * recursing from a partitioned table and the check was made at the - * parent level. Child triggers will always be marked "internal" (so - * this test does protect us from the user trying to replace a child - * trigger directly). + * An internal trigger or a child trigger (isClone) cannot be replaced + * by a user-defined trigger. However, skip this test when + * in_partition, because then we're recursing from a partitioned table + * and the check was made at the parent level. */ - if (existing_isInternal && !isInternal && !in_partition) + if ((existing_isInternal || existing_isClone) && + !isInternal && !in_partition) ereport(ERROR, (errcode(ERRCODE_DUPLICATE_OBJECT), - errmsg("trigger \"%s\" for relation \"%s\" is an internal trigger", + errmsg("trigger \"%s\" for relation \"%s\" is an internal or a child trigger", stmt->trigname, RelationGetRelationName(rel)))); /* @@ -867,7 +870,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); @@ -1236,6 +1239,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 53d3af08fe..cba909ab7e 100644 --- a/src/bin/pg_dump/pg_dump.c +++ b/src/bin/pg_dump/pg_dump.c @@ -7974,7 +7974,7 @@ getTriggers(Archive *fout, TableInfo tblinfo[], int numTables) i_tgconstrrelid, i_tgconstrrelname, i_tgenabled, - i_tgisinternal, + i_tgispartition, i_tgdeferrable, i_tginitdeferred, i_tgdef; @@ -8000,18 +8000,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) @@ -8114,7 +8116,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"); @@ -8134,7 +8136,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)); @@ -17763,11 +17765,13 @@ dumpTrigger(Archive *fout, const 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 8b2a8b67e7..0a58674a78 100644 --- a/src/bin/pg_dump/pg_dump.h +++ b/src/bin/pg_dump/pg_dump.h @@ -425,7 +425,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 48348750ee..f57d0af289 100644 --- a/src/bin/psql/describe.c +++ b/src/bin/psql/describe.c @@ -3266,10 +3266,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 9ef7f6d768..489c93de92 100644 --- a/src/include/commands/trigger.h +++ b/src/include/commands/trigger.h @@ -160,6 +160,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 5254447cf8..ae342bae7f 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); @@ -3311,7 +3311,7 @@ NOTICE: hello from funcA create or replace trigger my_trig after insert on parted_trig_1 for each row execute procedure funcB(); -- should fail -ERROR: trigger "my_trig" for relation "parted_trig_1" is an internal trigger +ERROR: trigger "my_trig" for relation "parted_trig_1" is an internal or a child trigger insert into parted_trig (a) values (50); NOTICE: hello from funcA drop trigger my_trig on parted_trig; -- 2.24.1