From 8c55244ae379822d8bf62f6db0b5b1f7637eea3a Mon Sep 17 00:00:00 2001 From: Andrew Gierth Date: Wed, 28 Jun 2017 19:00:55 +0100 Subject: [PATCH] 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 --- src/backend/commands/trigger.c | 58 +++++++++++++++++++++++--- src/include/commands/trigger.h | 14 +++++-- src/test/regress/expected/plpgsql.out | 9 +++- src/test/regress/expected/triggers.out | 38 +++++++++++++++++ src/test/regress/sql/plpgsql.sql | 10 ++++- src/test/regress/sql/triggers.sql | 39 +++++++++++++++++ 6 files changed, 155 insertions(+), 13 deletions(-) diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c index 54db16c909..b502941b08 100644 --- 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 06199953fe..36c1134b64 100644 --- 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 35b83f7b00..71099969a4 100644 --- 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 0261d66e5c..aaee30219a 100644 --- 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 = , new table = +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 = +-- +-- 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 f957d70c5f..771d68282e 100644 --- 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 128126a0f1..659a5a1422 100644 --- 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(); -- 2.40.0