diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index 88cfd55641..6c5a364009 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -523,7 +523,8 @@ static void ATExecSetRelOptions(Relation rel, List *defList, AlterTableType operation, LOCKMODE lockmode); static void ATExecEnableDisableTrigger(Relation rel, const char *trigname, - char fires_when, bool skip_system, LOCKMODE lockmode); + char fires_when, bool skip_system, bool recurse, + LOCKMODE lockmode); static void ATExecEnableDisableRule(Relation rel, const char *rulename, char fires_when, LOCKMODE lockmode); static void ATPrepAddInherit(Relation child_rel); @@ -3736,9 +3737,7 @@ AlterTableLookupRelation(AlterTableStmt *stmt, LOCKMODE lockmode) * be done in this phase. Generally, this phase acquires table locks, * checks permissions and relkind, and recurses to find child tables. * - * ATRewriteCatalogs performs phase 2 for each affected table. (Note that - * phases 2 and 3 normally do no explicit recursion, since phase 1 already - * did it --- although some subcommands have to recurse in phase 2 instead.) + * ATRewriteCatalogs performs phase 2 for each affected table. * Certain subcommands need to be performed before others to avoid * unnecessary conflicts; for example, DROP COLUMN should come before * ADD COLUMN. Therefore phase 1 divides the subcommands into multiple @@ -3746,6 +3745,12 @@ AlterTableLookupRelation(AlterTableStmt *stmt, LOCKMODE lockmode) * * ATRewriteTables performs phase 3 for those tables that need it. * + * For most subcommand types, phases 2 and 3 do no explicit recursion, + * since phase 1 already does it. However, for certain subcommand types + * it is only possible to determine how to recurse at phase 2 time; for + * those cases, phase 1 sets the cmd->recurse flag (or, in some older coding, + * changes the command subtype of a "Recurse" variant XXX to be cleaned up.) + * * Thanks to the magic of MVCC, an error anywhere along the way rolls back * the whole operation; we don't have to do anything special to clean up. * @@ -4144,11 +4149,12 @@ ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd, tab = ATGetQueueEntry(wqueue, rel); /* - * Copy the original subcommand for each table. This avoids conflicts - * when different child tables need to make different parse - * transformations (for example, the same column may have different column - * numbers in different children). It also ensures that we don't corrupt - * the original parse tree, in case it is saved in plancache. + * Copy the original subcommand for each table, so we can scribble on it. + * This avoids conflicts when different child tables need to make + * different parse transformations (for example, the same column may have + * different column numbers in different children). It also ensures that + * we don't corrupt the original parse tree, in case it is saved in + * plancache. */ cmd = copyObject(cmd); @@ -4411,8 +4417,9 @@ ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd, case AT_DisableTrigAll: case AT_DisableTrigUser: ATSimplePermissions(rel, ATT_TABLE | ATT_FOREIGN_TABLE); - if (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE) - ATSimpleRecursion(wqueue, rel, cmd, recurse, lockmode, context); + /* Set up recursion for phase 2; no other prep needed */ + if (recurse) + cmd->recurse = true; pass = AT_PASS_MISC; break; case AT_EnableRule: /* ENABLE/DISABLE RULE variants */ @@ -4731,35 +4738,51 @@ ATExecCmd(List **wqueue, AlteredTableInfo *tab, Relation rel, break; case AT_EnableTrig: /* ENABLE TRIGGER name */ ATExecEnableDisableTrigger(rel, cmd->name, - TRIGGER_FIRES_ON_ORIGIN, false, lockmode); + TRIGGER_FIRES_ON_ORIGIN, false, + cmd->recurse, + lockmode); break; case AT_EnableAlwaysTrig: /* ENABLE ALWAYS TRIGGER name */ ATExecEnableDisableTrigger(rel, cmd->name, - TRIGGER_FIRES_ALWAYS, false, lockmode); + TRIGGER_FIRES_ALWAYS, false, + cmd->recurse, + lockmode); break; case AT_EnableReplicaTrig: /* ENABLE REPLICA TRIGGER name */ ATExecEnableDisableTrigger(rel, cmd->name, - TRIGGER_FIRES_ON_REPLICA, false, lockmode); + TRIGGER_FIRES_ON_REPLICA, false, + cmd->recurse, + lockmode); break; case AT_DisableTrig: /* DISABLE TRIGGER name */ ATExecEnableDisableTrigger(rel, cmd->name, - TRIGGER_DISABLED, false, lockmode); + TRIGGER_DISABLED, false, + cmd->recurse, + lockmode); break; case AT_EnableTrigAll: /* ENABLE TRIGGER ALL */ ATExecEnableDisableTrigger(rel, NULL, - TRIGGER_FIRES_ON_ORIGIN, false, lockmode); + TRIGGER_FIRES_ON_ORIGIN, false, + cmd->recurse, + lockmode); break; case AT_DisableTrigAll: /* DISABLE TRIGGER ALL */ ATExecEnableDisableTrigger(rel, NULL, - TRIGGER_DISABLED, false, lockmode); + TRIGGER_DISABLED, false, + cmd->recurse, + lockmode); break; case AT_EnableTrigUser: /* ENABLE TRIGGER USER */ ATExecEnableDisableTrigger(rel, NULL, - TRIGGER_FIRES_ON_ORIGIN, true, lockmode); + TRIGGER_FIRES_ON_ORIGIN, true, + cmd->recurse, + lockmode); break; case AT_DisableTrigUser: /* DISABLE TRIGGER USER */ ATExecEnableDisableTrigger(rel, NULL, - TRIGGER_DISABLED, true, lockmode); + TRIGGER_DISABLED, true, + cmd->recurse, + lockmode); break; case AT_EnableRule: /* ENABLE RULE name */ @@ -13801,9 +13824,11 @@ index_copy_data(Relation rel, RelFileNode newrnode) */ static void ATExecEnableDisableTrigger(Relation rel, const char *trigname, - char fires_when, bool skip_system, LOCKMODE lockmode) + char fires_when, bool skip_system, bool recurse, + LOCKMODE lockmode) { - EnableDisableTrigger(rel, trigname, fires_when, skip_system, lockmode); + EnableDisableTriggerNew(rel, trigname, fires_when, skip_system, recurse, + lockmode); } /* diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c index 788b92c7b8..59b38d00ed 100644 --- a/src/backend/commands/trigger.c +++ b/src/backend/commands/trigger.c @@ -1468,14 +1468,16 @@ renametrig(RenameStmt *stmt) * enablement/disablement, this also defines when the trigger * should be fired in session replication roles. * skip_system: if true, skip "system" triggers (constraint triggers) + * recurse: if true, recurse to partitions * * Caller should have checked permissions for the table; here we also * enforce that superuser privilege is required to alter the state of * system triggers */ void -EnableDisableTrigger(Relation rel, const char *tgname, - char fires_when, bool skip_system, LOCKMODE lockmode) +EnableDisableTriggerNew(Relation rel, const char *tgname, + char fires_when, bool skip_system, bool recurse, + LOCKMODE lockmode) { Relation tgrel; int nkeys; @@ -1541,6 +1543,34 @@ EnableDisableTrigger(Relation rel, const char *tgname, changed = true; } + /* + * When altering FOR EACH ROW triggers on a partitioned table, do the + * same on the partitions as well, unless ONLY is specified. + * + * Note that we recurse even if we didn't change the trigger above, + * because the partitions' copy of the trigger may have a different + * value of tgenabled than the parent's trigger and thus might need to + * be changed. + */ + if (recurse && + rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE && + (TRIGGER_FOR_ROW(oldtrig->tgtype))) + { + PartitionDesc partdesc = RelationGetPartitionDesc(rel); + int i; + + for (i = 0; i < partdesc->nparts; i++) + { + Relation part; + + part = relation_open(partdesc->oids[i], lockmode); + EnableDisableTriggerNew(part, NameStr(oldtrig->tgname), + fires_when, skip_system, recurse, + lockmode); + table_close(part, NoLock); /* keep lock till commit */ + } + } + InvokeObjectPostAlterHook(TriggerRelationId, oldtrig->oid, 0); } @@ -1564,6 +1594,19 @@ EnableDisableTrigger(Relation rel, const char *tgname, CacheInvalidateRelcache(rel); } +/* + * ABI-compatible wrapper for the above. To keep as close possible to the old + * behavior, this never recurses. Do not call this function in new code. + */ +void +EnableDisableTrigger(Relation rel, const char *tgname, + char fires_when, bool skip_system, + LOCKMODE lockmode) +{ + EnableDisableTriggerNew(rel, tgname, fires_when, skip_system, + true, lockmode); +} + /* * Build trigger data to attach to the given relcache entry. diff --git a/src/backend/nodes/copyfuncs.c b/src/backend/nodes/copyfuncs.c index 682b28ed72..8db09fda63 100644 --- a/src/backend/nodes/copyfuncs.c +++ b/src/backend/nodes/copyfuncs.c @@ -3211,6 +3211,7 @@ _copyAlterTableCmd(const AlterTableCmd *from) COPY_NODE_FIELD(def); COPY_SCALAR_FIELD(behavior); COPY_SCALAR_FIELD(missing_ok); + COPY_SCALAR_FIELD(recurse); return newnode; } diff --git a/src/backend/nodes/equalfuncs.c b/src/backend/nodes/equalfuncs.c index 8dc50aea81..092fb37923 100644 --- a/src/backend/nodes/equalfuncs.c +++ b/src/backend/nodes/equalfuncs.c @@ -1103,6 +1103,7 @@ _equalAlterTableCmd(const AlterTableCmd *a, const AlterTableCmd *b) COMPARE_NODE_FIELD(def); COMPARE_SCALAR_FIELD(behavior); COMPARE_SCALAR_FIELD(missing_ok); + COMPARE_SCALAR_FIELD(recurse); return true; } diff --git a/src/include/commands/trigger.h b/src/include/commands/trigger.h index 40b8154876..b76d91af05 100644 --- a/src/include/commands/trigger.h +++ b/src/include/commands/trigger.h @@ -173,6 +173,9 @@ extern Oid get_trigger_oid(Oid relid, const char *name, bool missing_ok); extern ObjectAddress renametrig(RenameStmt *stmt); +extern void EnableDisableTriggerNew(Relation rel, const char *tgname, + char fires_when, bool skip_system, bool recurse, + LOCKMODE lockmode); extern void EnableDisableTrigger(Relation rel, const char *tgname, char fires_when, bool skip_system, LOCKMODE lockmode); diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h index b907e97207..943848782f 100644 --- a/src/include/nodes/parsenodes.h +++ b/src/include/nodes/parsenodes.h @@ -1876,6 +1876,7 @@ typedef struct AlterTableCmd /* one subcommand of an ALTER TABLE */ * constraint, or parent table */ DropBehavior behavior; /* RESTRICT or CASCADE for DROP cases */ bool missing_ok; /* skip error if missing? */ + bool recurse; /* exec-time recursion */ } AlterTableCmd; diff --git a/src/test/regress/expected/triggers.out b/src/test/regress/expected/triggers.out index fd38350340..8c3bee2bf1 100644 --- a/src/test/regress/expected/triggers.out +++ b/src/test/regress/expected/triggers.out @@ -2655,24 +2655,42 @@ create table parent (a int) partition by list (a); create table child1 partition of parent for values in (1); create trigger tg after insert on parent for each row execute procedure trig_nothing(); +create trigger tg_stmt after insert on parent + for statement execute procedure trig_nothing(); select tgrelid::regclass, tgname, tgenabled from pg_trigger where tgrelid in ('parent'::regclass, 'child1'::regclass) order by tgrelid::regclass::text; - tgrelid | tgname | tgenabled ----------+--------+----------- - child1 | tg | O - parent | tg | O -(2 rows) + tgrelid | tgname | tgenabled +---------+---------+----------- + child1 | tg | O + parent | tg | O + parent | tg_stmt | O +(3 rows) -alter table only parent enable always trigger tg; +alter table only parent enable always trigger tg; -- no recursion because ONLY +alter table parent enable always trigger tg_stmt; -- no recursion because statement trigger select tgrelid::regclass, tgname, tgenabled from pg_trigger where tgrelid in ('parent'::regclass, 'child1'::regclass) order by tgrelid::regclass::text; - tgrelid | tgname | tgenabled ----------+--------+----------- - child1 | tg | O - parent | tg | A -(2 rows) + tgrelid | tgname | tgenabled +---------+---------+----------- + child1 | tg | O + parent | tg | A + parent | tg_stmt | A +(3 rows) + +-- The following is a no-op for the parent trigger but not so +-- for the child trigger, so recursion should be applied. +alter table parent enable always trigger tg; +select tgrelid::regclass, tgname, tgenabled from pg_trigger + where tgrelid in ('parent'::regclass, 'child1'::regclass) + order by tgrelid::regclass::text; + tgrelid | tgname | tgenabled +---------+---------+----------- + child1 | tg | A + parent | tg | A + parent | tg_stmt | A +(3 rows) drop table parent, child1; -- Verify that firing state propagates correctly on creation, too diff --git a/src/test/regress/sql/triggers.sql b/src/test/regress/sql/triggers.sql index 353dd6a307..b9c49c2cb7 100644 --- a/src/test/regress/sql/triggers.sql +++ b/src/test/regress/sql/triggers.sql @@ -1832,10 +1832,19 @@ create table parent (a int) partition by list (a); create table child1 partition of parent for values in (1); create trigger tg after insert on parent for each row execute procedure trig_nothing(); +create trigger tg_stmt after insert on parent + for statement execute procedure trig_nothing(); select tgrelid::regclass, tgname, tgenabled from pg_trigger where tgrelid in ('parent'::regclass, 'child1'::regclass) order by tgrelid::regclass::text; -alter table only parent enable always trigger tg; +alter table only parent enable always trigger tg; -- no recursion because ONLY +alter table parent enable always trigger tg_stmt; -- no recursion because statement trigger +select tgrelid::regclass, tgname, tgenabled from pg_trigger + where tgrelid in ('parent'::regclass, 'child1'::regclass) + order by tgrelid::regclass::text; +-- The following is a no-op for the parent trigger but not so +-- for the child trigger, so recursion should be applied. +alter table parent enable always trigger tg; select tgrelid::regclass, tgname, tgenabled from pg_trigger where tgrelid in ('parent'::regclass, 'child1'::regclass) order by tgrelid::regclass::text;