On 2022-May-24, Amit Langote wrote:
> So, I think we should do something like the attached. A lot of
> boilerplate is needed given that the various enable/disable trigger
> variants are represented as separate sub-commands (AlterTableCmd
> subtypes), which can perhaps be avoided by inventing a
> EnableDisableTrigStmt sub-command node that stores (only?) the recurse
> flag.
Yeah, I don't know about adding tons of values to that enum just so that
we can use that to hide a boolean inside. Why not add a boolean to the
containing struct? Something like the attached.
We can later use the same thing to undo what happens in in AddColumn,
DropColumn, etc. It all looks pretty strange and confusing to me.
--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
"Investigación es lo que hago cuando no sé lo que estoy haciendo"
(Wernher von Braun)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index e7aef2f6b0..99fcd728dd 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -580,7 +580,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);
@@ -4777,8 +4778,10 @@ ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd,
case AT_DisableTrigAll:
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);
+ /* Recursion occurs during execution phase */
+ /* No command-specific prep needed except saving recurse flag */
+ if (recurse)
+ cmd->execTimeRecursion = true;
pass = AT_PASS_MISC;
break;
case AT_EnableRule: /* ENABLE/DISABLE RULE variants */
@@ -5119,35 +5122,51 @@ 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,
+ cmd->execTimeRecursion,
+ lockmode);
break;
case AT_EnableAlwaysTrig: /* ENABLE ALWAYS TRIGGER name */
ATExecEnableDisableTrigger(rel, cmd->name,
- TRIGGER_FIRES_ALWAYS, false, lockmode);
+ TRIGGER_FIRES_ALWAYS, false,
+ cmd->execTimeRecursion,
+ 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->execTimeRecursion,
+ lockmode);
break;
case AT_DisableTrig: /* DISABLE TRIGGER name */
ATExecEnableDisableTrigger(rel, cmd->name,
- TRIGGER_DISABLED, false, lockmode);
+ TRIGGER_DISABLED, false,
+ cmd->execTimeRecursion,
+ lockmode);
break;
case AT_EnableTrigAll: /* ENABLE TRIGGER ALL */
ATExecEnableDisableTrigger(rel, NULL,
- TRIGGER_FIRES_ON_ORIGIN, false, lockmode);
+ TRIGGER_FIRES_ON_ORIGIN, false,
+ cmd->execTimeRecursion,
+ lockmode);
break;
case AT_DisableTrigAll: /* DISABLE TRIGGER ALL */
ATExecEnableDisableTrigger(rel, NULL,
- TRIGGER_DISABLED, false, lockmode);
+ TRIGGER_DISABLED, false,
+ cmd->execTimeRecursion,
+ lockmode);
break;
case AT_EnableTrigUser: /* ENABLE TRIGGER USER */
ATExecEnableDisableTrigger(rel, NULL,
- TRIGGER_FIRES_ON_ORIGIN, true, lockmode);
+ TRIGGER_FIRES_ON_ORIGIN, true,
+ cmd->execTimeRecursion,
+ lockmode);
break;
case AT_DisableTrigUser: /* DISABLE TRIGGER USER */
ATExecEnableDisableTrigger(rel, NULL,
- TRIGGER_DISABLED, true, lockmode);
+ TRIGGER_DISABLED, true,
+ cmd->execTimeRecursion,
+ lockmode);
break;
case AT_EnableRule: /* ENABLE RULE name */
@@ -14660,9 +14679,11 @@ index_copy_data(Relation rel, RelFileLocator newrlocator)
*/
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 b8db53b66d..b0416ada94 100644
--- a/src/backend/commands/trigger.c
+++ b/src/backend/commands/trigger.c
@@ -1752,6 +1752,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
@@ -1759,7 +1760,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;
@@ -1825,6 +1827,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, 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 */
+ }
+ }
+
InvokeObjectPostAlterHook(TriggerRelationId,
oldtrig->oid, 0);
}
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 98fe1abaa2..030ea7dfe2 100644
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -2328,6 +2328,8 @@ 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 execTimeRecursion; /* set by ATPrepCmd if ATExecCmd must
+ * recurse to children */
} AlterTableCmd;
diff --git a/src/test/regress/expected/triggers.out b/src/test/regress/expected/triggers.out
index 89a34ffbb2..f131405bc7 100644
--- a/src/test/regress/expected/triggers.out
+++ b/src/test/regress/expected/triggers.out
@@ -2681,24 +2681,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 83cd00f54f..cb6fc4a90e 100644
--- a/src/test/regress/sql/triggers.sql
+++ b/src/test/regress/sql/triggers.sql
@@ -1865,10 +1865,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;
