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

Author: RhodiumToad

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);

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;
 
 /*

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();

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();

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();

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();