Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Commit6e7b372

Browse files
alvherreamitlan
andcommitted
Fix ENABLE/DISABLE TRIGGER to handle recursion correctly
Using ATSimpleRecursion() in ATPrepCmd() to do so asbbb927b did isnot correct, because ATPrepCmd() can't distinguish between triggers thatmay be cloned and those that may not, so would wrongly try to recursefor the latter category of triggers.So this commit restores the code in EnableDisableTrigger() that86f5759 had added to do the recursion, which would do it only fortriggers that may be cloned, that is, row-level triggers. This alsochanges tablecmds.c such that ATExecCmd() is able to pass the value ofONLY flag down to EnableDisableTrigger() using its new 'recurse'parameter.This also fixes what seems like an oversight of86f5759 that therecursion to partition triggers would only occur if EnableDisableTrigger()had actually changed the trigger. It is more apt to recurse to inspectpartition triggers even if the parent's trigger didn't need to bechanged: only then can we be certain that all descendants share the samestate afterwards.Backpatch all the way back to 11, likebbb927b. Care is taken notto 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 parent963a6b6 commit6e7b372

File tree

8 files changed

+134
-34
lines changed

8 files changed

+134
-34
lines changed

‎src/backend/commands/tablecmds.c

Lines changed: 44 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -503,7 +503,8 @@ static void ATExecSetRelOptions(Relation rel, List *defList,
503503
AlterTableType operation,
504504
LOCKMODE lockmode);
505505
static void ATExecEnableDisableTrigger(Relation rel, const char *trigname,
506-
char fires_when, bool skip_system, LOCKMODE lockmode);
506+
char fires_when, bool skip_system, bool recurse,
507+
LOCKMODE lockmode);
507508
static void ATExecEnableDisableRule(Relation rel, const char *rulename,
508509
char fires_when, LOCKMODE lockmode);
509510
static void ATPrepAddInherit(Relation child_rel);
@@ -3693,16 +3694,20 @@ AlterTableLookupRelation(AlterTableStmt *stmt, LOCKMODE lockmode)
36933694
* expressions that need to be evaluated with respect to the old table
36943695
* schema.
36953696
*
3696-
* ATRewriteCatalogs performs phase 2 for each affected table. (Note that
3697-
* phases 2 and 3 normally do no explicit recursion, since phase 1 already
3698-
* did it --- although some subcommands have to recurse in phase 2 instead.)
3697+
* ATRewriteCatalogs performs phase 2 for each affected table.
36993698
* Certain subcommands need to be performed before others to avoid
37003699
* unnecessary conflicts; for example, DROP COLUMN should come before
37013700
* ADD COLUMN. Therefore phase 1 divides the subcommands into multiple
37023701
* lists, one for each logical "pass" of phase 2.
37033702
*
37043703
* ATRewriteTables performs phase 3 for those tables that need it.
37053704
*
3705+
* For most subcommand types, phases 2 and 3 do no explicit recursion,
3706+
* since phase 1 already does it. However, for certain subcommand types
3707+
* it is only possible to determine how to recurse at phase 2 time; for
3708+
* those cases, phase 1 sets the cmd->recurse flag (or, in some older coding,
3709+
* changes the command subtype of a "Recurse" variant XXX to be cleaned up.)
3710+
*
37063711
* Thanks to the magic of MVCC, an error anywhere along the way rolls back
37073712
* the whole operation; we don't have to do anything special to clean up.
37083713
*
@@ -4090,10 +4095,10 @@ ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd,
40904095
tab = ATGetQueueEntry(wqueue, rel);
40914096

40924097
/*
4093-
* Copy the original subcommand for each table. This avoids conflicts
4094-
* when different child tables need to make different parse
4095-
* transformations (for example, the same column may have different column
4096-
* numbers in different children).
4098+
* Copy the original subcommand for each table, so we can scribble on it.
4099+
*This avoids conflictswhen different child tables need to make
4100+
*different parsetransformations (for example, the same column may have
4101+
*different columnnumbers in different children).
40974102
*/
40984103
cmd = copyObject(cmd);
40994104

