Should setting TupleTableSlotOps get_heap_tuple=NULL break INSERT..ON CONFLICT DO UPDATE..RETURNING?

Hackers,

Per the documentation in TupleTableSlotOps, an AM can choose not to supply a get_heap_tuple function, and instead set this field to NULL. Doing so appears to almost work, but breaks the xmin and xmax returned by a INSERT..ON CONFLICT DO UPDATE..RETURNING. In particular, the call chain ExecOnConflictUpdate -> ExecUpdate -> table_tuple_update seems to expect that upon return from table_tuple_update, the slot will hold onto a copy of the updated tuple, including its header fields. This assumption is inherent in how the slot is later used by the destination receiver. But for TAMs which do not keep a copy heaptuple of their own, the slot will only have copies of (tts_tupleDescriptor, tts_values, tts_isnull) to use to form up a tuple when the receiver asks for one, and that formed up MinimalTuple won't be preceded by any meaningful header.

I would expect similar problems for an UPDATE..RETURNING, but have not tested that yet.

I'd like to know if others agree with my analysis, and if this is a bug in the RETURNING, or just an unsupported way to design a custom TAM. If the latter, is this documented somewhere? For reference, I am working against REL_14_STABLE.

Details....

To illustrate this issue, I expanded the update.sql a little to give a bit more information, which demonstrates that the xmin and xmax returned are not the same as what gets written to the table for the same row, using a custom TAM named "pile" which neglects to provide a get_heap_update implementation:

SELECT tableoid::regclass, xmin, xmax, pg_current_xact_id()::xid, a, b FROM upsert_test;
tableoid | xmin | xmax | pg_current_xact_id | a | b
-------------+------+------+--------------------+---+---------------------------
upsert_test | 756 | 756 | 757 | 1 | Foo, Correlated, Excluded
upsert_test | 756 | 756 | 757 | 3 | Zoo, Correlated, Excluded
(2 rows)

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, xmax, pg_current_xact_id()::xid, xmin = pg_current_xact_id()::xid AS xmin_correct, xmax = 0 AS xmax_correct;
tableoid | xmin | xmax | pg_current_xact_id | xmin_correct | xmax_correct
-------------+------+------------+--------------------+--------------+--------------
upsert_test | 140 | 4294967295 | 758 | f | f
(1 row)

SELECT tableoid::regclass, xmin, xmax, pg_current_xact_id()::xid, a, b FROM upsert_test;
tableoid | xmin | xmax | pg_current_xact_id | a | b
-------------+------+------+--------------------+---+---------------------------
upsert_test | 756 | 756 | 759 | 1 | Foo, Correlated, Excluded
upsert_test | 756 | 756 | 759 | 3 | Zoo, Correlated, Excluded
upsert_test | 758 | 0 | 759 | 2 | Beeble
(3 rows)

Adding a bogus Assert I can get the following stack trace, showing in frame 4 tts_buffer_pile_copy_heap_tuple is called (rather than the tts_buffer_pile_get_heap_tuple which was called in this location prior to changing the get_heap_tuple to NULL). In frame 6, pileam_tuple_update is going to see that shouldFree is true and will free the slot's tuple, so the slot's copy won't be valid by the time the dest receiver wants it. That will force a tts_pile_materialize call, but since the slot's tuple will not be vaild, the materialize will operate by forming a tuple from the (descriptor,values,isnull) triple, rather than by copying a tuple, and the pile_form_tuple call won't do anything to set the tuple header fields.

