Skip to content

Commit 84ac126

Browse files
anarazelanarazel
committed
Fix ON CONFLICT UPDATE bug breaking AFTER UPDATE triggers.
ExecOnConflictUpdate() passed t_ctid of the to-be-updated tuple to ExecUpdate(). That's problematic primarily because of two reason: First and foremost t_ctid could point to a different tuple. Secondly, and that's what triggered the complaint by Stanislav, t_ctid is changed by heap_update() to point to the new tuple version. The behavior of AFTER UPDATE triggers was therefore broken, with NEW.* and OLD.* tuples spuriously identical within AFTER UPDATE triggers. To fix both issues, pass a pointer to t_self of a on-stack HeapTuple instead. Fixing this bug lead to one change in regression tests, which previously failed due to the first issue mentioned above. There's a reasonable expectation that test fails, as it updates one row repeatedly within one INSERT ... ON CONFLICT statement. That is only possible if the second update is triggered via ON CONFLICT ... SET, ON CONFLICT ... WHERE, or by a WITH CHECK expression, as those are executed after ExecOnConflictUpdate() does a visibility check. That could easily be prohibited, but given it's allowed for plain UPDATEs and a rare corner case, it doesn't seem worthwhile. Reported-By: Stanislav Grozev Author: Andres Freund and Peter Geoghegan Discussion: CAA78GVqy1+LisN-8DygekD_Ldfy=BJLarSpjGhytOsgkpMavfQ@mail.gmail.com Backpatch: 9.5, where ON CONFLICT was introduced
1 parent e3f4cfc commit 84ac126

File tree

5 files changed

+25
-11
lines changed

5 files changed

+25
-11
lines changed

