10000 Fix ENABLE/DISABLE TRIGGER to handle recursion correctly · postgres/postgres@ce8e066 · GitHub
[go: up one dir, main page]
Skip to content

Commit ce8e066

Browse files
alvherreamitlan
alvherre
and
amitlan
committed
Fix ENABLE/DISABLE TRIGGER to handle recursion correctly
Using ATSimpleRecursion() in ATPrepCmd() to do so as bbb927b did is not correct, because ATPrepCmd() can't distinguish between triggers that may be cloned and those that may not, so would wrongly try to recurse for the latter category of triggers. So this commit restores the code in EnableDisableTrigger() that 86f5759 had added to do the recursion, which would do it only for triggers that may be cloned, that is, row-level triggers. This also changes tablecmds.c such that ATExecCmd() is able to pass the value of ONLY flag down to EnableDisableTrigger() using its new 'recurse' parameter. This also fixes what seems like an oversight of 86f5759 that the recursion to partition triggers would only occur if EnableDisableTrigger() had actually changed the trigger. It is more apt to recurse to inspect partition triggers even if the parent's trigger didn't need to be changed: only then can we be certain that all descendants share the same state afterwards. Backpatch all the way back to 11, like bbb927b. Care is taken not to break ABI compatibility (and that no catversion bump is needed.) Co-authored-by: Amit Langote <amitlangote09@gmail.com> Reviewed-by: Dmitry Koval <d.koval@postgrespro.ru> Discussion: https://postgr.es/m/CA+HiwqG-cZT3XzGAnEgZQLoQbyfJApVwOTQaCaas1mhpf+4V5A@mail.gmail.com
1 parent 2e2f343 commit ce8e066

File tree

8 files changed

+135
-35
lines changed

8 files changed

+135
-35
lines changed

‎src/backend/commands/tablecmds.c

Lines changed: 44 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -467,7 +467,8 @@ static void ATExecSetRelOptions(Relation rel, List *defList,
467467
AlterTableType operation,
468468
LOCKMODE lockmode);
469469
static void ATExecEnableDisableTrigger(Relation rel, const char *trigname,
470-
char fires_when, bool skip_system, LOCKMODE lockmode);
470+
char fires_when, bool skip_system, bool recurse,
471+
LOCKMODE lockmode);
471472
static void ATExecEnableDisableRule(Relation rel, const char *rulename,
472473
char fires_when, LOCKMODE lockmode);
473474
static void ATPrepAddInherit(Relation child_rel);
@@ -3407,16 +3408,20 @@ AlterTableLookupRelation(AlterTableStmt *stmt, LOCKMODE lockmode)
34073408
* expressions that need to be evaluated with respect to the old table
34083409
* schema.
34093410
*
3410-
* ATRewriteCatalogs performs phase 2 for each affected table. (Note that
3411-
* phases 2 and 3 normally do no explicit recursion, since phase 1 already
3412-
* did it --- although some subcommands have to recurse in phase 2 instead.)
3411+
* ATRewriteCatalogs performs phase 2 for each affected table.
34133412
* Certain subcommands need to be performed before others to avoid
34143413
* unnecessary conflicts; for example, DROP COLUMN should come before
34153414
* ADD COLUMN. Therefore phase 1 divi 8000 des the subcommands into multiple
34163415
* lists, one for each logical "pass" of phase 2.
34173416
*
34183417
* ATRewriteTables performs phase 3 for those tables that need it.
34193418
*
3419+
* For most subcommand types, phases 2 and 3 do no explicit recursion,
3420+
* since phase 1 already does it. However, for certain subcommand types
3421+
* it is only possible to determine how to recurse at phase 2 time; for
3422+
* those cases, phase 1 sets the cmd->recurse flag (or, in some older coding,
3423+
* changes the command subtype of a "Recurse" variant XXX to be cleaned up.)
3424+
*
34203425
* Thanks to the magic of MVCC, an error anywhere along the way rolls back
34213426
* the whole operation; we don't have to do anything special to clean up.
34223427
*
@@ -3791,10 +3796,10 @@ ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd,
37913796
tab = ATGetQueueEntry(wqueue, rel);
37923797

37933798
/*
3794-
* Copy the original subcommand for each table. This avoids conflicts
3795-
* when different child tables need to make different parse
3796-
* transformations (for example, the same column may have different column
3797-
* numbers in different children).
3799+
* Copy the original subcommand for each table, so we can scribble on it.
3800+
* This avoids conflicts when different child tables need to make
3801+
* different parse transformations (for example, the same column may have
3802+
* different column numbers in different children).
37983803
*/
37993804
cmd = copyObject(cmd);
38003805

