Обсуждение: 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
thisfield 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
expectthat upon return from table_tuple_update, the slot will hold onto a copy of the updated tuple, including its
headerfields.  This assumption is inherent in how the slot is later used by the destination receiver.  But for TAMs
whichdo 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
byany 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
designa 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
thexmin 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
(ratherthan the tts_buffer_pile_get_heap_tuple which was called in this location prior to changing the get_heap_tuple
toNULL).  In frame 6, pileam_tuple_update is going to see that shouldFree is true and will free the slot's tuple, so
theslot's copy won't be valid by the time the dest receiver wants it.  That will force a tts_pile_materialize call, but
sincethe 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
thetuple 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)
INSERTINTO 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

Hi,

On 2022-09-29 09:04:42 -0700, Mark Dilger wrote:
> 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 assume that this can be avoided by the tuple slot implementation, but
without seeing what precisely you did in your pile slot...

Greetings,

Andres Freund

> On Sep 29, 2022, at 9:22 AM, Andres Freund <andres@anarazel.de> wrote:
>
> I would assume that this can be avoided by the tuple slot implementation, but
> without seeing what precisely you did in your pile slot...

"pile" is just a copy of "heap" placed into an extension with a slightly smarter version of s/heap/pile/g performed
acrossthe sources.  It is intended to behave exactly as heap does.  Without disabling the get_heap_tuple function, it
passesa wide variety of the regression/isolation/tap tests.  To test the claim made in the TupleTableSlotOps code
comments,I disabled that one function: 

    /*
     * Return a heap tuple "owned" by the slot. It is slot's responsibility to
     * free the memory consumed by the heap tuple. If the slot can not "own" a
     * heap tuple, it should not implement this callback and should set it as
     * NULL.
     */
    HeapTuple   (*get_heap_tuple) (TupleTableSlot *slot);

That comment suggests that I do not need to keep a copy of the heap tuple, and per the next comment:

    /*
     * Return a copy of heap tuple representing the contents of the slot. The
     * copy needs to be palloc'd in the current memory context. The slot
     * itself is expected to remain unaffected. It is *not* expected to have
     * meaningful "system columns" in the copy. The copy is not be "owned" by
     * the slot i.e. the caller has to take responsibility to free memory
     * consumed by the slot.
     */
    HeapTuple   (*copy_heap_tuple) (TupleTableSlot *slot);

I do not need to keep a copy of the "system columns".  But clearly this doesn't work.  When get_heap_tuple=NULL, the
AM'stuple_update is at liberty to free the update tuple (per the above documentation) and later return a copy of the
slot'stuple sans any "system columns" (also per the above documentation) and that's when the core code breaks.  It's
notthe TAM that is broken here, not according to the interface's documentation as I read it.  Am I reading it wrong? 



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