@@ -4331,8 +4336,9 @@ ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd,
43314336
case AT_DisableTrigAll:
43324337
case AT_DisableTrigUser:
43334338
ATSimplePermissions(rel, ATT_TABLE | ATT_FOREIGN_TABLE);
4334-
if (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
4335-
ATSimpleRecursion(wqueue, rel, cmd, recurse, lockmode);
4339+
/* Set up recursion for phase 2; no other prep needed */
4340+
if (recurse)
4341+
cmd->recurse = true;
43364342
pass = AT_PASS_MISC;
43374343
break;
43384344
case AT_EnableRule:/* ENABLE/DISABLE RULE variants */
@@ -4623,35 +4629,51 @@ ATExecCmd(List **wqueue, AlteredTableInfo *tab, Relation rel,
46234629
break;
46244630
case AT_EnableTrig:/* ENABLE TRIGGER name */
46254631
ATExecEnableDisableTrigger(rel, cmd->name,
4626-
TRIGGER_FIRES_ON_ORIGIN, false, lockmode);
4632+
TRIGGER_FIRES_ON_ORIGIN, false,
4633+
cmd->recurse,
4634+
lockmode);
46274635
break;
46284636
case AT_EnableAlwaysTrig:/* ENABLE ALWAYS TRIGGER name */
46294637
ATExecEnableDisableTrigger(rel, cmd->name,
4630-
TRIGGER_FIRES_ALWAYS, false, lockmode);
4638+
TRIGGER_FIRES_ALWAYS, false,
4639+
cmd->recurse,
4640+
lockmode);
46314641
break;
46324642
case AT_EnableReplicaTrig:/* ENABLE REPLICA TRIGGER name */
46334643
ATExecEnableDisableTrigger(rel, cmd->name,
4634-
TRIGGER_FIRES_ON_REPLICA, false, lockmode);
4644+
TRIGGER_FIRES_ON_REPLICA, false,
4645+
cmd->recurse,
4646+
lockmode);
46354647
break;
46364648
case AT_DisableTrig:/* DISABLE TRIGGER name */
46374649
ATExecEnableDisableTrigger(rel, cmd->name,
4638-
TRIGGER_DISABLED, false, lockmode);
4650+
TRIGGER_DISABLED, false,
4651+
cmd->recurse,
4652+
lockmode);
46394653
break;
46404654
case AT_EnableTrigAll:/* ENABLE TRIGGER ALL */
46414655
ATExecEnableDisableTrigger(rel, NULL,
4642-
TRIGGER_FIRES_ON_ORIGIN, false, lockmode);
4656+
TRIGGER_FIRES_ON_ORIGIN, false,
4657+
cmd->recurse,
4658+
lockmode);
46434659
break;
46444660
case AT_DisableTrigAll: /* DISABLE TRIGGER ALL */
46454661
ATExecEnableDisableTrigger(rel, NULL,
4646-
TRIGGER_DISABLED, false, lockmode);
4662+
TRIGGER_DISABLED, false,
4663+
cmd->recurse,
4664+
lockmode);
46474665
break;
46484666
case AT_EnableTrigUser: /* ENABLE TRIGGER USER */
46494667
ATExecEnableDisableTrigger(rel, NULL,
4650-
TRIGGER_FIRES_ON_ORIGIN, true, lockmode);
4668+
TRIGGER_FIRES_ON_ORIGIN, true,
4669+
cmd->recurse,
4670+
lockmode);
46514671
break;
46524672
case AT_DisableTrigUser:/* DISABLE TRIGGER USER */
46534673
ATExecEnableDisableTrigger(rel, NULL,
4654-
TRIGGER_DISABLED, true, lockmode);
4674+
TRIGGER_DISABLED, true,
4675+
cmd->recurse,
4676+
lockmode);
46554677
break;
46564678

46574679
case AT_EnableRule:/* ENABLE RULE name */
@@ -13321,9 +13343,11 @@ index_copy_data(Relation rel, RelFileNode newrnode)
1332113343
*/
1332213344
static void
1332313345
ATExecEnableDisableTrigger(Relation rel, const char *trigname,
13324-
char fires_when, bool skip_system, LOCKMODE lockmode)
13346+
char fires_when, bool skip_system, bool recurse,
13347+
LOCKMODE lockmode)
1332513348
{
13326-
EnableDisableTrigger(rel, trigname, fires_when, skip_system, lockmode);
13349+
EnableDisableTriggerNew(rel, trigname, fires_when, skip_system, recurse,
13350+
lockmode);
1332713351
}
1332813352