@@ -4038,8 +4043,9 @@ ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd,
40384043
case AT_DisableTrigAll:
40394044
case AT_DisableTrigUser:
40404045
ATSimplePermissions(rel, ATT_TABLE | ATT_FOREIGN_TABLE);
4041-
if (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
4042-
ATSimpleRecursion(wqueue, rel, cmd, recurse, lockmode);
4046+
/* Set up recursion for phase 2; no other prep needed */
4047+
if (recurse)
4048+
cmd->recurse = true;
40434049
pass = AT_PASS_MISC;
40444050
break;
40454051
case AT_EnableRule: /* ENABLE/DISABLE RULE variants */
@@ -4339,35 +4345,51 @@ ATExecCmd(List **wqueue, AlteredTableInfo *tab, Relation rel,
43394345
break;
43404346
case AT_EnableTrig: /* ENABLE TRIGGER name */
43414347
ATExecEnableDisableTrigger(rel, cmd->name,
4342-
TRIGGER_FIRES_ON_ORIGIN, false, lockmode);
4348+
TRIGGER_FIRES_ON_ORIGIN, false,
4349+
cmd->recurse,
4350+
lockmode);
43434351
break;
43444352
case AT_EnableAlwaysTrig: /* ENABLE ALWAYS TRIGGER name */
43454353
ATExecEnableDisableTrigger(rel, cmd->name,
4346-
TRIGGER_FIRES_ALWAYS, false, lockmode);
4354+
TRIGGER_FIRES_ALWAYS, false,
4355+
cmd->recurse,
4356+
lockmode);
43474357
break;
43484358
case AT_EnableReplicaTrig: /* ENABLE REPLICA TRIGGER name */
43494359
ATExecEnableDisableTrigger(rel, cmd->name,
4350-
TRIGGER_FIRES_ON_REPLICA, false, lockmode);
4360+
TRIGGER_FIRES_ON_REPLICA, false,
4361+
cmd->recurse,
4362+
lockmode);
43514363
break;
43524364
case AT_DisableTrig: /* DISABLE TRIGGER name */
43534365
ATExecEnableDisableTrigger(rel, cmd->name,
4354-
TRIGGER_DISABLED, false, lockmode);
4366+
TRIGGER_DISABLED, false,
4367+
cmd->recurse,
4368+
lockmode);
43554369
break;
43564370
case AT_EnableTrigAll: /* ENABLE TRIGGER ALL */
43574371
ATExecEnableDisableTrigger(rel, NULL,
4358-
TRIGGER_FIRES_ON_ORIGIN, false, lockmode);
4372+
TRIGGER_FIRES_ON_ORIGIN, false,
4373+
cmd->recurse,
4374+
lockmode);
43594375
break;
43604376
case AT_DisableTrigAll: /* DISABLE TRIGGER ALL */
43614377
ATExecEnableDisableTrigger(rel, NULL,
4362-
TRIGGER_DISABLED, false, lockmode);
4378+
TRIGGER_DISABLED, false,
4379+
cmd->recurse,
4380+
lockmode);
43634381
break;
43644382
case AT_EnableTrigUser: /* ENABLE TRIGGER USER */
43654383
ATExecEnableDisableTrigger(rel, NULL,
4366-
TRIGGER_FIRES_ON_ORIGIN, true, lockmode);
4384+
TRIGGER_FIRES_ON_ORIGIN, true,
4385+
cmd->recurse,
4386+
lockmode);
43674387
break;
43684388
case AT_DisableTrigUser: /* DISABLE TRIGGER USER */
43694389
ATExecEnableDisableTrigger(rel, NULL,
4370-
TRIGGER_DISABLED, true, lockmode);
4390+
TRIGGER_DISABLED, true,
4391+
cmd->recurse,
4392+
lockmode);
43714393
break;
43724394

43734395
case AT_EnableRule: /* ENABLE RULE name */
@@ -12325,9 +12347,11 @@ copy_relation_data(SMgrRelation src, SMgrRelation dst,
1232512347
*/
1232612348
static void
1232712349
ATExecEnableDisableTrigger(Relation rel, const char *trigname,
12328-
char fires_when, bool skip_system, LOCKMODE lockmode)
12350+
char fires_when, bool skip_system, bool recurse,
12351+
LOCKMODE lockmode)
1232912352
{
12330-
EnableDisableTrigger(rel, trigname, fires_when, skip_system, lockmode);
12353+
EnableDisableTriggerNew(rel, trigname, fires_when, skip_system, recurse,
12354+
lockmode);
1233112355
}
1233212356

