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

Commit ec0925c

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 d2e1508 commit ec0925c

File tree

6 files changed

+117
-34
lines changed

6 files changed

+117
-34
lines changed

‎src/backend/commands/tablecmds.c

Lines changed: 44 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -580,7 +580,8 @@ static void ATExecSetRelOptions(Relation rel, List *defList,
580580
AlterTableType operation,
581581
LOCKMODE lockmode);
582582
static void ATExecEnableDisableTrigger(Relation rel, const char *trigname,
583-
char fires_when, bool skip_system, LOCKMODE lockmode);
583+
char fires_when, bool skip_system, bool recurse,
584+
LOCKMODE lockmode);
584585
static void ATExecEnableDisableRule(Relation rel, const char *rulename,
585586
char fires_when, LOCKMODE lockmode);
586587
static void ATPrepAddInherit(Relation child_rel);
@@ -4057,16 +4058,20 @@ AlterTableLookupRelation(AlterTableStmt *stmt, LOCKMODE lockmode)
40574058
* be done in this phase. Generally, this phase acquires table locks,
40584059
* checks permissions and relkind, and recurses to find child tables.
40594060
*
4060-
* ATRewriteCatalogs performs phase 2 for each affected table. (Note that
4061-
* phases 2 and 3 normally do no explicit recursion, since phase 1 already
4062-
* did it --- although some subcommands have to recurse in phase 2 instead.)
4061+
* ATRewriteCatalogs performs phase 2 for each affected table.
40634062
* Certain subcommands need to be performed before others to avoid
40644063
* unnecessary conflicts; for example, DROP COLUMN should come before
40654064
* ADD COLUMN. Therefore phase 1 divides the subcommands into multiple
40664065
* lists, one for each logical "pass" of phase 2.
40674066
*
40684067
* ATRewriteTables performs phase 3 for those tables that need it.
40694068
*
4069+
* For most subcommand types, phases 2 and 3 do no explicit recursion,
4070+
* since phase 1 already does it. However, for certain subcommand types
4071+
* it is only possible to determine how to recurse at phase 2 time; for
4072+
* those cases, phase 1 sets the cmd->recurse flag (or, in some older coding,
4073+
* changes the command subtype of a "Recurse" variant XXX to be cleaned up.)
4074+
*
40704075
* Thanks to the magic of MVCC, an error anywhere along the way rolls back
40714076
* the whole operation; we don't have to do anything special to clean up.
40724077
*
@@ -4487,10 +4492,10 @@ ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd,
44874492
errhint("Use ALTER TABLE ... DETACH PARTITION ... FINALIZE to complete the pending detach operation."));
44884493

44894494
/*
4490-
* Copy the original subcommand for each table. This avoids conflicts
4491-
* when different child tables need to make different parse
4492-
* transformations (for example, the same column may have different column
4493-
* numbers in different children).
44 10000 95+
* Copy the original subcommand for each table, so we can scribble on it.
4496+
* This avoids conflicts when different child tables need to make
4497+
* different parse transformations (for example, the same column may have
4498+
* different column numbers in different children).
44944499
*/
44954500
cmd = copyObject(cmd);
44964501