1332913353
/*

‎src/backend/commands/trigger.c

Lines changed: 45 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1806,14 +1806,16 @@ renametrig(RenameStmt *stmt)
18061806
* enablement/disablement, this also defines when the trigger
18071807
* should be fired in session replication roles.
18081808
* skip_system: if true, skip "system" triggers (constraint triggers)
1809+
* recurse: if true, recurse to partitions
18091810
*
18101811
* Caller should have checked permissions for the table; here we also
18111812
* enforce that superuser privilege is required to alter the state of
18121813
* system triggers
18131814
*/
18141815
void
1815-
EnableDisableTrigger(Relationrel,constchar*tgname,
1816-
charfires_when,boolskip_system,LOCKMODElockmode)
1816+
EnableDisableTriggerNew(Relationrel,constchar*tgname,
1817+
charfires_when,boolskip_system,boolrecurse,
1818+
LOCKMODElockmode)
18171819
{
18181820
Relationtgrel;
18191821
intnkeys;
@@ -1879,6 +1881,34 @@ EnableDisableTrigger(Relation rel, const char *tgname,
18791881
changed= true;
18801882
}
18811883

1884+
/*
1885+
* When altering FOR EACH ROW triggers on a partitioned table, do the
1886+
* same on the partitions as well, unless ONLY is specified.
1887+
*
1888+
* Note that we recurse even if we didn't change the trigger above,
1889+
* because the partitions' copy of the trigger may have a different
1890+
* value of tgenabled than the parent's trigger and thus might need to
1891+
* be changed.
1892+
*/
1893+
if (recurse&&
1894+
rel->rd_rel->relkind==RELKIND_PARTITIONED_TABLE&&
1895+
(TRIGGER_FOR_ROW(oldtrig->tgtype)))
1896+
{
1897+
PartitionDescpartdesc=RelationGetPartitionDesc(rel);
1898+
inti;
1899+
1900+
for (i=0;i<partdesc->nparts;i++)
1901+
{
1902+
Relationpart;
1903+
1904+
part=relation_open(partdesc->oids[i],lockmode);
1905+
EnableDisableTriggerNew(part,NameStr(oldtrig->tgname),
1906+
fires_when,skip_system,recurse,
1907+
lockmode);
1908+
table_close(part,NoLock);/* keep lock till commit */
1909+
}
1910+
}
1911+
18821912
InvokeObjectPostAlterHook(TriggerRelationId,
18831913
oldtrig->oid,0);
18841914
}
@@ -1902,6 +1932,19 @@ EnableDisableTrigger(Relation rel, const char *tgname,
19021932
CacheInvalidateRelcache(rel);
19031933
}
19041934

1935+
/*
1936+
* ABI-compatible wrapper for the above. To keep as close possible to the old
1937+
* behavior, this never recurses. Do not call this function in new code.
1938+
*/
1939+
void
1940+
EnableDisableTrigger(Relationrel,constchar*tgname,
1941+
charfires_when,boolskip_system,
1942+
LOCKMODElockmode)
1943+
{
1944+
EnableDisableTriggerNew(rel,tgname,fires_when,skip_system,
1945+
true,lockmode);
1946+
}
1947+
19051948

19061949
/*
19071950
* 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
@@ -3160,6 +3160,7 @@ _copyAlterTableCmd(const AlterTableCmd *from)
31603160
COPY_NODE_FIELD(def);
31613161
COPY_SCALAR_FIELD(behavior);
31623162
COPY_SCALAR_FIELD(missing_ok);
3163+
COPY_SCALAR_FIELD(recurse);
31633164

31643165
returnnewnode;
31653166
}

‎src/backend/nodes/equalfuncs.c

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

10991100
return true;
11001101
}

‎src/include/commands/trigger.h

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -172,6 +172,9 @@ extern Oidget_trigger_oid(Oid relid, const char *name, bool missing_ok);
172172

173173
externObjectAddressrenametrig(RenameStmt*stmt);
174174

175+
externvoidEnableDisableTriggerNew(Relationrel,constchar*tgname,
176+
charfires_when,boolskip_system,boolrecurse,
177+
LOCKMODElockmode);
175178
externvoidEnableDisableTrigger(Relationrel,constchar*tgname,
176179
charfires_when,boolskip_system,LOCKMODElockmode);
177180

‎src/include/nodes/parsenodes.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1853,6 +1853,7 @@ typedef struct AlterTableCmd/* one subcommand of an ALTER TABLE */
18531853
* constraint, or parent table */
18541854
DropBehaviorbehavior;/* RESTRICT or CASCADE for DROP cases */
18551855
boolmissing_ok;/* skip error if missing? */
1856+
boolrecurse;/* exec-time recursion */
18561857
}AlterTableCmd;
18571858

