From a795f6782fa8b466ee47bd6d3ff8e9075237d566 Mon Sep 17 00:00:00 2001 From: Alvaro Herrera Date: Tue, 20 Oct 2020 19:22:09 -0300 Subject: [PATCH] Fix ALTER TABLE .. ENABLE/DISABLE TRIGGER recursion More precisely, correctly handle the ONLY flag indicating not to recurse. This was implemented in 86f575948c77 by recursing in trigger.c, but that's the wrong place; use ATSimpleRecursion instead, which behaves properly. However, because legacy inheritance has never recursed in that situation, make sure to do that only for new-style partitioning. I noticed this problem while testing a fix for another bug in the vicinity. This has been wrong all along, so backpatch to 11. Discussion: https://postgr.es/m/20201016235925.GA29829@alvherre.pgsql --- src/backend/commands/tablecmds.c | 2 + src/backend/commands/trigger.c | 21 ---------- src/test/regress/expected/triggers.out | 56 ++++++++++++++++++++++++++ src/test/regress/sql/triggers.sql | 35 ++++++++++++++++ 4 files changed, 93 insertions(+), 21 deletions(-) diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index b46cf4f1b5..18dcb65f9f 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -3978,6 +3978,8 @@ 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); pass = AT_PASS_MISC; break; case AT_EnableRule: /* ENABLE/DISABLE RULE variants */ diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c index ad26fb77f3..2a1bf8bf46 100644 --- a/src/backend/commands/trigger.c +++ b/src/backend/commands/trigger.c @@ -1847,27 +1847,6 @@ 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. - */ - if (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); - EnableDisableTrigger(part, NameStr(oldtrig->tgname), - fires_when, skip_system, lockmode); - heap_close(part, NoLock); /* keep lock till commit */ - } - } - changed = true; } diff --git a/src/test/regress/expected/triggers.out b/src/test/regress/expected/triggers.out index 8f7e9a872c..df6c2498e7 100644 --- a/src/test/regress/expected/triggers.out +++ b/src/test/regress/expected/triggers.out @@ -2440,6 +2440,62 @@ select tgrelid::regclass, count(*) from pg_trigger (5 rows) drop table trg_clone; +-- Test the interaction between ALTER TABLE .. DISABLE TRIGGER and +-- both kinds of inheritance. Historically, legacy inheritance has +-- not recursed to children, so that behavior is preserved. +create table parent (a int); +create table child1 () inherits (parent); +create function trig_nothing() returns trigger language plpgsql + as $$ begin return null; end $$; +create trigger tg after insert on parent + for each row execute function trig_nothing(); +create trigger tg after insert on child1 + for each row execute function trig_nothing(); +alter table parent disable 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 | O + parent | tg | D +(2 rows) + +alter table only 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 | O + parent | tg | A +(2 rows) + +drop table parent, child1; +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(); +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) + +alter table only 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 | O + parent | tg | A +(2 rows) + +drop table parent, child1; -- -- Test the interaction between transition tables and both kinds of -- inheritance. We'll dump the contents of the transition tables in a diff --git a/src/test/regress/sql/triggers.sql b/src/test/regress/sql/triggers.sql index 3e9a4d98ce..06043b2020 100644 --- a/src/test/regress/sql/triggers.sql +++ b/src/test/regress/sql/triggers.sql @@ -1703,6 +1703,41 @@ select tgrelid::regclass, count(*) from pg_trigger group by tgrelid::regclass order by tgrelid::regclass; drop table trg_clone; +-- Test the interaction between ALTER TABLE .. DISABLE TRIGGER and +-- both kinds of inheritance. Historically, legacy inheritance has +-- not recursed to children, so that behavior is preserved. +create table parent (a int); +create table child1 () inherits (parent); +create function trig_nothing() returns trigger language plpgsql + as $$ begin return null; end $$; +create trigger tg after insert on parent + for each row execute function trig_nothing(); +create trigger tg after insert on child1 + for each row execute function trig_nothing(); +alter table parent disable trigger tg; +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; +select tgrelid::regclass, tgname, tgenabled from pg_trigger + where tgrelid in ('parent'::regclass, 'child1'::regclass) + order by tgrelid::regclass::text; +drop table parent, child1; + +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(); +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; +select tgrelid::regclass, tgname, tgenabled from pg_trigger + where tgrelid in ('parent'::regclass, 'child1'::regclass) + order by tgrelid::regclass::text; +drop table parent, child1; + + -- -- Test the interaction between transition tables and both kinds of -- inheritance. We'll dump the contents of the transition tables in a