@@ -4777,8 +4782,9 @@ ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd,
47774782
case AT_DisableTrigAll:
47784783
case AT_DisableTrigUser:
47794784
ATSimplePermissions(cmd->subtype, rel, ATT_TABLE | ATT_FOREIGN_TABLE);
4780-
if (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
4781-
ATSimpleRecursion(wqueue, rel, cmd, recurse, lockmode, context);
4785+
/* Set up recursion for phase 2; no other prep needed */
4786+
if (recurse)
4787+
cmd->recurse = true;
47824788
pass = AT_PASS_MISC;
47834789
break;
47844790
case AT_EnableRule: /* ENABLE/DISABLE RULE variants */
@@ -5119,35 +5125,51 @@ ATExecCmd(List **wqueue, AlteredTableInfo *tab,
51195125
break;
51205126
case AT_EnableTrig: /* ENABLE TRIGGER name */
51215127
ATExecEnableDisableTrigger(rel, cmd->name,
5122-
TRIGGER_FIRES_ON_ORIGIN, false, lockmode);
5128+
TRIGGER_FIRES_ON_ORIGIN, false,
5129+
cmd->recurse,
5130+
lockmode);
51235131
break;
51245132
case AT_EnableAlwaysTrig: /* ENABLE ALWAYS TRIGGER name */
51255133
ATExecEnableDisableTrigger(rel, cmd->name,
5126-
TRIGGER_FIRES_ALWAYS, false, lockmode);
5134+
TRIGGER_FIRES_ALWAYS, false,
5135+
cmd->recurse,
5136+
lockmode);
51275137
break;
51285138
case AT_EnableReplicaTrig: /* ENABLE REPLICA TRIGGER name */
51295139
ATExecEnableDisableTrigger(rel, cmd->name,
5130-
TRIGGER_FIRES_ON_REPLICA, false, lockmode);
5140+
TRIGGER_FIRES_ON_REPLICA, false,
5141+
cmd->recurse,
5142+
lockmode);
51315143
break;
51325144
case AT_DisableTrig: /* DISABLE TRIGGER name */
51335145
ATExecEnableDisableTrigger(rel, cmd->name,
5134-
TRIGGER_DISABLED, false, lockmode);
5146+
TRIGGER_DISABLED, false,
5147+
cmd->recurse,
5148+
lockmode);
51355149
break;
51365150
case AT_EnableTrigAll: /* ENABLE TRIGGER ALL */
51375151
ATExecEnableDisableTrigger(rel, NULL,
5138-
TRIGGER_FIRES_ON_ORIGIN, false, lockmode);
5152+
TRIGGER_FIRES_ON_ORIGIN, false,
5153+
cmd->recurse,
5154+
lockmode);
51395155
break;
51405156
case AT_DisableTrigAll: /* DISABLE TRIGGER ALL */
51415157
ATExecEnableDisableTrigger(rel, NULL,
5142-
TRIGGER_DISABLED, false, lockmode);
5158+
TRIGGER_DISABLED, false,
5159+
cmd->recurse,
5160+
lockmode);
51435161
break;
51445162
case AT_EnableTrigUser: /* ENABLE TRIGGER USER */
51455163
ATExecEnableDisableTrigger(rel, NULL,
5146-
TRIGGER_FIRES_ON_ORIGIN, true, lockmode);
5164+
TRIGGER_FIRES_ON_ORIGIN, true,
5165+
cmd->recurse,
5166+
lockmode);
51475167
break;
51485168
case AT_DisableTrigUser: /* DISABLE TRIGGER USER */
51495169
ATExecEnableDisableTrigger(rel, NULL,
5150-
TRIGGER_DISABLED, true, lockmode);
5170+
TRIGGER_DISABLED, true,
5171+
cmd->recurse,
5172+
lockmode);
51515173
break;
51525174

51535175
case AT_EnableRule: /* ENABLE RULE name */
@@ -14660,9 +14682,11 @@ index_copy_data(Relation rel, RelFileLocator newrlocator)
1466014682
*/
1466114683
static void
1466214684
ATExecEnableDisableTrigger(Relation rel, const char *trigname,
14663-
char fires_when, bool skip_system, LOCKMODE lockmode)
14685+
char fires_when, bool skip_system, bool recurse,
14686+
LOCKMODE lockmode)
1466414687
{
14665-
EnableDisableTrigger(rel, trigname, fires_when, skip_system, lockmode);
14688+
EnableDisableTrigger(rel, trigname, fires_when, skip_system, recurse,
14689+
lockmode);
1466614690
}
1466714691

1466814692
/*

‎src/backend/commands/trigger.c

Lines changed: 31 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1752,14 +1752,16 @@ renametrig_partition(Relation tgrel, Oid partitionId, Oid parentTriggerOid,
17521752
* enablement/disablement, this also defines when the trigger
17531753
* should be fired in session replication roles.
17541754
* skip_system: if true, skip "system" triggers (constraint triggers)
1755+
* recurse: if true, recurse to partitions
17551756
*
17561757
* Caller should have checked permissions for the table; here we also
17571758
* enforce that superuser privilege is required to alter the state of
17581759
* system triggers
17591760
*/
17601761
void
17611762
EnableDisableTrigger(Relation rel, const char *tgname,
1762-
char fires_when, bool skip_system, LOCKMODE lockmode)
1763+
char fires_when, bool skip_system, bool recurse,
1764+
LOCKMODE lockmode)
17631765
{
17641766
Relation tgrel;
17651767
int nkeys;
@@ -1825,6 +1827,34 @@ EnableDisableTrigger(Relation rel, const char *tgname,
18251827
changed = true;
18261828
}
18271829

1830+
/*
1831+
* When altering FOR EACH ROW triggers on a partitioned table, do the
1832+
* same on the partitions as well, unless ONLY is specified.
1833+
*
1834+
* Note that we recurse even if we didn't change the trigger above,
1835+
* because the partitions' copy of the trigger may have a different
1836+
* value of tgenabled than the parent's trigger and thus might need to
1837+
* be changed.
1838+
*/
1839+
if (recurse &&
1840+
rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE &&
1841+
(TRIGGER_FOR_ROW(oldtrig->tgtype)))
1842+
{
1843+
PartitionDesc partdesc = RelationGetPartitionDesc(rel, true);
1844+
int i;
1845+
1846+
for (i = 0; i < partdesc->nparts; i++)
1847+
{
1848+
Relation part;
1849+
1850+
part = relation_open(partdesc->oids[i], lockmode);
1851+
EnableDisableTrigger(part, NameStr(oldtrig->tgname),
1852+
fires_when, skip_system, recurse,
1853+
lockmode);
1854+
table_close(part, NoLock); /* keep lock till commit */
1855+
}
1856+
}
1857+
18281858
InvokeObjectPostAlterHook(TriggerRelationId,
18291859
oldtrig->oid, 0);
18301860
}