‎src/backend/executor/nodeModifyTable.c

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1181,8 +1181,17 @@ ExecOnConflictUpdate(ModifyTableState *mtstate,
11811181
/* Project the new tuple version */
11821182
ExecProject(resultRelInfo->ri_onConflictSetProj, NULL);
11831183

1184+
/*
1185+
* Note that it is possible that the target tuple has been modified in
1186+
* this session, after the above heap_lock_tuple. We choose to not error
1187+
* out in that case, in line with ExecUpdate's treatment of similar
1188+
* cases. This can happen if an UPDATE is triggered from within
1189+
* ExecQual(), ExecWithCheckOptions() or ExecProject() above, e.g. by
1190+
* selecting from a wCTE in the ON CONFLICT's SET.
1191+
*/
1192+
11841193
/* Execute UPDATE with projection */
1185-
*returning = ExecUpdate(&tuple.t_data->t_ctid, NULL,
1194+
*returning = ExecUpdate(&tuple.t_self, NULL,
11861195
mtstate->mt_conflproj, planSlot,
11871196
&mtstate->mt_epqstate, mtstate->ps.state,
11881197
canSetTag);

‎src/test/regress/expected/triggers.out

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1703,7 +1703,7 @@ create function upsert_after_func()
17031703
$$
17041704
begin
17051705
if (TG_OP = 'UPDATE') then
1706-
raise warning 'after update (old): %', new.*::text;
1706+
raise warning 'after update (old): %', old.*::text;
17071707
raise warning 'after update (new): %', new.*::text;
17081708
elsif (TG_OP = 'INSERT') then
17091709
raise warning 'after insert (new): %', new.*::text;
@@ -1724,7 +1724,7 @@ insert into upsert values(3, 'orange') on conflict (key) do update set color = '
17241724
WARNING: before insert (new): (3,orange)
17251725
WARNING: before update (old): (3,"red trig modified")
17261726
WARNING: before update (new): (3,"updated red trig modified")
1727-
WARNING: after update (old): (3,"updated red trig modified")
1727+
WARNING: after update (old): (3,"red trig modified")
17281728
WARNING: after update (new): (3,"updated red trig modified")
17291729
insert into upsert values(4, 'green') on conflict (key) do update set color = 'updated ' || upsert.color;
17301730
WARNING: before insert (new): (4,green)
@@ -1734,7 +1734,7 @@ insert into upsert values(5, 'purple') on conflict (key) do update set color = '
17341734
WARNING: before insert (new): (5,purple)
17351735
WARNING: before update (old): (5,"green trig modified")
17361736
WARNING: before update (new): (5,"updated green trig modified")
1737-
WARNING: after update (old): (5,"updated green trig modified")
1737+
WARNING: after update (old): (5,"green trig modified")
17381738
WARNING: after update (new): (5,"updated green trig modified")
17391739
insert into upsert values(6, 'white') on conflict (key) do update set color = 'updated ' || upsert.color;
17401740
WARNING: before insert (new): (6,white)
@@ -1744,7 +1744,7 @@ insert into upsert values(7, 'pink') on conflict (key) do update set color = 'up
17441744
WARNING: before insert (new): (7,pink)
17451745
WARNING: before update (old): (7,"white trig modified")
17461746
WARNING: before update (new): (7,"updated white trig modified")
1747-
WARNING: after update (old): (7,"updated white trig modified")
1747+
WARNING: after update (old): (7,"white trig modified")
17481748
WARNING: after update (new): (7,"updated white trig modified")
17491749
insert into upsert values(8, 'yellow') on conflict (key) do update set color = 'updated ' || upsert.color;
17501750
WARNING: before insert (new): (8,yellow)

‎src/test/regress/expected/with.out

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1875,8 +1875,9 @@ ON CONFLICT (k) DO UPDATE SET v = (SELECT b || ' update' FROM aa WHERE a = 'a' L
18751875
WITH aa AS (SELECT 1 a, 2 b)
18761876
INSERT INTO z VALUES(1, (SELECT b || ' insert' FROM aa WHERE a = 1 ))
18771877
ON CONFLICT (k) DO UPDATE SET v = (SELECT b || ' update' FROM aa WHERE a = 1 LIMIT 1);
1878-
-- This shows an attempt to update an invisible row, which should really be
1879-
-- reported as a cardinality violation, but it doesn't seem worth fixing:
1878+
-- Update a row more than once, in different parts of a wCTE. That is
1879+
-- an allowed, presumably very rare, edge case, but since it was
1880+
-- broken in the past, having a test seems worthwhile.
18801881
WITH simpletup AS (
18811882
SELECT 2 k, 'Green' v),
18821883
upsert_cte AS (
@@ -1886,7 +1887,10 @@ upsert_cte AS (
18861887
INSERT INTO z VALUES(2, 'Red') ON CONFLICT (k) DO
18871888
UPDATE SET (k, v) = (SELECT k, v FROM upsert_cte WHERE upsert_cte.k = z.k)
18881889
RETURNING k, v;
1889-
ERROR: attempted to update invisible tuple
1890+
k | v
1891+
---+---
1892+
(0 rows)
1893+
18901894
DROP TABLE z;
18911895
-- check that run to completion happens in proper ordering
18921896
TRUNCATE TABLE y;

‎src/test/regress/sql/triggers.sql

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1215,7 +1215,7 @@ create function upsert_after_func()
12151215
$$
12161216
begin
12171217
if (TG_OP = 'UPDATE') then
1218-
raise warning 'after update (old): %', new.*::text;
1218+
raise warning 'after update (old): %', old.*::text;
12191219
raise warning 'after update (new): %', new.*::text;
12201220
elsif (TG_OP = 'INSERT') then
12211221
raise warning 'after insert (new): %', new.*::text;

‎src/test/regress/sql/with.sql

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -838,8 +838,9 @@ WITH aa AS (SELECT 1 a, 2 b)
838838
INSERT INTO z VALUES(1, (SELECT b || ' insert' FROM aa WHERE a = 1 ))
839839
ON CONFLICT (k) DO UPDATE SET v = (SELECT b || ' update' FROM aa WHERE a = 1 LIMIT 1);
840840

841-
-- This shows an attempt to update an invisible row, which should really be
842-
-- reported as a cardinality violation, but it doesn't seem worth fixing:
841+
-- Update a row more than once, in different parts of a wCTE. That is
842+
-- an allowed, presumably very rare, edge case, but since it was
843+
-- broken in the past, having a test seems worthwhile.
843844
WITH simpletup AS (
844845
SELECT 2 k, 'Green' v),
845846
upsert_cte AS (

0 commit comments

Comments
 (0)