Fix transition tables for ON CONFLICT.

We now disallow having triggers with both transition tables and ON
INSERT OR UPDATE (which was a PG extension to the spec anyway),
because in this case it's not at all clear how the transition tables
should work for an INSERT ... ON CONFLICT query.  Separate ON INSERT
and ON UPDATE triggers with transition tables are allowed, and the
transition tables for these reflect only the inserted and only the
updated tuples respectively.

Patch by Thomas Munro

Discussion: https://postgr.es/m/CAEepm%3D11KHQ0JmETJQihSvhZB5mUZL2xrqHeXbCeLhDiqQ39%3Dw%40mail.gmail.com

diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c
index 54db16c909029a040f0c92c3c9e3145e20afe866..b502941b08bdfd527529ba0c79b9aa1d1da14953 100644 (file)
--- a/src/backend/commands/trigger.c
+++ b/src/backend/commands/trigger.c
@@ -401,6 +401,23 @@ CreateTrigger(CreateTrigStmt *stmt, const char *queryString,
                                                (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
                                                 errmsg("TRUNCATE triggers with transition tables are not supported")));
 
+                       /*
+                        * We currently don't allow multi-event triggers ("INSERT OR
+                        * UPDATE") with transition tables, because it's not clear how to
+                        * handle INSERT ... ON CONFLICT statements which can fire both
+                        * INSERT and UPDATE triggers.  We show the inserted tuples to
+                        * INSERT triggers and the updated tuples to UPDATE triggers, but
+                        * it's not yet clear what INSERT OR UPDATE trigger should see.
+                        * This restriction could be lifted if we can decide on the right
+                        * semantics in a later release.
+                        */
+                       if (((TRIGGER_FOR_INSERT(tgtype) ? 1 : 0) +
+                                (TRIGGER_FOR_UPDATE(tgtype) ? 1 : 0) +
+                                (TRIGGER_FOR_DELETE(tgtype) ? 1 : 0)) != 1)
+                               ereport(ERROR,
+                                               (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+                                                errmsg("Transition tables cannot be specified for triggers with more than one event")));
+
                        if (tt->isNew)
                        {
                                if (!(TRIGGER_FOR_INSERT(tgtype) ||
@@ -2128,8 +2145,10 @@ MakeTransitionCaptureState(TriggerDesc *trigdesc)
                        CurrentResourceOwner = CurTransactionResourceOwner;
                        if (trigdesc->trig_delete_old_table || trigdesc->trig_update_old_table)
                                state->tcs_old_tuplestore = tuplestore_begin_heap(false, false, work_mem);
-                       if (trigdesc->trig_insert_new_table || trigdesc->trig_update_new_table)
-                               state->tcs_new_tuplestore = tuplestore_begin_heap(false, false, work_mem);
+                       if (trigdesc->trig_insert_new_table)
+                               state->tcs_insert_tuplestore = tuplestore_begin_heap(false, false, work_mem);
+                       if (trigdesc->trig_update_new_table)
+                               state->tcs_update_tuplestore = tuplestore_begin_heap(false, false, work_mem);
                }
                PG_CATCH();
                {
@@ -2147,8 +2166,10 @@ MakeTransitionCaptureState(TriggerDesc *trigdesc)
 void
 DestroyTransitionCaptureState(TransitionCaptureState *tcs)
 {
-       if (tcs->tcs_new_tuplestore != NULL)
-               tuplestore_end(tcs->tcs_new_tuplestore);
+       if (tcs->tcs_insert_tuplestore != NULL)
+               tuplestore_end(tcs->tcs_insert_tuplestore);
+       if (tcs->tcs_update_tuplestore != NULL)
+               tuplestore_end(tcs->tcs_update_tuplestore);
        if (tcs->tcs_old_tuplestore != NULL)
                tuplestore_end(tcs->tcs_old_tuplestore);
        pfree(tcs);
@@ -3993,7 +4014,29 @@ AfterTriggerExecute(AfterTriggerEvent event,
                if (LocTriggerData.tg_trigger->tgoldtable)
                        LocTriggerData.tg_oldtable = transition_capture->tcs_old_tuplestore;
                if (LocTriggerData.tg_trigger->tgnewtable)
-                       LocTriggerData.tg_newtable = transition_capture->tcs_new_tuplestore;
+               {
+                       /*
+                        * Currently a trigger with transition tables may only be defined
+                        * for a single event type (here AFTER INSERT or AFTER UPDATE, but
+                        * not AFTER INSERT OR ...).
+                        */
+                       Assert((TRIGGER_FOR_INSERT(LocTriggerData.tg_trigger->tgtype) != 0) ^
+                                  (TRIGGER_FOR_UPDATE(LocTriggerData.tg_trigger->tgtype) != 0));
+
+                       /*
+                        * Show either the insert or update new tuple images, depending on
+                        * which event type the trigger was registered for.  A single
+                        * statement may have produced both in the case of INSERT ... ON
+                        * CONFLICT ... DO UPDATE, and in that case the event determines
+                        * which tuplestore the trigger sees as the NEW TABLE.
+                        */
+                       if (TRIGGER_FOR_INSERT(LocTriggerData.tg_trigger->tgtype))
+                               LocTriggerData.tg_newtable =
+                                       transition_capture->tcs_insert_tuplestore;
+                       else
+                               LocTriggerData.tg_newtable =
+                                       transition_capture->tcs_update_tuplestore;
+               }
        }
 
        /*
@@ -5241,7 +5284,10 @@ AfterTriggerSaveEvent(EState *estate, ResultRelInfo *relinfo,
                        Tuplestorestate *new_tuplestore;
 
                        Assert(newtup != NULL);
-                       new_tuplestore = transition_capture->tcs_new_tuplestore;
+                       if (event == TRIGGER_EVENT_INSERT)
+                               new_tuplestore = transition_capture->tcs_insert_tuplestore;
+                       else
+                               new_tuplestore = transition_capture->tcs_update_tuplestore;
 
                        if (original_insert_tuple != NULL)
                                tuplestore_puttuple(new_tuplestore, original_insert_tuple);

diff --git a/src/include/commands/trigger.h b/src/include/commands/trigger.h
index 06199953fe9afda58068bc41c5b570d63874baa1..36c1134b649d954348f3c465a4d92c581354a46f 100644 (file)
--- a/src/include/commands/trigger.h
+++ b/src/include/commands/trigger.h
@@ -73,9 +73,17 @@ typedef struct TransitionCaptureState
         */
        HeapTuple       tcs_original_insert_tuple;
 
-       /* The tuplestores backing the transition tables. */
-       Tuplestorestate *tcs_old_tuplestore;
-       Tuplestorestate *tcs_new_tuplestore;
+       /*
+        * The tuplestores backing the transition tables.  We use separate
+        * tuplestores for INSERT and UPDATE, because INSERT ... ON CONFLICT
+        * ... DO UPDATE causes INSERT and UPDATE triggers to fire and needs a way
+        * to keep track of the new tuple images resulting from the two cases
+        * separately.  We only need a single old image tuplestore, because there
+        * is no statement that can both update and delete at the same time.
+        */
+       Tuplestorestate *tcs_old_tuplestore; /* for DELETE and UPDATE old images */
+       Tuplestorestate *tcs_insert_tuplestore; /* for INSERT new images */
+       Tuplestorestate *tcs_update_tuplestore; /* for UPDATE new images */
 } TransitionCaptureState;
 
 /*

diff --git a/src/test/regress/expected/plpgsql.out b/src/test/regress/expected/plpgsql.out
index 35b83f7b009797f1ffe954f6f252a07aa5dfb156..71099969a47e8db16478db61f3d476ca4859e5d0 100644 (file)
--- a/src/test/regress/expected/plpgsql.out
+++ b/src/test/regress/expected/plpgsql.out
@@ -5847,8 +5847,13 @@ AS $$
     RETURN NULL;
   END;
 $$;
-CREATE TRIGGER transition_table_level2_ri_child_insupd_trigger
-  AFTER INSERT OR UPDATE ON transition_table_level2
+CREATE TRIGGER transition_table_level2_ri_child_ins_trigger
+  AFTER INSERT ON transition_table_level2
+  REFERENCING NEW TABLE AS i
+  FOR EACH STATEMENT EXECUTE PROCEDURE
+    transition_table_level2_ri_child_insupd_func();
+CREATE TRIGGER transition_table_level2_ri_child_upd_trigger
+  AFTER UPDATE ON transition_table_level2
   REFERENCING NEW TABLE AS i
   FOR EACH STATEMENT EXECUTE PROCEDURE
     transition_table_level2_ri_child_insupd_func();

diff --git a/src/test/regress/expected/triggers.out b/src/test/regress/expected/triggers.out
index 0261d66e5c593b91fb2aea80412e5fc64c10b8a9..aaee30219ad2d710d09eccf13449944fa32b622e 100644 (file)
--- a/src/test/regress/expected/triggers.out
+++ b/src/test/regress/expected/triggers.out
@@ -2214,6 +2214,44 @@ NOTICE:  trigger = table2_trig, new table = ("hello world")
 NOTICE:  trigger = table1_trig, new table = (42)
 drop table table1;
 drop table table2;
+--
+-- Verify behavior of INSERT ... ON CONFLICT DO UPDATE ... with
+-- transition tables.
+--
+create table my_table (a int primary key, b text);
+create trigger my_table_insert_trig
+  after insert on my_table referencing new table as new_table
+  for each statement execute procedure dump_insert();
+create trigger my_table_update_trig
+  after update on my_table referencing old table as old_table new table as new_table
+  for each statement execute procedure dump_update();
+-- inserts only
+insert into my_table values (1, 'AAA'), (2, 'BBB')
+  on conflict (a) do
+  update set b = my_table.b || ':' || excluded.b;
+NOTICE:  trigger = my_table_update_trig, old table = <NULL>, new table = <NULL>
+NOTICE:  trigger = my_table_insert_trig, new table = (1,AAA), (2,BBB)
+-- mixture of inserts and updates
+insert into my_table values (1, 'AAA'), (2, 'BBB'), (3, 'CCC'), (4, 'DDD')
+  on conflict (a) do
+  update set b = my_table.b || ':' || excluded.b;
+NOTICE:  trigger = my_table_update_trig, old table = (1,AAA), (2,BBB), new table = (1,AAA:AAA), (2,BBB:BBB)
+NOTICE:  trigger = my_table_insert_trig, new table = (3,CCC), (4,DDD)
+-- updates only
+insert into my_table values (3, 'CCC'), (4, 'DDD')
+  on conflict (a) do
+  update set b = my_table.b || ':' || excluded.b;
+NOTICE:  trigger = my_table_update_trig, old table = (3,CCC), (4,DDD), new table = (3,CCC:CCC), (4,DDD:DDD)
+NOTICE:  trigger = my_table_insert_trig, new table = <NULL>
+--
+-- Verify that you can't create a trigger with transition tables for
+-- more than one event.
+--
+create trigger my_table_multievent_trig
+  after insert or update on my_table referencing new table as new_table
+  for each statement execute procedure dump_insert();
+ERROR:  Transition tables cannot be specified for triggers with more than one event
+drop table my_table;
 -- cleanup
 drop function dump_insert();
 drop function dump_update();

diff --git a/src/test/regress/sql/plpgsql.sql b/src/test/regress/sql/plpgsql.sql
index f957d70c5ff5f8f63d25f239fffa6b0a81f771f4..771d68282eed8c744f77f9bcf955ce23202ee835 100644 (file)
--- a/src/test/regress/sql/plpgsql.sql
+++ b/src/test/regress/sql/plpgsql.sql
@@ -4639,8 +4639,14 @@ AS $$
   END;
 $$;
 
-CREATE TRIGGER transition_table_level2_ri_child_insupd_trigger
-  AFTER INSERT OR UPDATE ON transition_table_level2
+CREATE TRIGGER transition_table_level2_ri_child_ins_trigger
+  AFTER INSERT ON transition_table_level2
+  REFERENCING NEW TABLE AS i
+  FOR EACH STATEMENT EXECUTE PROCEDURE
+    transition_table_level2_ri_child_insupd_func();
+
+CREATE TRIGGER transition_table_level2_ri_child_upd_trigger
+  AFTER UPDATE ON transition_table_level2
   REFERENCING NEW TABLE AS i
   FOR EACH STATEMENT EXECUTE PROCEDURE
     transition_table_level2_ri_child_insupd_func();

diff --git a/src/test/regress/sql/triggers.sql b/src/test/regress/sql/triggers.sql
index 128126a0f1a1bf11382c88bf4fb52326025e079c..659a5a1422bcd0914b59926a6887f190e6500c69 100644 (file)
--- a/src/test/regress/sql/triggers.sql
+++ b/src/test/regress/sql/triggers.sql
@@ -1725,6 +1725,45 @@ with wcte as (insert into table1 values (42))
 drop table table1;
 drop table table2;
 
+--
+-- Verify behavior of INSERT ... ON CONFLICT DO UPDATE ... with
+-- transition tables.
+--
+
+create table my_table (a int primary key, b text);
+create trigger my_table_insert_trig
+  after insert on my_table referencing new table as new_table
+  for each statement execute procedure dump_insert();
+create trigger my_table_update_trig
+  after update on my_table referencing old table as old_table new table as new_table
+  for each statement execute procedure dump_update();
+
+-- inserts only
+insert into my_table values (1, 'AAA'), (2, 'BBB')
+  on conflict (a) do
+  update set b = my_table.b || ':' || excluded.b;
+
+-- mixture of inserts and updates
+insert into my_table values (1, 'AAA'), (2, 'BBB'), (3, 'CCC'), (4, 'DDD')
+  on conflict (a) do
+  update set b = my_table.b || ':' || excluded.b;
+
+-- updates only
+insert into my_table values (3, 'CCC'), (4, 'DDD')
+  on conflict (a) do
+  update set b = my_table.b || ':' || excluded.b;
+
+--
+-- Verify that you can't create a trigger with transition tables for
+-- more than one event.
+--
+
+create trigger my_table_multievent_trig
+  after insert or update on my_table referencing new table as new_table
+  for each statement execute procedure dump_insert();
+
+drop table my_table;
+
 -- cleanup
 drop function dump_insert();
 drop function dump_update();