PG-1056 xmin and xmax correctness (#292)

* PG-1056 Add failing test

* PG-1056 Use proper AM in test

* Fix UPDATE SET ... RETURNING processing for encrypted tuples

If `get_heap_tuple` is NULL, the core uses `copy_heap_tuple` instead. The former returns a pointer to a tuple in the slot and the latter makes a copy of such a tuple. For UPDATE SET, the core uses the slot for INSERT and later for RETURNING processing. If we copy the tuple the next happens:
1. The core creates a slot with the generic tuple.
2. It passed to `pg_tdeam_tuple_update()` and it gets a copy of the tuple here [https://github.com/Percona-Lab/pg_tde/blob/6d4f7e5b7bd2507ce65deb315ea8d5647f474cf9/src17/access/pg_tdeam_handler.c#L336].
3. This generic tuple is filled with the proper data and used for the update here [https://github.com/Percona-Lab/pg_tde/blob/6d4f7e5b7bd2507ce65deb315ea8d5647f474cf9/src17/access/pg_tdeam_handler.c#L343].
4. Later on, RETURNING processing uses the slot's tuple but is still a  generic unmodified one because of the copy.
5. That results in wrong RETURNING data.

To avoid this, we should return a pointer to the slot's tuple instead of copying it.

Fixes PG-1056

* PG-1056 Split 'update' testcase for tde_heap and tde_heap_basic

---------

Co-authored-by: Andrew Pogrebnoy <absourd.noise@gmail.com>

Makefile.in

@@ -14,6 +14,7 @@ update_compare_indexes_basic \
 pg_tde_is_encrypted_basic \
 test_issue_153_fix_basic \
 multi_insert_basic \
+update_basic \
 subtransaction_basic \
 trigger_on_view_basic \
 change_access_method_basic \

expected/update.out

@@ -0,0 +1,47 @@
+\set tde_am tde_heap
+\i sql/update.inc
+CREATE EXTENSION pg_tde;
+SELECT pg_tde_add_key_provider_file('file-vault','/tmp/pg_tde_test_keyring.per');
+ pg_tde_add_key_provider_file 
+------------------------------
+                            1
+(1 row)
+
+SELECT pg_tde_set_principal_key('test-db-principal-key','file-vault');
+ pg_tde_set_principal_key 
+--------------------------
+ t
+(1 row)
+
+CREATE TABLE update_test (
+    a   INT DEFAULT 10,
+    b   INT,
+    c   TEXT
+) USING tde_heap_basic;
+CREATE TABLE upsert_test (
+    a   INT PRIMARY KEY,
+    b   TEXT
+) USING tde_heap_basic;
+INSERT INTO update_test VALUES (5, 10, 'foo');
+INSERT INTO update_test(b, a) VALUES (15, 10);
+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 = pg_current_xact_id()::xid 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 = pg_current_xact_id()::xid AS xmin_correct, xmax = pg_current_xact_id()::xid AS xmax_correct;
+  tableoid   | xmin_correct | xmax_correct 
+-------------+--------------+--------------
+ upsert_test | t            | t
+(1 row)
+
+DROP TABLE update_test;
+DROP TABLE upsert_test;
+DROP EXTENSION pg_tde;

expected/update_basic.out

@@ -0,0 +1,47 @@
+\set tde_am tde_heap_basic
+\i sql/update.inc
+CREATE EXTENSION pg_tde;
+SELECT pg_tde_add_key_provider_file('file-vault','/tmp/pg_tde_test_keyring.per');
+ pg_tde_add_key_provider_file 
+------------------------------
+                            1
+(1 row)
+
+SELECT pg_tde_set_principal_key('test-db-principal-key','file-vault');
+ pg_tde_set_principal_key 
+--------------------------
+ t
+(1 row)
+
+CREATE TABLE update_test (
+    a   INT DEFAULT 10,
+    b   INT,
+    c   TEXT
+) USING tde_heap_basic;
+CREATE TABLE upsert_test (
+    a   INT PRIMARY KEY,
+    b   TEXT
+) USING tde_heap_basic;
+INSERT INTO update_test VALUES (5, 10, 'foo');
+INSERT INTO update_test(b, a) VALUES (15, 10);
+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 = pg_current_xact_id()::xid 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 = pg_current_xact_id()::xid AS xmin_correct, xmax = pg_current_xact_id()::xid AS xmax_correct;
+  tableoid   | xmin_correct | xmax_correct 
+-------------+--------------+--------------
+ upsert_test | t            | t
+(1 row)
+
+DROP TABLE update_test;
+DROP TABLE upsert_test;
+DROP EXTENSION pg_tde;

meson.build

@@ -106,6 +106,7 @@ if get_variable('percona_ext', false)
       'move_large_tuples',
       'non_sorted_off_compact',
       'update_compare_indexes',
+      'update',
       'pg_tde_is_encrypted',
       'test_issue_153_fix',
       'multi_insert',

sql/update.inc

@@ -0,0 +1,33 @@
+CREATE EXTENSION pg_tde;
+
+SELECT pg_tde_add_key_provider_file('file-vault','/tmp/pg_tde_test_keyring.per');
+SELECT pg_tde_set_principal_key('test-db-principal-key','file-vault');
+
+
+CREATE TABLE update_test (
+    a   INT DEFAULT 10,
+    b   INT,
+    c   TEXT
+) USING tde_heap_basic;
+
+CREATE TABLE upsert_test (
+    a   INT PRIMARY KEY,
+    b   TEXT
+) USING tde_heap_basic;
+
+INSERT INTO update_test VALUES (5, 10, 'foo');
+INSERT INTO update_test(b, a) VALUES (15, 10);
+
+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 = pg_current_xact_id()::xid 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 = pg_current_xact_id()::xid AS xmin_correct, xmax = pg_current_xact_id()::xid AS xmax_correct;
+
+DROP TABLE update_test;
+DROP TABLE upsert_test;
+
+DROP EXTENSION pg_tde;

sql/update.sql

@@ -0,0 +1,2 @@
+\set tde_am tde_heap
+\i sql/update.inc

sql/update_basic.sql

@@ -0,0 +1,2 @@
+\set tde_am tde_heap_basic
+\i sql/update.inc

src/access/pg_tde_slot.c

@@ -241,6 +241,17 @@ tdeheap_tts_buffer_heap_copyslot(TupleTableSlot *dstslot, TupleTableSlot *srcslo
 	}
 }
 
+static HeapTuple
+tdeheap_tts_buffer_heap_get_heap_tuple(TupleTableSlot *slot)
+{
+	BufferHeapTupleTableSlot *bslot = (BufferHeapTupleTableSlot *) slot;
+	Assert(!TTS_EMPTY(slot));
+
+	if (!bslot->base.tuple)
+		tdeheap_tts_buffer_heap_materialize(slot);
+	return bslot->base.tuple;
+}
+
 static HeapTuple
 tdeheap_tts_buffer_heap_copy_heap_tuple(TupleTableSlot *slot)
 {
@@ -462,7 +473,7 @@ const TupleTableSlotOps TTSOpsTDEBufferHeapTuple = {
 	.is_current_xact_tuple = tdeheap_buffer_is_current_xact_tuple,
 #endif
 	.copyslot = tdeheap_tts_buffer_heap_copyslot,
-	.get_heap_tuple = NULL,
+	.get_heap_tuple = tdeheap_tts_buffer_heap_get_heap_tuple,
 
 	/* A buffer heap tuple table slot can not "own" a minimal tuple. */
 	.get_minimal_tuple = NULL,