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

diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c
index dabaea991096998c78d61653569c59c30546bd24..9db4c9174353be58d498091a55e3a26241d61735 100644 (file)
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -1181,8 +1181,17 @@ ExecOnConflictUpdate(ModifyTableState *mtstate,
    /* Project the new tuple version */
    ExecProject(resultRelInfo->ri_onConflictSetProj, NULL);
 
+   /*
+    * Note that it is possible that the target tuple has been modified in
+    * this session, after the above heap_lock_tuple. We choose to not error
+    * out in that case, in line with ExecUpdate's treatment of similar
+    * cases. This can happen if an UPDATE is triggered from within
+    * ExecQual(), ExecWithCheckOptions() or ExecProject() above, e.g. by
+    * selecting from a wCTE in the ON CONFLICT's SET.
+    */
+
    /* Execute UPDATE with projection */
-   *returning = ExecUpdate(&tuple.t_data->t_ctid, NULL,
+   *returning = ExecUpdate(&tuple.t_self, NULL,
                            mtstate->mt_conflproj, planSlot,
                            &mtstate->mt_epqstate, mtstate->ps.state,
                            canSetTag);

diff --git a/src/test/regress/expected/triggers.out b/src/test/regress/expected/triggers.out
index dd4a99bff1bd4421a149fb4ff5af4c269b4b455d..a7bf5dc159d5947fd9226d293c327690590f222e 100644 (file)
--- 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 2c9226c3db58de845a85038f69539400112c2ac1..137420d9b7ea25b5361c9c15a23ebc4d2f42fd34 100644 (file)
--- 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:
+-- Update a row more than once, in different parts of a wCTE. That is
+-- an allowed, presumably very rare, edge case, but since it was
+-- broken in the past, having a test seems worthwhile.
 WITH simpletup AS (
   SELECT 2 k, 'Green' v),
 upsert_cte AS (
@@ -1886,7 +1887,10 @@ 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
+ k | v 
+---+---
+(0 rows)
+
 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 9f66702ceef16b9d44754a9ed0c172f307e8f3b8..b6de1b32560b7c72f58e291143dfc0fc25c2f6cf 100644 (file)
--- 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 3fd55f96b3f617e1cb2757c95d211bb8e7d8fd77..133ff0b19570c212896647af16a5c6893bcacab9 100644 (file)
--- 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:
+-- Update a row more than once, in different parts of a wCTE. That is
+-- an allowed, presumably very rare, edge case, but since it was
+-- broken in the past, having a test seems worthwhile.
 WITH simpletup AS (
   SELECT 2 k, 'Green' v),
 upsert_cte AS (