Skip to content

Commit 34263e8

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 5b51805 commit 34263e8

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
@@ -1771,7 +1771,7 @@ create function upsert_after_func()
17711771
$$
17721772
begin
17731773
if (TG_OP = 'UPDATE') then
1774-
raise warning 'after update (old): %', new.*::text;
1774+
raise warning 'after update (old): %', old.*::text;
17751775
raise warning 'after update (new): %', new.*::text;
17761776
elsif (TG_OP = 'INSERT') then
17771777
raise warning 'after insert (new): %', new.*::text;
@@ -1792,7 +1792,7 @@ insert into upsert values(3, 'orange') on conflict (key) do update set color = '
17921792
WARNING: before insert (new): (3,orange)
17931793
WARNING: before update (old): (3,"red trig modified")
17941794
WARNING: before update (new): (3,"updated red trig modified")
1795-
WARNING: after update (old): (3,"updated red trig modified")
1795+
WARNING: after update (old): (3,"red trig modified")
17961796
WARNING: after update (new): (3,"updated red trig modified")
17971797
insert into upsert values(4, 'green') on conflict (key) do update set color = 'updated ' || upsert.color;
17981798
WARNING: before insert (new): (4,green)
@@ -1802,7 +1802,7 @@ insert into upsert values(5, 'purple') on conflict (key) do update set color = '
18021802
WARNING: before insert (new): (5,purple)
18031803
WARNING: before update (old): (5,"green trig modified")
18041804
WARNING: before update (new): (5,"updated green trig modified")
1805-
WARNING: after update (old): (5,"updated green trig modified")
1805+
WARNING: after update (old): (5,"green trig modified")
18061806
WARNING: after update (new): (5,"updated green trig modified")
18071807
insert into upsert values(6, 'white') on conflict (key) do update set color = 'updated ' || upsert.color;
18081808
WARNING: before insert (new): (6,white)
@@ -1812,7 +1812,7 @@ insert into upsert values(7, 'pink') on conflict (key) do update set color = 'up
18121812
WARNING: before insert (new): (7,pink)
18131813
WARNING: before update (old): (7,"white trig modified")
18141814
WARNING: before update (new): (7,"updated white trig modified")
1815-
WARNING: after update (old): (7,"updated white trig modified")
1815+
WARNING: after update (old): (7,"white trig modified")
18161816
WARNING: after update (new): (7,"updated white trig modified")
18171817
insert into upsert values(8, 'yellow') on conflict (key) do update set color = 'updated ' || upsert.color;
18181818
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)