Fix system column accesses in ON CONFLICT ... RETURNING.

After 277cb789836 ON CONFLICT ... SET ... RETURNING failed with
ERROR:  virtual tuple table slot does not have system attributes
when taking the update path, as the slot used to insert into the
table (and then process RETURNING) was defined to be a virtual slot in
that commit. Virtual slots don't support system columns except for
tableoid and ctid, as the other system columns are AM dependent.

Fix that by using a slot of the table's type. Add tests for system
column accesses in ON CONFLICT ...  RETURNING.

Reported-By: Roby, bisected to the relevant commit by Jeff Janes
Author: Andres Freund
Discussion: https://postgr.es/m/73436355-6432-49B1-92ED-1FE4F7E7E100@finefun.com.au
Backpatch: 12-, where the bug was introduced in 277cb789836

diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c
index d8b695d897f815708e9f4b5ba6d15cadf9b11491..9e0c8794c4026caae1db7ab8a29d577d01bba48e 100644 (file)
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -2542,11 +2542,16 @@ ExecInitModifyTable(ModifyTable *node, EState *estate, int eflags)
                        table_slot_create(resultRelInfo->ri_RelationDesc,
                                                          &mtstate->ps.state->es_tupleTable);
 
-               /* create the tuple slot for the UPDATE SET projection */
+               /*
+                * Create the tuple slot for the UPDATE SET projection. We want a slot
+                * of the table's type here, because the slot will be used to insert
+                * into the table, and for RETURNING processing - which may access
+                * system attributes.
+                */
                tupDesc = ExecTypeFromTL((List *) node->onConflictSet);
                resultRelInfo->ri_onConflict->oc_ProjSlot =
                        ExecInitExtraTupleSlot(mtstate->ps.state, tupDesc,
-                                                                  &TTSOpsVirtual);
+                                                                  table_slot_callbacks(resultRelInfo->ri_RelationDesc));
 
                /* build UPDATE SET projection state */
                resultRelInfo->ri_onConflict->oc_ProjInfo =

diff --git a/src/test/regress/expected/update.out b/src/test/regress/expected/update.out
index 2083345c8eea970aaddc7105fa6f8efaa35f4b6a..a24ecd61df8fcec586ed532f88dde9ec11f0dd44 100644 (file)
--- a/src/test/regress/expected/update.out
+++ b/src/test/regress/expected/update.out
@@ -227,6 +227,29 @@ INSERT INTO upsert_test VALUES (1, 'Bat') ON CONFLICT(a)
  1 | Foo, Correlated, Excluded
 (1 row)
 
+-- ON CONFLICT using system attributes in RETURNING, testing both the
+-- inserting and updating paths. See bug report at:
+-- https://www.postgresql.org/message-id/73436355-6432-49B1-92ED-1FE4F7E7E100%40finefun.com.au
+CREATE FUNCTION xid_current() RETURNS xid LANGUAGE SQL AS $$SELECT (txid_current() % ((1::int8<<32)))::text::xid;$$;
+INSERT INTO upsert_test VALUES (2, 'Beeble') ON CONFLICT(a)
+  DO UPDATE SET (b, a) = (SELECT b || ', Excluded', a from upsert_test i WHERE i.a = excluded.a)
+  RETURNING tableoid::regclass, xmin = xid_current() AS xmin_correct, xmax = 0 AS xmax_correct;
+  tableoid   | xmin_correct | xmax_correct 
+-------------+--------------+--------------
+ upsert_test | t            | t
+(1 row)
+
+-- currently xmax is set after a conflict - that's probably not good,
+-- but it seems worthwhile to have to be explicit if that changes.
+INSERT INTO upsert_test VALUES (2, 'Brox') ON CONFLICT(a)
+  DO UPDATE SET (b, a) = (SELECT b || ', Excluded', a from upsert_test i WHERE i.a = excluded.a)
+  RETURNING tableoid::regclass, xmin = xid_current() AS xmin_correct, xmax = xid_current() AS xmax_correct;
+  tableoid   | xmin_correct | xmax_correct 
+-------------+--------------+--------------
+ upsert_test | t            | t
+(1 row)
+
+DROP FUNCTION xid_current();
 DROP TABLE update_test;
 DROP TABLE upsert_test;
 ---------------------------

diff --git a/src/test/regress/sql/update.sql b/src/test/regress/sql/update.sql
index 8754ccb7b01a997bb243981c7e205035e0cfe067..bb9c24e40f817bb18630fd8dc4f11a8cf7f9e213 100644 (file)
--- a/src/test/regress/sql/update.sql
+++ b/src/test/regress/sql/update.sql
@@ -114,6 +114,21 @@ INSERT INTO upsert_test VALUES (1, 'Bat') ON CONFLICT(a)
   DO UPDATE SET (b, a) = (SELECT b || ', Excluded', a from upsert_test i WHERE i.a = excluded.a)
   RETURNING *;
 
+-- ON CONFLICT using system attributes in RETURNING, testing both the
+-- inserting and updating paths. See bug report at:
+-- https://www.postgresql.org/message-id/73436355-6432-49B1-92ED-1FE4F7E7E100%40finefun.com.au
+CREATE FUNCTION xid_current() RETURNS xid LANGUAGE SQL AS $$SELECT (txid_current() % ((1::int8<<32)))::text::xid;$$;
+INSERT INTO upsert_test VALUES (2, 'Beeble') ON CONFLICT(a)
+  DO UPDATE SET (b, a) = (SELECT b || ', Excluded', a from upsert_test i WHERE i.a = excluded.a)
+  RETURNING tableoid::regclass, xmin = xid_current() AS xmin_correct, xmax = 0 AS xmax_correct;
+-- currently xmax is set after a conflict - that's probably not good,
+-- but it seems worthwhile to have to be explicit if that changes.
+INSERT INTO upsert_test VALUES (2, 'Brox') ON CONFLICT(a)
+  DO UPDATE SET (b, a) = (SELECT b || ', Excluded', a from upsert_test i WHERE i.a = excluded.a)
+  RETURNING tableoid::regclass, xmin = xid_current() AS xmin_correct, xmax = xid_current() AS xmax_correct;
+
+
+DROP FUNCTION xid_current();
 DROP TABLE update_test;
 DROP TABLE upsert_test;