From 9ae5a4552641b4e0438f20c85c8cb586ab8e1f64 Mon Sep 17 00:00:00 2001 From: Peter Geoghegan Date: Thu, 3 Dec 2015 17:07:47 -0800 Subject: [PATCH] Fix AFTER UPDATE trigger behavior. ExecOnConflictUpdate() neglected to ensure that the ItemPointer it passed to ExecUpdate() did not point anywhere that could be modified during the execution of ExecUpdate(). The behavior of AFTER UPDATE triggers was therefore broken, as heap_update() modified the current/new ItemPointerData in the tuple in shared memory (t_ctid) before AFTER UPDATE triggers fired. NEW.* and OLD.* tuples were always spuriously identical within AFTER UPDATE triggers. As a further matter, passing a pointer to t_ctid was broken even in the absence of AFTER triggers, as t_ctid does not reliably indicate the TID that was locked (e.g. due to an update made by an aborted transaction). To fix both issues, pass a pointer to t_self instead. Finally, forbid updating a tuple within ExecUpdate() when it was already updated from the predicate or targetlist evaluation of ON CONFLICT DO UPDATE (or perhaps elsewhere within the command). This is similar to, but distinct from a cardinality violation. Reported-By: Stanislav Grozev Author: Andres Freund and Peter Geoghegan --- src/backend/executor/nodeModifyTable.c | 33 ++++++++++++++++++++++++++++----- src/test/regress/expected/triggers.out | 8 ++++---- src/test/regress/expected/with.out | 7 ++++--- src/test/regress/sql/triggers.sql | 2 +- src/test/regress/sql/with.sql | 5 +++-- 5 files changed, 40 insertions(+), 15 deletions(-) diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c index dabaea9..8e2178b 100644 --- a/src/backend/executor/nodeModifyTable.c +++ b/src/backend/executor/nodeModifyTable.c @@ -769,6 +769,7 @@ ExecUpdate(ItemPointer tupleid, TupleTableSlot *planSlot, EPQState *epqstate, EState *estate, + bool onConflict, bool canSetTag) { HeapTuple tuple; @@ -919,6 +920,27 @@ lreplace:; errmsg("tuple to be updated was already modified by an operation triggered by the current command"), errhint("Consider using an AFTER trigger instead of a BEFORE trigger to propagate changes to other rows."))); + /* + * It's possible for ON CONFLICT callers to update their target + * tuple as part of expression evaluation after it is locked, + * but before calling here. This may happen with a + * conventional UPDATE or DELETE, before control reaches here. + * Since the tuple was locked specifically to ensure the call + * here updates without any further conflicts, we throw an + * error. + * + * This is similar to an ON CONFLICT cardinality violation; + * it's also not clear what the final value of the tuple ought + * to be here. We distinguish this case because it's not + * associated with multiple rows that duplicate each other (in + * terms of attributes constrained by an arbiter index) being + * proposed for insertion by the same INSERT statement. + */ + if (onConflict) + ereport(ERROR, + (errcode(ERRCODE_INVALID_RECURSION), + errmsg("tuple locked for update by ON CONFLICT DO UPDATE was modified early"))); + /* Else, already updated by self; nothing to do */ return NULL; @@ -926,6 +948,7 @@ lreplace:; break; case HeapTupleUpdated: + Assert(!onConflict); if (IsolationUsesXactSnapshot()) ereport(ERROR, (errcode(ERRCODE_T_R_SERIALIZATION_FAILURE), @@ -1182,10 +1205,9 @@ ExecOnConflictUpdate(ModifyTableState *mtstate, ExecProject(resultRelInfo->ri_onConflictSetProj, NULL); /* Execute UPDATE with projection */ - *returning = ExecUpdate(&tuple.t_data->t_ctid, NULL, - mtstate->mt_conflproj, planSlot, - &mtstate->mt_epqstate, mtstate->ps.state, - canSetTag); + *returning = ExecUpdate(&tuple.t_self, NULL, mtstate->mt_conflproj, + planSlot, &mtstate->mt_epqstate, mtstate->ps.state, + true, canSetTag); ReleaseBuffer(buffer); return true; @@ -1433,7 +1455,8 @@ ExecModifyTable(ModifyTableState *node) break; case CMD_UPDATE: slot = ExecUpdate(tupleid, oldtuple, slot, planSlot, - &node->mt_epqstate, estate, node->canSetTag); + &node->mt_epqstate, estate, false, + node->canSetTag); break; case CMD_DELETE: slot = ExecDelete(tupleid, oldtuple, planSlot, diff --git a/src/test/regress/expected/triggers.out b/src/test/regress/expected/triggers.out index dd4a99b..a7bf5dc 100644 --- a/src/test/regress/expected/triggers.out +++ b/src/test/regress/expected/triggers.out @@ -1703,7 +1703,7 @@ create function upsert_after_func() $$ begin if (TG_OP = 'UPDATE') then - raise warning 'after update (old): %', new.*::text; + raise warning 'after update (old): %', old.*::text; raise warning 'after update (new): %', new.*::text; elsif (TG_OP = 'INSERT') then raise warning 'after insert (new): %', new.*::text; @@ -1724,7 +1724,7 @@ insert into upsert values(3, 'orange') on conflict (key) do update set color = ' WARNING: before insert (new): (3,orange) WARNING: before update (old): (3,"red trig modified") WARNING: before update (new): (3,"updated red trig modified") -WARNING: after update (old): (3,"updated red trig modified") +WARNING: after update (old): (3,"red trig modified") WARNING: after update (new): (3,"updated red trig modified") insert into upsert values(4, 'green') on conflict (key) do update set color = 'updated ' || upsert.color; WARNING: before insert (new): (4,green) @@ -1734,7 +1734,7 @@ insert into upsert values(5, 'purple') on conflict (key) do update set color = ' WARNING: before insert (new): (5,purple) WARNING: before update (old): (5,"green trig modified") WARNING: before update (new): (5,"updated green trig modified") -WARNING: after update (old): (5,"updated green trig modified") +WARNING: after update (old): (5,"green trig modified") WARNING: after update (new): (5,"updated green trig modified") insert into upsert values(6, 'white') on conflict (key) do update set color = 'updated ' || upsert.color; WARNING: before insert (new): (6,white) @@ -1744,7 +1744,7 @@ insert into upsert values(7, 'pink') on conflict (key) do update set color = 'up WARNING: before insert (new): (7,pink) WARNING: before update (old): (7,"white trig modified") WARNING: before update (new): (7,"updated white trig modified") -WARNING: after update (old): (7,"updated white trig modified") +WARNING: after update (old): (7,"white trig modified") WARNING: after update (new): (7,"updated white trig modified") insert into upsert values(8, 'yellow') on conflict (key) do update set color = 'updated ' || upsert.color; WARNING: before insert (new): (8,yellow) diff --git a/src/test/regress/expected/with.out b/src/test/regress/expected/with.out index 2c9226c..d9c7b3e 100644 --- a/src/test/regress/expected/with.out +++ b/src/test/regress/expected/with.out @@ -1875,8 +1875,9 @@ ON CONFLICT (k) DO UPDATE SET v = (SELECT b || ' update' FROM aa WHERE a = 'a' L WITH aa AS (SELECT 1 a, 2 b) INSERT INTO z VALUES(1, (SELECT b || ' insert' FROM aa WHERE a = 1 )) ON CONFLICT (k) DO UPDATE SET v = (SELECT b || ' update' FROM aa WHERE a = 1 LIMIT 1); --- This shows an attempt to update an invisible row, which should really be --- reported as a cardinality violation, but it doesn't seem worth fixing: +-- Attempt to update a row more than once following locking a row to update +-- from ON CONFLICT DO UPDATE. This is rejected, much like a cardinality +-- violation. WITH simpletup AS ( SELECT 2 k, 'Green' v), upsert_cte AS ( @@ -1886,7 +1887,7 @@ upsert_cte AS ( INSERT INTO z VALUES(2, 'Red') ON CONFLICT (k) DO UPDATE SET (k, v) = (SELECT k, v FROM upsert_cte WHERE upsert_cte.k = z.k) RETURNING k, v; -ERROR: attempted to update invisible tuple +ERROR: tuple locked for update by ON CONFLICT DO UPDATE was modified early DROP TABLE z; -- check that run to completion happens in proper ordering TRUNCATE TABLE y; diff --git a/src/test/regress/sql/triggers.sql b/src/test/regress/sql/triggers.sql index 9f66702..b6de1b3 100644 --- a/src/test/regress/sql/triggers.sql +++ b/src/test/regress/sql/triggers.sql @@ -1215,7 +1215,7 @@ create function upsert_after_func() $$ begin if (TG_OP = 'UPDATE') then - raise warning 'after update (old): %', new.*::text; + raise warning 'after update (old): %', old.*::text; raise warning 'after update (new): %', new.*::text; elsif (TG_OP = 'INSERT') then raise warning 'after insert (new): %', new.*::text; diff --git a/src/test/regress/sql/with.sql b/src/test/regress/sql/with.sql index 3fd55f9..71dbce5 100644 --- a/src/test/regress/sql/with.sql +++ b/src/test/regress/sql/with.sql @@ -838,8 +838,9 @@ WITH aa AS (SELECT 1 a, 2 b) INSERT INTO z VALUES(1, (SELECT b || ' insert' FROM aa WHERE a = 1 )) ON CONFLICT (k) DO UPDATE SET v = (SELECT b || ' update' FROM aa WHERE a = 1 LIMIT 1); --- This shows an attempt to update an invisible row, which should really be --- reported as a cardinality violation, but it doesn't seem worth fixing: +-- Attempt to update a row more than once following locking a row to update +-- from ON CONFLICT DO UPDATE. This is rejected, much like a cardinality +-- violation. WITH simpletup AS ( SELECT 2 k, 'Green' v), upsert_cte AS ( -- 1.9.1