1233312357
/*

‎src/backend/commands/trigger.c

Lines changed: 45 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1786,14 +1786,16 @@ renametrig(RenameStmt *stmt)
17861786
* enablement/disablement, this also defines when the trigger
17871787
* should be fired in session replication roles.
17881788
* skip_system: if true, skip "system" triggers (constraint triggers)
1789+
* recurse: if true, recurse to partitions
17891790
*
17901791
* Caller should have checked permissions for the table; here we also
17911792
* enforce that superuser privilege is required to alter the state of
17921793
* system triggers
17931794
*/
17941795
void
1795-
EnableDisableTrigger(Relation rel, const char *tgname,
1796-
char fires_when, bool skip_system, LOCKMODE lockmode)
1796+
EnableDisableTriggerNew(Relation rel, const char *tgname,
1797+
char fires_when, bool skip_system, bool recurse,
1798+
LOCKMODE lockmode)
17971799
{
17981800
Relation tgrel;
17991801
int nkeys;
@@ -1859,6 +1861,34 @@ EnableDisableTrigger(Relation rel, const char *tgname,
18591861
changed = true;
18601862
}
18611863

1864+
/*
1865+
* When altering FOR EACH ROW triggers on a partitioned table, do the
1866+
* same on the partitions as well, unless ONLY is specified.
1867+
*
1868+
* Note that we recurse even if we didn't change the trigger above,
1869+
* because the partitions' copy of the trigger may have a different
1870+
* value of tgenabled than the parent's trigger and thus might need to
1871+
* be changed.
1872+
*/
1873+
if (recurse &&
1874+
rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE &&
1875+
(TRIGGER_FOR_ROW(oldtrig->tgtype)))
1876+
{
1877+
PartitionDesc partdesc = RelationGetPartitionDesc(rel);
1878+
int i;
1879+
1880+
for (i = 0; i < partdesc->nparts; i++)
1881+
{
1882+
Relation part;
1883+
1884+
part = heap_open(partdesc->oids[i], lockmode);
1885+
EnableDisableTriggerNew(part, NameStr(oldtrig->tgname),
1886+
fires_when, skip_system, recurse,
1887+
lockmode);
1888+
heap_close(part, NoLock); /* keep lock till commit */
1889+
}
1890+
}
1891+
18621892
InvokeObjectPostAlterHook(TriggerRelationId,
18631893
HeapTupleGetOid(tuple), 0);
18641894
}
@@ -1882,6 +1912,19 @@ EnableDisableTrigger(Relation rel, const char *tgname,
18821912
CacheInvalidateRelcache(rel);
18831913
}
18841914

1915+
/*
1916+
* ABI-compatible wrapper for the above. To keep as close possible to the old
1917+
* behavior, this never recurses. Do not call this function in new code.
1918+
*/
1919+
void
1920+
EnableDisableTrigger(Relation rel, const char *tgname,
1921+
char fires_when, bool skip_system,
1922+
LOCKMODE lockmode)
1923+
{
1924+
EnableDisableTriggerNew(rel, tgname, fires_when, skip_system,
1925+
true, lockmode);
1926+
}
1927+
18851928

18861929
/*
18871930
* Build trigger data to attach to the given relcache entry.

‎src/backend/nodes/copyfuncs.c

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3150,6 +3150,7 @@ _copyAlterTableCmd(const AlterTableCmd *from)
31503150
COPY_NODE_FIELD(def);
31513151
COPY_SCALAR_FIELD(behavior);
31523152
COPY_SCALAR_FIELD(missing_ok);
3153+
COPY_SCALAR_FIELD(recurse);
31533154

31543155
return newnode;
31553156
}

‎src/backend/nodes/equalfuncs.c

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1094,6 +1094,7 @@ _equalAlterTableCmd(const AlterTableCmd *a, const AlterTableCmd *b)
10941094
COMPARE_NODE_FIELD(def);
10951095
COMPARE_SCALAR_FIELD(behavior);
10961096
COMPARE_SCALAR_FIELD(missing_ok);
1097+
COMPARE_SCALAR_FIELD(recurse);
10971098

10981099
return true;
10991100
}

‎src/include/commands/trigger.h

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -172,8 +172,11 @@ extern Oid get_trigger_oid(Oid relid, const char *name, bool missing_ok);
172172

173173
extern ObjectAddress renametrig(RenameStmt *stmt);
174174

175+
extern void EnableDisableTriggerNew(Relation rel, const char *tgname,
176+
char fires_when, bool skip_system, bool recurse,
177+
LOCKMODE lockmode);
175178
extern void EnableDisableTrigger(Relation rel, const char *tgname,
176-
char fires_when, bool skip_system, LOCKMODE lockmode);
179+
char fires_when, bool skip_system, LOCKMODE lockmode);
177180

178181
extern void RelationBuildTriggers(Relation relation);
179182

‎src/include/nodes/parsenodes.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1816,6 +1816,7 @@ typedef struct AlterTableCmd /* one subcommand of an ALTER TABLE */
18161816
* constraint, or parent table */
18171817
DropBehavior behavior; /* RESTRICT or CASCADE for DROP cases */
18181818
bool missing_ok; /* skip error if missing? */
1819+
bool recurse; /* exec-time recursion */
18191820
} AlterTableCmd;
18201821