‎src/include/commands/trigger.h

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -171,7 +171,8 @@ extern Oid get_trigger_oid(Oid relid, const char *name, bool missing_ok);
171171
extern ObjectAddress renametrig(RenameStmt *stmt);
172172

173173
extern void EnableDisableTrigger(Relation rel, const char *tgname,
174-
char fires_when, bool skip_system, LOCKMODE lockmode);
174+
char fires_when, bool skip_system, bool recurse,
175+
LOCKMODE lockmode);
175176

176177
extern void RelationBuildTriggers(Relation relation);
177178

‎src/include/nodes/parsenodes.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2328,6 +2328,7 @@ typedef struct AlterTableCmd /* one subcommand of an ALTER TABLE */
23282328
* constraint, or parent table */
23292329
DropBehavior behavior; /* RESTRICT or CASCADE for DROP cases */
23302330
bool missing_ok; /* skip error if missing? */
2331+
bool recurse; /* exec-time recursion */
23312332
} AlterTableCmd;
23322333

23332334

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

Lines changed: 29 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -2681,24 +2681,42 @@ create table parent (a int) partition by list (a);
26812681
create table child1 partition of parent for values in (1);
26822682
create trigger tg after insert on parent
26832683
for each row execute procedure trig_nothing();
2684+
create trigger tg_stmt after insert on parent
2685+
for statement execute procedure trig_nothing();
26842686
select tgrelid::regclass, tgname, tgenabled from pg_trigger
26852687
where tgrelid in ('parent'::regclass, 'child1'::regclass)
26862688
order by tgrelid::regclass::text;
2687-
tgrelid | tgname | tgenabled
2688-
---------+--------+-----------
2689-
child1 | tg | O
2690-
parent | tg | O
2691-
(2 rows)
2689+
tgrelid | tgname | tgenabled
2690+
---------+---------+-----------
2691+
child1 | tg | O
2692+
parent | tg | O
2693+
parent | tg_stmt | O
2694+
(3 rows)
26922695

2693-
alter table only parent enable always trigger tg;
2696+
alter table only parent enable always trigger tg; -- no recursion because ONLY
2697+
alter table parent enable always trigger tg_stmt; -- no recursion because statement trigger
26942698
select tgrelid::regclass, tgname, tgenabled from pg_trigger
26952699
where tgrelid in ('parent'::regclass, 'child1'::regclass)
26962700
order by tgrelid::regclass::text;
2697-
tgrelid | tgname | tgenabled
2698-
---------+--------+-----------
2699-
child1 | tg | O
2700-
parent | tg | A
2701-
(2 rows)
2701+
tgrelid | tgname | tgenabled
2702+
---------+---------+-----------
2703+
child1 | tg | O
2704+
parent | tg | A
2705+
parent | tg_stmt | A
2706+
(3 rows)
2707+
2708+
-- The following is a no-op for the parent trigger but not so
2709+
-- for the child trigger, so recursion should be applied.
2710+
alter table parent enable always trigger tg;
2711+
select tgrelid::regclass, tgname, tgenabled from pg_trigger
2712+
where tgrelid in ('parent'::regclass, 'child1'::regclass)
2713+
order by tgrelid::regclass::text;
2714+
tgrelid | tgname | tgenabled
2715+
---------+---------+-----------
2716+
child1 | tg | A
2717+
parent | tg | A
2718+
parent | tg_stmt | A
2719+
(3 rows)
27022720

27032721
drop table parent, child1;
27042722
-- Verify that firing state propagates correctly on creation, too

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

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1865,10 +1865,19 @@ create table parent (a int) partition by list (a);
18651865
create table child1 partition of parent for values in (1);
18661866
create trigger tg after insert on parent
18671867
for each row execute procedure trig_nothing();
1868+
create trigger tg_stmt after insert on parent
1869+
for statement execute procedure trig_nothing();
18681870
select tgrelid::regclass, tgname, tgenabled from pg_trigger
18691871
where tgrelid in ('parent'::regclass, 'child1'::regclass)
18701872
order by tgrelid::regclass::text;
1871-
alter table only parent enable always trigger tg;
1873+
alter table only parent enable always trigger tg; -- no recursion because ONLY
1874+
alter table parent enable always trigger tg_stmt; -- no recursion because statement trigger
1875+
select tgrelid::regclass, tgname, tgenabled from pg_trigger
1876+
where tgrelid in ('parent'::regclass, 'child1'::regclass)
1877+
order by tgrelid::regclass::text;
1878+
-- The following is a no-op for the parent trigger but not so
1879+
-- for the child trigger, so recursion should be applied.
1880+
alter table parent enable always trigger tg;
18721881
select tgrelid::regclass, tgname, tgenabled from pg_trigger
18731882
where tgrelid in ('parent'::regclass, 'child1'::regclass)
18741883
order by tgrelid::regclass::text;

0 commit comments

Comments
 (0)
0