From 01a71dc0af9f2d6190863754aabbe1bc04bc40e4 Mon Sep 17 00:00:00 2001 From: amitlan Date: Tue, 24 May 2022 12:43:48 +0900 Subject: [PATCH v1] Fix ENABLE/DISABLE TRIGGER to handle recursion correctly Using ATSimpleRecursion() as bbb927b4 taught it to do is not correct, because it can't understand that some triggers are not cloned, so need not be recursivel handled. This restores the original code in trigger.c: EnableDisableTrigger() to do the recursion so that it is only performed for triggers that are cloned -- only row-level triggers -- and also changes tablecmds.c to pass the value of the ONLY flag down to EnableDisableTrigger() so the latter knows whether or not the ALTER TABLE command has requested recursion. --- src/backend/commands/tablecmds.c | 131 ++++++++++++++++++++++--- src/backend/commands/trigger.c | 27 ++++- src/include/commands/trigger.h | 3 +- src/include/nodes/parsenodes.h | 8 ++ src/test/regress/expected/triggers.out | 27 ++--- src/test/regress/sql/triggers.sql | 5 +- 6 files changed, 174 insertions(+), 27 deletions(-) diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index 2de0ebacec..a09680da9b 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -579,7 +579,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); @@ -4756,16 +4757,59 @@ ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd, /* No command-specific prep needed */ break; case AT_EnableTrig: /* ENABLE TRIGGER variants */ + ATSimplePermissions(cmd->subtype, rel, ATT_TABLE | ATT_FOREIGN_TABLE); + /* No command-specific prep needed except saving recurse flag */ + if (recurse) + cmd->subtype = AT_EnableTrigRecurse; + pass = AT_PASS_MISC; + break; case AT_EnableAlwaysTrig: + ATSimplePermissions(cmd->subtype, rel, ATT_TABLE | ATT_FOREIGN_TABLE); + /* No command-specific prep needed except saving recurse flag */ + if (recurse) + cmd->subtype = AT_EnableAlwaysTrigRecurse; + pass = AT_PASS_MISC; + break; case AT_EnableReplicaTrig: + ATSimplePermissions(cmd->subtype, rel, ATT_TABLE | ATT_FOREIGN_TABLE); + /* No command-specific prep needed except saving recurse flag */ + if (recurse) + cmd->subtype = AT_EnableReplicaTrigRecurse; + pass = AT_PASS_MISC; + break; case AT_EnableTrigAll: + ATSimplePermissions(cmd->subtype, rel, ATT_TABLE | ATT_FOREIGN_TABLE); + /* No command-specific prep needed except saving recurse flag */ + if (recurse) + cmd->subtype = AT_EnableTrigAllRecurse; + pass = AT_PASS_MISC; + break; case AT_EnableTrigUser: + ATSimplePermissions(cmd->subtype, rel, ATT_TABLE | ATT_FOREIGN_TABLE); + /* No command-specific prep needed except saving recurse flag */ + if (recurse) + cmd->subtype = AT_EnableTrigUserRecurse; + pass = AT_PASS_MISC; + break; case AT_DisableTrig: /* DISABLE TRIGGER variants */ + ATSimplePermissions(cmd->subtype, rel, ATT_TABLE | ATT_FOREIGN_TABLE); + /* No command-specific prep needed except saving recurse flag */ + if (recurse) + cmd->subtype = AT_DisableTrigRecurse; + pass = AT_PASS_MISC; + break; case AT_DisableTrigAll: + ATSimplePermissions(cmd->subtype, rel, ATT_TABLE | ATT_FOREIGN_TABLE); + /* No command-specific prep needed except saving recurse flag */ + if (recurse) + cmd->subtype = AT_DisableTrigAllRecurse; + pass = AT_PASS_MISC; + break; case AT_DisableTrigUser: ATSimplePermissions(cmd->subtype, rel, ATT_TABLE | ATT_FOREIGN_TABLE); - if (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE) - ATSimpleRecursion(wqueue, rel, cmd, recurse, lockmode, context); + /* No command-specific prep needed except saving recurse flag */ + if (recurse) + cmd->subtype = AT_DisableTrigUserRecurse; pass = AT_PASS_MISC; break; case AT_EnableRule: /* ENABLE/DISABLE RULE variants */ @@ -5106,35 +5150,86 @@ ATExecCmd(List **wqueue, AlteredTableInfo *tab, break; case AT_EnableTrig: /* ENABLE TRIGGER name */ ATExecEnableDisableTrigger(rel, cmd->name, - TRIGGER_FIRES_ON_ORIGIN, false, lockmode); + TRIGGER_FIRES_ON_ORIGIN, false, false, + lockmode); + break; + case AT_EnableTrigRecurse: /* ENABLE TRIGGER name with recursion */ + ATExecEnableDisableTrigger(rel, cmd->name, + TRIGGER_FIRES_ON_ORIGIN, false, true, + lockmode); break; case AT_EnableAlwaysTrig: /* ENABLE ALWAYS TRIGGER name */ ATExecEnableDisableTrigger(rel, cmd->name, - TRIGGER_FIRES_ALWAYS, false, lockmode); + TRIGGER_FIRES_ALWAYS, false, false, + lockmode); + break; + case AT_EnableAlwaysTrigRecurse: /* ENABLE ALWAYS TRIGGER name with + * recursion */ + ATExecEnableDisableTrigger(rel, cmd->name, + TRIGGER_FIRES_ALWAYS, false, true, + lockmode); break; case AT_EnableReplicaTrig: /* ENABLE REPLICA TRIGGER name */ ATExecEnableDisableTrigger(rel, cmd->name, - TRIGGER_FIRES_ON_REPLICA, false, lockmode); + TRIGGER_FIRES_ON_REPLICA, false, false, + lockmode); + break; + case AT_EnableReplicaTrigRecurse: /* ENABLE REPLICA TRIGGER name with + * recursion */ + ATExecEnableDisableTrigger(rel, cmd->name, + TRIGGER_FIRES_ON_REPLICA, false, true, + lockmode); break; case AT_DisableTrig: /* DISABLE TRIGGER name */ ATExecEnableDisableTrigger(rel, cmd->name, - TRIGGER_DISABLED, false, lockmode); + TRIGGER_DISABLED, false, false, + lockmode); + break; + case AT_DisableTrigRecurse: /* DISABLE TRIGGER name with recursion */ + ATExecEnableDisableTrigger(rel, cmd->name, + TRIGGER_DISABLED, false, true, + lockmode); break; case AT_EnableTrigAll: /* ENABLE TRIGGER ALL */ ATExecEnableDisableTrigger(rel, NULL, - TRIGGER_FIRES_ON_ORIGIN, false, lockmode); + TRIGGER_FIRES_ON_ORIGIN, false, false, + lockmode); + break; + case AT_EnableTrigAllRecurse: /* ENABLE TRIGGER ALL with recursion */ + ATExecEnableDisableTrigger(rel, NULL, + TRIGGER_FIRES_ON_ORIGIN, false, true, + lockmode); break; case AT_DisableTrigAll: /* DISABLE TRIGGER ALL */ ATExecEnableDisableTrigger(rel, NULL, - TRIGGER_DISABLED, false, lockmode); + TRIGGER_DISABLED, false, false, + lockmode); + break; + case AT_DisableTrigAllRecurse: /* DISABLE TRIGGER ALL with recursion */ + ATExecEnableDisableTrigger(rel, NULL, + TRIGGER_DISABLED, false, true, + lockmode); break; case AT_EnableTrigUser: /* ENABLE TRIGGER USER */ ATExecEnableDisableTrigger(rel, NULL, - TRIGGER_FIRES_ON_ORIGIN, true, lockmode); + TRIGGER_FIRES_ON_ORIGIN, true, false, + lockmode); + break; + case AT_EnableTrigUserRecurse: /* ENABLE TRIGGER USER with recursion */ + ATExecEnableDisableTrigger(rel, NULL, + TRIGGER_FIRES_ON_ORIGIN, true, true, + lockmode); break; case AT_DisableTrigUser: /* DISABLE TRIGGER USER */ ATExecEnableDisableTrigger(rel, NULL, - TRIGGER_DISABLED, true, lockmode); + TRIGGER_DISABLED, true, false, + lockmode); + break; + case AT_DisableTrigUserRecurse: /* DISABLE TRIGGER USER with + * recursion */ + ATExecEnableDisableTrigger(rel, NULL, + TRIGGER_DISABLED, true, true, + lockmode); break; case AT_EnableRule: /* ENABLE RULE name */ @@ -6146,20 +6241,28 @@ alter_table_type_to_string(AlterTableType cmdtype) case AT_ReplaceRelOptions: return NULL; /* not real grammar */ case AT_EnableTrig: + case AT_EnableTrigRecurse: return "ENABLE TRIGGER"; case AT_EnableAlwaysTrig: + case AT_EnableAlwaysTrigRecurse: return "ENABLE ALWAYS TRIGGER"; case AT_EnableReplicaTrig: + case AT_EnableReplicaTrigRecurse: return "ENABLE REPLICA TRIGGER"; case AT_DisableTrig: + case AT_DisableTrigRecurse: return "DISABLE TRIGGER"; case AT_EnableTrigAll: + case AT_EnableTrigAllRecurse: return "ENABLE TRIGGER ALL"; case AT_DisableTrigAll: + case AT_DisableTrigAllRecurse: return "DISABLE TRIGGER ALL"; case AT_EnableTrigUser: + case AT_EnableTrigUserRecurse: return "ENABLE TRIGGER USER"; case AT_DisableTrigUser: + case AT_DisableTrigUserRecurse: return "DISABLE TRIGGER USER"; case AT_EnableRule: return "ENABLE RULE"; @@ -14690,9 +14793,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); + EnableDisableTrigger(rel, trigname, fires_when, skip_system, recurse, + lockmode); } /* diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c index 13cb516752..94aec8d7d1 100644 --- a/src/backend/commands/trigger.c +++ b/src/backend/commands/trigger.c @@ -1759,6 +1759,7 @@ renametrig_partition(Relation tgrel, Oid partitionId, Oid parentTriggerOid, * 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 @@ -1766,7 +1767,8 @@ renametrig_partition(Relation tgrel, Oid partitionId, Oid parentTriggerOid, */ void EnableDisableTrigger(Relation rel, const char *tgname, - char fires_when, bool skip_system, LOCKMODE lockmode) + char fires_when, bool skip_system, bool recurse, + LOCKMODE lockmode) { Relation tgrel; int nkeys; @@ -1829,6 +1831,29 @@ EnableDisableTrigger(Relation rel, const char *tgname, heap_freetuple(newtup); + /* + * When altering FOR EACH ROW triggers on a partitioned table, do + * the same on the partitions as well, unless ONLY is specified. + */ + if (recurse && + rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE && + (TRIGGER_FOR_ROW(oldtrig->tgtype))) + { + PartitionDesc partdesc = RelationGetPartitionDesc(rel, true); + int i; + + for (i = 0; i < partdesc->nparts; i++) + { + Relation part; + + part = relation_open(partdesc->oids[i], lockmode); + EnableDisableTrigger(part, NameStr(oldtrig->tgname), + fires_when, skip_system, recurse, + lockmode); + table_close(part, NoLock); /* keep lock till commit */ + } + } + changed = true; } diff --git a/src/include/commands/trigger.h b/src/include/commands/trigger.h index 194e8d5bc1..b7b6bd6008 100644 --- a/src/include/commands/trigger.h +++ b/src/include/commands/trigger.h @@ -171,7 +171,8 @@ extern Oid get_trigger_oid(Oid relid, const char *name, bool missing_ok); extern ObjectAddress renametrig(RenameStmt *stmt); extern void EnableDisableTrigger(Relation rel, const char *tgname, - char fires_when, bool skip_system, LOCKMODE lockmode); + char fires_when, bool skip_system, bool recurse, + LOCKMODE lockmode); extern void RelationBuildTriggers(Relation relation); diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h index 73f635b455..d9644c72eb 100644 --- a/src/include/nodes/parsenodes.h +++ b/src/include/nodes/parsenodes.h @@ -2267,13 +2267,21 @@ typedef enum AlterTableType AT_ResetRelOptions, /* RESET (...) -- AM specific parameters */ AT_ReplaceRelOptions, /* replace reloption list in its entirety */ AT_EnableTrig, /* ENABLE TRIGGER name */ + AT_EnableTrigRecurse, /* internal to commands/tablecmds.c */ AT_EnableAlwaysTrig, /* ENABLE ALWAYS TRIGGER name */ + AT_EnableAlwaysTrigRecurse, /* internal to commands/tablecmds.c */ AT_EnableReplicaTrig, /* ENABLE REPLICA TRIGGER name */ + AT_EnableReplicaTrigRecurse, /* internal to commands/tablecmds.c */ AT_DisableTrig, /* DISABLE TRIGGER name */ + AT_DisableTrigRecurse, /* internal to commands/tablecmds.c */ AT_EnableTrigAll, /* ENABLE TRIGGER ALL */ + AT_EnableTrigAllRecurse, /* internal to commands/tablecmds.c */ AT_DisableTrigAll, /* DISABLE TRIGGER ALL */ + AT_DisableTrigAllRecurse, /* internal to commands/tablecmds.c */ AT_EnableTrigUser, /* ENABLE TRIGGER USER */ + AT_EnableTrigUserRecurse, /* internal to commands/tablecmds.c */ AT_DisableTrigUser, /* DISABLE TRIGGER USER */ + AT_DisableTrigUserRecurse, /* internal to commands/tablecmds.c */ AT_EnableRule, /* ENABLE RULE name */ AT_EnableAlwaysRule, /* ENABLE ALWAYS RULE name */ AT_EnableReplicaRule, /* ENABLE REPLICA RULE name */ diff --git a/src/test/regress/expected/triggers.out b/src/test/regress/expected/triggers.out index cd812336f2..05bc71ee7e 100644 --- a/src/test/regress/expected/triggers.out +++ b/src/test/regress/expected/triggers.out @@ -2681,24 +2681,29 @@ 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) 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 83cd00f54f..5cf1e90e84 100644 --- a/src/test/regress/sql/triggers.sql +++ b/src/test/regress/sql/triggers.sql @@ -1865,10 +1865,13 @@ 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; -- 2.35.3