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[email protected]

diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c
index da0850bfd6d9dca3d2021f4edcda58c73fc48886..bbfbc06db9094a156ed1bb05213b52676269dc38 100644 (file)
--- a/src/backend/commands/trigger.c
+++ b/src/backend/commands/trigger.c
@@ -5474,7 +5474,9 @@ AfterTriggerSaveEvent(EState *estate, ResultRelInfo *relinfo,
        new_shared.ats_tgoid = trigger->tgoid;
        new_shared.ats_relid = RelationGetRelid(rel);
        new_shared.ats_firing_id = 0;
-       new_shared.ats_transition_capture = transition_capture;
+       /* deferrable triggers cannot access transition data */
+       new_shared.ats_transition_capture =
+           trigger->tgdeferrable ? NULL : transition_capture;
 
        afterTriggerAddEvent(&afterTriggers.query_stack[afterTriggers.query_depth],
                             &new_event, &new_shared);

diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c
index bd84778739824a192de303ed9f9c29d44544db7c..49586a3c032cfc450b8e98a0989f25cfa678cc51 100644 (file)
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -2318,8 +2318,14 @@ ExecEndModifyTable(ModifyTableState *node)
 {
    int         i;
 
-   /* Free transition tables */
-   if (node->mt_transition_capture != NULL)
+   /*
+    * Free transition tables, unless this query is being run in
+    * EXEC_FLAG_SKIP_TRIGGERS mode, which means that it may have queued AFTER
+    * triggers that won't be run till later.  In that case we'll just leak
+    * the transition tables till end of (sub)transaction.
+    */
+   if (node->mt_transition_capture != NULL &&
+       !(node->ps.state->es_top_eflags & EXEC_FLAG_SKIP_TRIGGERS))
        DestroyTransitionCaptureState(node->mt_transition_capture);
 
    /*

diff --git a/src/test/regress/expected/triggers.out b/src/test/regress/expected/triggers.out
index ac132b042d598f104fda067835d1bd706800f595..2f8029a2f73679088707faaace4af39f87633fa8 100644 (file)
--- a/src/test/regress/expected/triggers.out
+++ b/src/test/regress/expected/triggers.out
@@ -2257,6 +2257,58 @@ create trigger my_table_multievent_trig
   for each statement execute procedure dump_insert();
 ERROR:  Transition tables cannot be specified for triggers with more than one event
 drop table my_table;
+--
+-- Test firing of triggers with transition tables by foreign key cascades
+--
+create table refd_table (a int primary key, b text);
+create table trig_table (a int, b text,
+  foreign key (a) references refd_table on update cascade on delete cascade
+);
+create trigger trig_table_insert_trig
+  after insert on trig_table referencing new table as new_table
+  for each statement execute procedure dump_insert();
+create trigger trig_table_update_trig
+  after update on trig_table referencing old table as old_table new table as new_table
+  for each statement execute procedure dump_update();
+create trigger trig_table_delete_trig
+  after delete on trig_table referencing old table as old_table
+  for each statement execute procedure dump_delete();
+insert into refd_table values
+  (1, 'one'),
+  (2, 'two'),
+  (3, 'three');
+insert into trig_table values
+  (1, 'one a'),
+  (1, 'one b'),
+  (2, 'two a'),
+  (2, 'two b'),
+  (3, 'three a'),
+  (3, 'three b');
+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")
+update refd_table set a = 11 where b = 'one';
+NOTICE:  trigger = trig_table_update_trig, old table = (1,"one a"), (1,"one b"), new table = (11,"one a"), (11,"one b")
+select * from trig_table;
+ a  |    b    
+----+---------
+  2 | two a
+  2 | two b
+  3 | three a
+  3 | three b
+ 11 | one a
+ 11 | one b
+(6 rows)
+
+delete from refd_table where length(b) = 3;
+NOTICE:  trigger = trig_table_delete_trig, old table = (2,"two a"), (2,"two b")
+NOTICE:  trigger = trig_table_delete_trig, old table = (11,"one a"), (11,"one b")
+select * from trig_table;
+ a |    b    
+---+---------
+ 3 | three a
+ 3 | three b
+(2 rows)
+
+drop table refd_table, trig_table;
 -- cleanup
 drop function dump_insert();
 drop function dump_update();

diff --git a/src/test/regress/sql/triggers.sql b/src/test/regress/sql/triggers.sql
index b10159a1cf2a3938ea9c51c0259b30ffcb1f721b..c6deb56c50756c4381519583bef9a363d243bd58 100644 (file)
--- a/src/test/regress/sql/triggers.sql
+++ b/src/test/regress/sql/triggers.sql
@@ -1771,6 +1771,47 @@ create trigger my_table_multievent_trig
 
 drop table my_table;
 
+--
+-- Test firing of triggers with transition tables by foreign key cascades
+--
+
+create table refd_table (a int primary key, b text);
+create table trig_table (a int, b text,
+  foreign key (a) references refd_table on update cascade on delete cascade
+);
+
+create trigger trig_table_insert_trig
+  after insert on trig_table referencing new table as new_table
+  for each statement execute procedure dump_insert();
+create trigger trig_table_update_trig
+  after update on trig_table referencing old table as old_table new table as new_table
+  for each statement execute procedure dump_update();
+create trigger trig_table_delete_trig
+  after delete on trig_table referencing old table as old_table
+  for each statement execute procedure dump_delete();
+
+insert into refd_table values
+  (1, 'one'),
+  (2, 'two'),
+  (3, 'three');
+insert into trig_table values
+  (1, 'one a'),
+  (1, 'one b'),
+  (2, 'two a'),
+  (2, 'two b'),
+  (3, 'three a'),
+  (3, 'three b');
+
+update refd_table set a = 11 where b = 'one';
+
+select * from trig_table;
+
+delete from refd_table where length(b) = 3;
+
+select * from trig_table;
+
+drop table refd_table, trig_table;
+
 -- cleanup
 drop function dump_insert();
 drop function dump_update();