18581859

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

Lines changed: 29 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -2532,24 +2532,42 @@ create table parent (a int) partition by list (a);
25322532
create table child1 partition of parent for values in (1);
25332533
create trigger tg after insert on parent
25342534
for each row execute procedure trig_nothing();
2535+
create trigger tg_stmt after insert on parent
2536+
for statement execute procedure trig_nothing();
25352537
select tgrelid::regclass, tgname, tgenabled from pg_trigger
25362538
where tgrelid in ('parent'::regclass, 'child1'::regclass)
25372539
order by tgrelid::regclass::text;
2538-
tgrelid | tgname | tgenabled
2539-
---------+--------+-----------
2540-
child1 | tg | O
2541-
parent | tg | O
2542-
(2 rows)
2540+
tgrelid | tgname | tgenabled
2541+
---------+---------+-----------
2542+
child1 | tg | O
2543+
parent | tg | O
2544+
parent | tg_stmt | O
2545+
(3 rows)
25432546

2544-
alter table only parent enable always trigger tg;
2547+
alter table only parent enable always trigger tg;-- no recursion because ONLY
2548+
alter table parent enable always trigger tg_stmt;-- no recursion because statement trigger
25452549
select tgrelid::regclass, tgname, tgenabled from pg_trigger
25462550
where tgrelid in ('parent'::regclass, 'child1'::regclass)
25472551
order by tgrelid::regclass::text;
2548-
tgrelid | tgname | tgenabled
2549-
---------+--------+-----------
2550-
child1 | tg | O
2551-
parent | tg | A
2552-
(2 rows)
2552+
tgrelid | tgname | tgenabled
2553+
---------+---------+-----------
2554+
child1 | tg | O
2555+
parent | tg | A
2556+
parent | tg_stmt | A
2557+
(3 rows)
2558+
2559+
-- The following is a no-op for the parent trigger but not so
2560+
-- for the child trigger, so recursion should be applied.
2561+
alter table parent enable always trigger tg;
2562+
select tgrelid::regclass, tgname, tgenabled from pg_trigger
2563+
where tgrelid in ('parent'::regclass, 'child1'::regclass)
2564+
order by tgrelid::regclass::text;
2565+
tgrelid | tgname | tgenabled
2566+
---------+---------+-----------
2567+
child1 | tg | A
2568+
parent | tg | A
2569+
parent | tg_stmt | A
2570+
(3 rows)
25532571

25542572
drop table parent, child1;
25552573
-- 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
@@ -1750,10 +1750,19 @@ create table parent (a int) partition by list (a);
17501750
createtablechild1 partition of parent forvaluesin (1);
17511751
createtriggertg after inserton parent
17521752
for each row execute procedure trig_nothing();
1753+
createtriggertg_stmt after inserton parent
1754+
for statement execute procedure trig_nothing();
17531755
select tgrelid::regclass, tgname, tgenabledfrom pg_trigger
17541756
where tgrelidin ('parent'::regclass,'child1'::regclass)
17551757
order by tgrelid::regclass::text;
1756-
altertable only parent enable always trigger tg;
1758+
altertable only parent enable always trigger tg;-- no recursion because ONLY
1759+
altertable parent enable always trigger tg_stmt;-- no recursion because statement trigger
1760+
select tgrelid::regclass, tgname, tgenabledfrom pg_trigger
1761+
where tgrelidin ('parent'::regclass,'child1'::regclass)
1762+
order by tgrelid::regclass::text;
1763+
-- The following is a no-op for the parent trigger but not so
1764+
-- for the child trigger, so recursion should be applied.
1765+
altertable parent enable always trigger tg;
17571766
select tgrelid::regclass, tgname, tgenabledfrom pg_trigger
17581767
where tgrelidin ('parent'::regclass,'child1'::regclass)
17591768
order by tgrelid::regclass::text;

0 commit comments

Comments
 (0)
[8]ページ先頭
©2009-2025 Movatter.jp