Skip to content

Commit 3c43595

Browse files
tglsfdctglsfdc
committed
Quick-hack fix for foreign key cascade vs triggers with transition tables.
AFTER triggers using transition tables crashed if they were fired due to a foreign key ON CASCADE update. This is because ExecEndModifyTable flushes the transition tables, on the assumption that any trigger that could need them was already fired during ExecutorFinish. Normally that's true, because we don't allow transition-table-using triggers to be deferred. However, foreign key CASCADE updates force any triggers on the referencing table to be deferred to the outer query level, by means of the EXEC_FLAG_SKIP_TRIGGERS flag. I don't recall all the details of why it's like that and am pretty loath to redesign it right now. Instead, just teach ExecEndModifyTable to skip destroying the TransitionCaptureState when that flag is set. This will allow the transition table data to survive until end of the current subtransaction. This isn't a terribly satisfactory solution, because (1) we might be leaking the transition tables for much longer than really necessary, and (2) as things stand, an AFTER STATEMENT trigger will fire once per RI updating query, ie once per row updated or deleted in the referenced table. I suspect that is not per SQL spec. But redesigning this is a research project that we're certainly not going to get done for v10. So let's go with this hackish answer for now. In passing, tweak AfterTriggerSaveEvent to not save the transition_capture pointer into the event record for a deferrable trigger. This is not necessary to fix the current bug, but it avoids letting dangling pointers to long-gone transition tables persist in the trigger event queue. That's at least a safety feature. It might also allow merging shared trigger states in more cases than before. I added a regression test that demonstrates the crash on unpatched code, and also exposes the behavior of firing the AFTER STATEMENT triggers once per row update. Per bug #14808 from Philippe Beaudoin. Back-patch to v10. Discussion: https://postgr.es/m/20170909064853.25630.12825@wrigleys.postgresql.org
1 parent 610bbdd commit 3c43595

File tree

4 files changed

+104
-3
lines changed

4 files changed

+104
-3
lines changed