* thread #1, stop reason = signal SIGSTOP
* frame #0: 0x00007fff70ea632a libsystem_kernel.dylib`__pthread_kill + 10
frame #1: 0x00007fff70f62e60 libsystem_pthread.dylib`pthread_kill + 430
frame #2: 0x00007fff70e2d808 libsystem_c.dylib`abort + 120
frame #3: 0x000000010c992251 postgres`ExceptionalCondition(conditionName="false", errorType="FailedAssertion", fileName="access/pile_slotops.c", lineNumber=419) at assert.c:69:2
frame #4: 0x000000010d33f8b8 pile.so`tts_buffer_pile_copy_heap_tuple(slot=0x00007f9edb02c550) at pile_slotops.c:419:3
frame #5: 0x000000010d33e2f7 pile.so`ExecFetchSlotPileTuple(slot=0x00007f9edb02c550, materialize=true, shouldFree=0x00007ffee3aa3ace) at pile_slotops.c:639:32
frame #6: 0x000000010d35bccc pile.so`pileam_tuple_update(relation=0x00007f9ed0083c58, otid=0x00007ffee3aa3f30, slot=0x00007f9edb02c550, cid=0, snapshot=0x00007f9edd04b7f0, crosscheck=0x0000000000000000, wait=true, tmfd=0x00007ffee3aa3cf8, lockmode=0x00007ffee3aa3ce8, update_indexes=0x00007ffee3aa3ce6) at pileam_handler.c:327:20
frame #7: 0x000000010c51bcad postgres`table_tuple_update(rel=0x00007f9ed0083c58, otid=0x00007ffee3aa3f30, slot=0x00007f9edb02c550, cid=0, snapshot=0x00007f9edd04b7f0, crosscheck=0x0000000000000000, wait=true, tmfd=0x00007ffee3aa3cf8, lockmode=0x00007ffee3aa3ce8, update_indexes=0x00007ffee3aa3ce6) at tableam.h:1509:9
frame #8: 0x000000010c518ec7 postgres`ExecUpdate(mtstate=0x00007f9edd0acd40, resultRelInfo=0x00007f9edd0acf58, tupleid=0x00007ffee3aa3f30, oldtuple=0x0000000000000000, slot=0x00007f9edb02c550, planSlot=0x00007f9edd0ad540, epqstate=0x00007f9edd0ace28, estate=0x00007f9edd0abd20, canSetTag=true) at nodeModifyTable.c:1809:12
frame #9: 0x000000010c51b187 postgres`ExecOnConflictUpdate(mtstate=0x00007f9edd0acd40, resultRelInfo=0x00007f9edd0acf58, conflictTid=0x00007ffee3aa3f30, planSlot=0x00007f9edd0ad540, excludedSlot=0x00007f9edb02ec40, estate=0x00007f9edd0abd20, canSetTag=true, returning=0x00007ffee3aa3f18) at nodeModifyTable.c:2199:15
frame #10: 0x000000010c518453 postgres`ExecInsert(mtstate=0x00007f9edd0acd40, resultRelInfo=0x00007f9edd0acf58, slot=0x00007f9edb02ec40, planSlot=0x00007f9edd0ad540, estate=0x00007f9edd0abd20, canSetTag=true) at nodeModifyTable.c:870:10
frame #11: 0x000000010c516fd4 postgres`ExecModifyTable(pstate=0x00007f9edd0acd40) at nodeModifyTable.c:2583:12
frame #12: 0x000000010c4d4862 postgres`ExecProcNodeFirst(node=0x00007f9edd0acd40) at execProcnode.c:464:9
frame #13: 0x000000010c4cc6d2 postgres`ExecProcNode(node=0x00007f9edd0acd40) at executor.h:257:9
frame #14: 0x000000010c4c7d21 postgres`ExecutePlan(estate=0x00007f9edd0abd20, planstate=0x00007f9edd0acd40, use_parallel_mode=false, operation=CMD_INSERT, sendTuples=true, numberTuples=0, direction=ForwardScanDirection, dest=0x00007f9edc00b458, execute_once=true) at execMain.c:1551:10
frame #15: 0x000000010c4c7bf1 postgres`standard_ExecutorRun(queryDesc=0x00007f9edc00b4f0, direction=ForwardScanDirection, count=0, execute_once=true) at execMain.c:361:3
frame #16: 0x000000010c4c7982 postgres`ExecutorRun(queryDesc=0x00007f9edc00b4f0, direction=ForwardScanDirection, count=0, execute_once=true) at execMain.c:305:3
frame #17: 0x000000010c7930dc postgres`ProcessQuery(plan=0x00007f9edb021fc0, sourceText="WITH aaa AS (SELECT 1 AS a, 'Foo' AS b) INSERT INTO upsert_test\n VALUES (1, 'Bar') ON CONFLICT(a)\n DO UPDATE SET (b, a) = (SELECT b, a FROM aaa) RETURNING *;", params=0x0000000000000000, queryEnv=0x0000000000000000, dest=0x00007f9edc00b458, qc=0x00007ffee3aa4408) at pquery.c:160:2
frame #18: 0x000000010c791f07 postgres`PortalRunMulti(portal=0x00007f9edd028920, isTopLevel=true, setHoldSnapshot=true, dest=0x00007f9edc00b458, altdest=0x000000010cbc8890, qc=0x00007ffee3aa4408) at pquery.c:1274:5
frame #19: 0x000000010c791835 postgres`FillPortalStore(portal=0x00007f9edd028920, isTopLevel=true) at pquery.c:1023:4
frame #20: 0x000000010c7913ee postgres`PortalRun(portal=0x00007f9edd028920, count=9223372036854775807, isTopLevel=true, run_once=true, dest=0x00007f9edb0220b0, altdest=0x00007f9edb0220b0, qc=0x00007ffee3aa46a0) at pquery.c:760:6
frame #21: 0x000000010c78c394 postgres`exec_simple_query(query_string="WITH aaa AS (SELECT 1 AS a, 'Foo' AS b) INSERT INTO upsert_test\n VALUES (1, 'Bar') ON CONFLICT(a)\n DO UPDATE SET (b, a) = (SELECT b, a FROM aaa) RETURNING *;") at postgres.c:1213:10
frame #22: 0x000000010c78b3f7 postgres`PostgresMain(argc=1, argv=0x00007ffee3aa49d0, dbname="contrib_regression", username="mark.dilger") at postgres.c:4496:7
frame #23: 0x000000010c692a59 postgres`BackendRun(port=0x00007f9edc804080) at postmaster.c:4530:2
frame #24: 0x000000010c691fa5 postgres`BackendStartup(port=0x00007f9edc804080) at postmaster.c:4252:3
frame #25: 0x000000010c690d0e postgres`ServerLoop at postmaster.c:1745:7
frame #26: 0x000000010c68e23a postgres`PostmasterMain(argc=8, argv=0x00007f9edac06440) at postmaster.c:1417:11
frame #27: 0x000000010c565249 postgres`main(argc=8, argv=0x00007f9edac06440) at main.c:209:3
frame #28: 0x00007fff70d5ecc9 libdyld.dylib`start + 1
frame #29: 0x00007fff70d5ecc9 libdyld.dylib`start + 1

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company