From 601524ff0cecdca75474873185bbdd2c018f4422 Mon Sep 17 00:00:00 2001 From: Sven Klemm Date: Sat, 2 Mar 2019 08:31:43 +0100 Subject: [PATCH] Fix ON CONFLICT when using prepared statements and functions A prepared statement or function plan can be turned into a generic plan after a couple executions. When this happens the last plan that run through plan creation gets reused so we need to revert any modifications we did to that plan so reusing it is safe. --- CHANGELOG.md | 2 + src/chunk_dispatch_state.c | 27 ++++++-- test/expected/upsert.out | 134 +++++++++++++++++++++++++++++++++++++ test/sql/upsert.sql | 62 +++++++++++++++++ 4 files changed, 220 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d47303c6d..bfe585d46 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,7 @@ accidentally triggering the load of a previous DB version.** * PR #1067 Add treat_null_as_missing option to locf **Bugfixes** +* PR #1088 Fix ON CONFLICT when using prepared statements and functions * PR #1089 Fix compatibility with extensions that define planner_hook * [5a3edfd] Fix chunk exclusion constraint type inference * [8e86bda] Fix sort_transform optimization @@ -21,6 +22,7 @@ accidentally triggering the load of a previous DB version.** **Thanks** * @esatterwhite for reporting a bug when using timescaledb with zombodb * @eeeebbbbrrrr for fixing compatibility with extensions that also define planner_hook +* @naquad for reporting a segfault when using ON conflict in stored procedures ## 1.2.1 (2019-02-11) diff --git a/src/chunk_dispatch_state.c b/src/chunk_dispatch_state.c index b0c0ecfcf..e458e3af9 100644 --- a/src/chunk_dispatch_state.c +++ b/src/chunk_dispatch_state.c @@ -88,9 +88,7 @@ chunk_dispatch_exec(CustomScanState *node) { /* * Update the arbiter indexes for ON CONFLICT statements so that they - * match the chunk. Note that this requires updating the existing List - * head (not replacing it), or otherwise the ModifyTableState node - * won't pick it up. + * match the chunk. */ if (cis->arbiter_indexes != NIL) { @@ -160,6 +158,17 @@ chunk_dispatch_end(CustomScanState *node) ExecEndNode(substate); ts_chunk_dispatch_destroy(state->dispatch); ts_cache_release(state->hypertable_cache); + + /* + * we need to restore arbiterIndexes list to it's original value + * in case this gets turned into generic plan and skips the plan + * creation for the next execution + */ +#if PG96 || PG10 + state->parent->mt_arbiterindexes = state->dispatch->arbiter_indexes; +#else + ((ModifyTable *) state->parent->ps.plan)->arbiterIndexes = state->dispatch->arbiter_indexes; +#endif } static void @@ -240,9 +249,17 @@ ts_chunk_dispatch_state_set_parent(ChunkDispatchState *state, ModifyTableState * mt_plan = (ModifyTable *) parent->ps.plan; state->dispatch->returning_lists = mt_plan->returningLists; - state->dispatch->on_conflict_set = mt_plan->onConflictSet; - state->dispatch->arbiter_indexes = mt_plan->arbiterIndexes; state->dispatch->on_conflict = mt_plan->onConflictAction; + state->dispatch->on_conflict_set = mt_plan->onConflictSet; + + /* + * Save arbiterIndexes from the ModifyTable plan node because we + * replace the list during execution with our modified version. + * We restore it to it's original value after execution in + * chunk_dispatch_end, this is required should this plan be + * turned into a generic plan. + */ + state->dispatch->arbiter_indexes = mt_plan->arbiterIndexes; Assert(mt_plan->onConflictWhere == NULL || IsA(mt_plan->onConflictWhere, List)); state->dispatch->on_conflict_where = (List *) mt_plan->onConflictWhere; diff --git a/test/expected/upsert.out b/test/expected/upsert.out index 7ecbea2f3..6ea3d9b23 100644 --- a/test/expected/upsert.out +++ b/test/expected/upsert.out @@ -437,3 +437,137 @@ select * from cte; Sun Jan 21 09:00:01 2018 | dev3 (2 rows) +-- test ON CONFLICT with prepared statements +CREATE TABLE prepared_test(time timestamptz PRIMARY KEY, value float); +SELECT create_hypertable('prepared_test','time'); + create_hypertable +---------------------------- + (7,public,prepared_test,t) +(1 row) + +PREPARE prep_insert AS INSERT INTO prepared_test VALUES('2000-01-01',0.5) ON CONFLICT (time) DO UPDATE SET value = EXCLUDED.value; +-- at some point PostgreSQL will turn the plan into a generic plan +-- so we execute the prepared statement 10 times +EXECUTE prep_insert; +EXECUTE prep_insert; +EXECUTE prep_insert; +EXECUTE prep_insert; +EXECUTE prep_insert; +EXECUTE prep_insert; +EXECUTE prep_insert; +EXECUTE prep_insert; +EXECUTE prep_insert; +EXECUTE prep_insert; +SELECT * FROM prepared_test; + time | value +------------------------------+------- + Sat Jan 01 00:00:00 2000 PST | 0.5 +(1 row) + +DELETE FROM prepared_test; +-- test ON CONFLICT with functions +CREATE OR REPLACE FUNCTION test_upsert(t timestamptz, v float) RETURNS VOID AS $sql$ +BEGIN +INSERT INTO prepared_test VALUES(t,v) ON CONFLICT (time) DO UPDATE SET value = EXCLUDED.value; +END; +$sql$ LANGUAGE PLPGSQL; +-- at some point PostgreSQL will turn the plan into a generic plan +-- so we execute the function 10 times +SELECT counter,test_upsert('2000-01-01',0.5) FROM generate_series(1,10) AS g(counter); + counter | test_upsert +---------+------------- + 1 | + 2 | + 3 | + 4 | + 5 | + 6 | + 7 | + 8 | + 9 | + 10 | +(10 rows) + +SELECT * FROM prepared_test; + time | value +------------------------------+------- + Sat Jan 01 00:00:00 2000 PST | 0.5 +(1 row) + +DELETE FROM prepared_test; +-- at some point PostgreSQL will turn the plan into a generic plan +-- so we execute the function 10 times +SELECT counter,test_upsert('2000-01-01',0.5) FROM generate_series(1,10) AS g(counter); + counter | test_upsert +---------+------------- + 1 | + 2 | + 3 | + 4 | + 5 | + 6 | + 7 | + 8 | + 9 | + 10 | +(10 rows) + +SELECT * FROM prepared_test; + time | value +------------------------------+------- + Sat Jan 01 00:00:00 2000 PST | 0.5 +(1 row) + +DELETE FROM prepared_test; +-- run it again to ensure INSERT path is still working as well +SELECT counter,test_upsert('2000-01-01',0.5) FROM generate_series(1,10) AS g(counter); + counter | test_upsert +---------+------------- + 1 | + 2 | + 3 | + 4 | + 5 | + 6 | + 7 | + 8 | + 9 | + 10 | +(10 rows) + +SELECT * FROM prepared_test; + time | value +------------------------------+------- + Sat Jan 01 00:00:00 2000 PST | 0.5 +(1 row) + +DELETE FROM prepared_test; +-- test ON CONFLICT with functions +CREATE OR REPLACE FUNCTION test_upsert2(t timestamptz, v float) RETURNS VOID AS $sql$ +BEGIN +INSERT INTO prepared_test VALUES(t,v) ON CONFLICT (time) DO UPDATE SET value = prepared_test.value + 1.0; +END; +$sql$ LANGUAGE PLPGSQL; +-- at some point PostgreSQL will turn the plan into a generic plan +-- so we execute the function 10 times +SELECT counter,test_upsert2('2000-01-01',1.0) FROM generate_series(1,10) AS g(counter); + counter | test_upsert2 +---------+-------------- + 1 | + 2 | + 3 | + 4 | + 5 | + 6 | + 7 | + 8 | + 9 | + 10 | +(10 rows) + +SELECT * FROM prepared_test; + time | value +------------------------------+------- + Sat Jan 01 00:00:00 2000 PST | 10 +(1 row) + diff --git a/test/sql/upsert.sql b/test/sql/upsert.sql index d712195ff..fba8da419 100644 --- a/test/sql/upsert.sql +++ b/test/sql/upsert.sql @@ -209,3 +209,65 @@ INSERT INTO upsert_test_arbiter (time, device_id) VALUES ON CONFLICT (time, device_id) DO UPDATE SET device_id = 'dev3' RETURNING *) select * from cte; + +-- test ON CONFLICT with prepared statements +CREATE TABLE prepared_test(time timestamptz PRIMARY KEY, value float); +SELECT create_hypertable('prepared_test','time'); + +PREPARE prep_insert AS INSERT INTO prepared_test VALUES('2000-01-01',0.5) ON CONFLICT (time) DO UPDATE SET value = EXCLUDED.value; + +-- at some point PostgreSQL will turn the plan into a generic plan +-- so we execute the prepared statement 10 times +EXECUTE prep_insert; +EXECUTE prep_insert; +EXECUTE prep_insert; +EXECUTE prep_insert; +EXECUTE prep_insert; +EXECUTE prep_insert; +EXECUTE prep_insert; +EXECUTE prep_insert; +EXECUTE prep_insert; +EXECUTE prep_insert; + +SELECT * FROM prepared_test; +DELETE FROM prepared_test; + +-- test ON CONFLICT with functions +CREATE OR REPLACE FUNCTION test_upsert(t timestamptz, v float) RETURNS VOID AS $sql$ +BEGIN +INSERT INTO prepared_test VALUES(t,v) ON CONFLICT (time) DO UPDATE SET value = EXCLUDED.value; +END; +$sql$ LANGUAGE PLPGSQL; + +-- at some point PostgreSQL will turn the plan into a generic plan +-- so we execute the function 10 times +SELECT counter,test_upsert('2000-01-01',0.5) FROM generate_series(1,10) AS g(counter); + +SELECT * FROM prepared_test; +DELETE FROM prepared_test; + +-- at some point PostgreSQL will turn the plan into a generic plan +-- so we execute the function 10 times +SELECT counter,test_upsert('2000-01-01',0.5) FROM generate_series(1,10) AS g(counter); + +SELECT * FROM prepared_test; +DELETE FROM prepared_test; + +-- run it again to ensure INSERT path is still working as well +SELECT counter,test_upsert('2000-01-01',0.5) FROM generate_series(1,10) AS g(counter); + +SELECT * FROM prepared_test; +DELETE FROM prepared_test; + +-- test ON CONFLICT with functions +CREATE OR REPLACE FUNCTION test_upsert2(t timestamptz, v float) RETURNS VOID AS $sql$ +BEGIN +INSERT INTO prepared_test VALUES(t,v) ON CONFLICT (time) DO UPDATE SET value = prepared_test.value + 1.0; +END; +$sql$ LANGUAGE PLPGSQL; + +-- at some point PostgreSQL will turn the plan into a generic plan +-- so we execute the function 10 times +SELECT counter,test_upsert2('2000-01-01',1.0) FROM generate_series(1,10) AS g(counter); + +SELECT * FROM prepared_test;