‎src/backend/commands/trigger.c

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5474,7 +5474,9 @@ AfterTriggerSaveEvent(EState *estate, ResultRelInfo *relinfo,
54745474
new_shared.ats_tgoid = trigger->tgoid;
54755475
new_shared.ats_relid = RelationGetRelid(rel);
54765476
new_shared.ats_firing_id = 0;
5477-
new_shared.ats_transition_capture = transition_capture;
5477+
/* deferrable triggers cannot access transition data */
5478+
new_shared.ats_transition_capture =
5479+
trigger->tgdeferrable ? NULL : transition_capture;
54785480

54795481
afterTriggerAddEvent(&afterTriggers.query_stack[afterTriggers.query_depth],
54805482
&new_event, &new_shared);

‎src/backend/executor/nodeModifyTable.c

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2318,8 +2318,14 @@ ExecEndModifyTable(ModifyTableState *node)
23182318
{
23192319
int i;
23202320

2321-
/* Free transition tables */
2322-
if (node->mt_transition_capture != NULL)
2321+
/*
2322+
* Free transition tables, unless this query is being run in
2323+
* EXEC_FLAG_SKIP_TRIGGERS mode, which means that it may have queued AFTER
2324+
* triggers that won't be run till later. In that case we'll just leak
2325+
* the transition tables till end of (sub)transaction.
2326+
*/
2327+
if (node->mt_transition_capture != NULL &&
2328+
!(node->ps.state->es_top_eflags & EXEC_FLAG_SKIP_TRIGGERS))
23232329
DestroyTransitionCaptureState(node->mt_transition_capture);
23242330

23252331
/*

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

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2257,6 +2257,58 @@ create trigger my_table_multievent_trig
22572257
for each statement execute procedure dump_insert();
22582258
ERROR: Transition tables cannot be specified for triggers with more than one event
22592259
drop table my_table;
2260+
--
2261+
-- Test firing of triggers with transition tables by foreign key cascades
2262+
--
2263+
create table refd_table (a int primary key, b text);
2264+
create table trig_table (a int, b text,
2265+
foreign key (a) references refd_table on update cascade on delete cascade
2266+
);
2267+
create trigger trig_table_insert_trig
2268+
after insert on trig_table referencing new table as new_table
2269+
for each statement execute procedure dump_insert();
2270+
create trigger trig_table_update_trig
2271+
after update on trig_table referencing old table as old_table new table as new_table
2272+
for each statement execute procedure dump_update();
2273+
create trigger trig_table_delete_trig
2274+
after delete on trig_table referencing old table as old_table
2275+
for each statement execute procedure dump_delete();
2276+
insert into refd_table values
2277+
(1, 'one'),
2278+
(2, 'two'),
2279+
(3, 'three');
2280+
insert into trig_table values
2281+
(1, 'one a'),
2282+
(1, 'one b'),
2283+
(2, 'two a'),
2284+
(2, 'two b'),
2285+
(3, 'three a'),
2286+
(3, 'three b');
2287+
NOTICE: trigger = trig_table_insert_trig, new table = (1,"one a"), (1,"one b"), (2,"two a"), (2,"two b"), (3,"three a"), (3,"three b")
2288+
update refd_table set a = 11 where b = 'one';
2289+
NOTICE: trigger = trig_table_update_trig, old table = (1,"one a"), (1,"one b"), new table = (11,"one a"), (11,"one b")
2290+
select * from trig_table;
2291+
a | b
2292+
----+---------
2293+
2 | two a
2294+
2 | two b
2295+
3 | three a
2296+
3 | three b
2297+
11 | one a
2298+
11 | one b
2299+
(6 rows)
2300+
2301+
delete from refd_table where length(b) = 3;
2302+
NOTICE: trigger = trig_table_delete_trig, old table = (2,"two a"), (2,"two b")
2303+
NOTICE: trigger = trig_table_delete_trig, old table = (11,"one a"), (11,"one b")
2304+
select * from trig_table;
2305+
a | b
2306+
---+---------
2307+
3 | three a
2308+
3 | three b
2309+
(2 rows)
2310+
2311+
drop table refd_table, trig_table;
22602312
-- cleanup
22612313
drop function dump_insert();
22622314
drop function dump_update();

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

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1771,6 +1771,47 @@ create trigger my_table_multievent_trig
17711771

17721772
drop table my_table;
17731773

1774+
--
1775+
-- Test firing of triggers with transition tables by foreign key cascades
1776+
--
1777+
1778+
create table refd_table (a int primary key, b text);
1779+
create table trig_table (a int, b text,
1780+
foreign key (a) references refd_table on update cascade on delete cascade
1781+
);
1782+
1783+
create trigger trig_table_insert_trig
1784+
after insert on trig_table referencing new table as new_table
1785+
for each statement execute procedure dump_insert();
1786+
create trigger trig_table_update_trig
1787+
after update on trig_table referencing old table as old_table new table as new_table
1788+
for each statement execute procedure dump_update();
1789+
create trigger trig_table_delete_trig
1790+
after delete on trig_table referencing old table as old_table
1791+
for each statement execute procedure dump_delete();
1792+
1793+
insert into refd_table values
1794+
(1, 'one'),
1795+
(2, 'two'),
1796+
(3, 'three');
1797+
insert into trig_table values
1798+
(1, 'one a'),
1799+
(1, 'one b'),
1800+
(2, 'two a'),
1801+
(2, 'two b'),
1802+
(3, 'three a'),
1803+
(3, 'three b');
1804+
1805+
update refd_table set a = 11 where b = 'one';
1806+
1807+
select * from trig_table;
1808+
1809+
delete from refd_table where length(b) = 3;
1810+
1811+
select * from trig_table;
1812+
1813+
drop table refd_table, trig_table;
1814+
17741815
-- cleanup
17751816
drop function dump_insert();
17761817
drop function dump_update();

0 commit comments

Comments
 (0)