18211822

‎src/test/regress/expected/triggers.out

Lines changed: 29 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -2476,24 +2476,42 @@ create table parent (a int) partition by list (a);
24762476
create table child1 partition of parent for values in (1);
24772477
create trigger tg after insert on parent
24782478
for each row execute procedure trig_nothing();
2479+
create trigger tg_stmt after insert on parent
2480+
for statement execute procedure trig_nothing();
24792481
select tgrelid::regclass, tgname, tgenabled from pg_trigger
24802482
where tgrelid in ('parent'::regclass, 'child1'::regclass)
24812483
order by tgrelid::regclass::text;
2482-
tgrelid | tgname | tgenabled
2483-
---------+--------+-----------
2484-
child1 | tg | O
2485-
parent | tg | O
2486-
(2 rows)
2484+
tgrelid | tgname | tgenabled
2485+
---------+---------+-----------
2486+
child1 | tg | O
2487+
parent | tg | O
2488+
parent | tg_stmt | O
2489+
(3 rows)
24872490

2488-
alter table only parent enable always trigger tg;
2491+
alter table only parent enable always trigger tg; -- no recursion because ONLY
2492+
alter table parent enable always trigger tg_stmt; -- no recursion because statement trigger
24892493
select tgrelid::regclass, tgname, tgenabled from pg_trigger
24902494
where tgrelid in ('parent'::regclass, 'child1'::regclass)
24912495
order by tgrelid::regclass::text;
2492-
tgrelid | tgname | tgenabled
2493-
---------+--------+-----------
2494-
child1 | tg | O
2495-
parent | tg | A
2496-
(2 rows)
2496+
tgrelid | tgname | tgenabled
2497+
---------+---------+-----------
2498+
child1 | tg | O
2499+
parent | tg | A
2500+
parent | tg_stmt | A
2501+
(3 rows)
2502+
2503+
-- The following is a no-op for the parent trigger but not so
2504+
-- for the child trigger, so recursion should be applied.
2505+
alter table parent enable always trigger tg;
2506+
select tgrelid::regclass, tgname, tgenabled from pg_trigger
2507+
where tgrelid in ('parent'::regclass, 'child1'::regclass)
2508+
order by tgrelid::regclass::text;
2509+
tgrelid | tgname | tgenabled
2510+
---------+---------+-----------
2511+
child1 | tg | A
2512+
parent | tg | A
2513+
parent | tg_stmt | A
2514+
(3 rows)
24972515

24982516
drop table parent, child1;
24992517
-- Verify that firing state propagates correctly

‎src/test/regress/sql/triggers.sql

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1728,10 +1728,19 @@ create table parent (a int) partition by list (a);
17281728
create table child1 partition of parent for values in (1);
17291729
create trigger tg after insert on parent
17301730
for each row execute procedure trig_nothing();
1731+
create trigger tg_stmt after insert on parent
1732+
for statement execute procedure trig_nothing();
17311733
select tgrelid::regclass, tgname, tgenabled from pg_trigger
17321734
where tgrelid in ('parent'::regclass, 'child1'::regclass)
17331735
order by tgrelid::regclass::text;
1734-
alter table only parent enable always trigger tg;
1736+
alter table only parent enable always trigger tg; -- no recursion because ONLY
1737+
alter table parent enable always trigger tg_stmt; -- no recursion because statement trigger
1738+
select tgrelid::regclass, tgname, tgenabled from pg_trigger
1739+
where tgrelid in ('parent'::regclass, 'child1'::regclass)
1740+
order by tgrelid::regclass::text;
1741+
-- The following is a no-op for the parent trigger but not so
1742+
-- for the child trigger, so recursion should be applied.
1743+
alter table parent enable always trigger tg;
17351744
select tgrelid::regclass, tgname, tgenabled from pg_trigger
17361745
where tgrelid in ('parent'::regclass, 'child1'::regclass)
17371746
order by tgrelid::regclass::text;

0 commit comments

Comments
 (0)
0