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

From: Mark Dilger <mark(dot)dilger(at)enterprisedb(dot)com>
To: Andres Freund <andres(at)anarazel(dot)de>
Cc: PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>
Subject: Re: Should setting TupleTableSlotOps get_heap_tuple=NULL break INSERT..ON CONFLICT DO UPDATE..RETURNING?
Date: 2022-09-29 16:40:02
Message-ID: 549147E3-F866-4CC0-9821-8CA1706AF90A@enterprisedb.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

> On Sep 29, 2022, at 9:22 AM, Andres Freund <andres(at)anarazel(dot)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 across the sources. It is intended to behave exactly as heap does. Without disabling the get_heap_tuple function, it passes a 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's tuple_update is at liberty to free the update tuple (per the above documentation) and later return a copy of the slot's tuple sans any "system columns" (also per the above documentation) and that's when the core code breaks. It's not the 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

In response to

Browse pgsql-hackers by date

  From Date Subject
Next Message Bharath Rupireddy 2022-09-29 17:08:33 Re: Avoid memory leaks during base backups
Previous Message Matthias van de Meent 2022-09-29 16:24:12 Re: problems with making relfilenodes 56-bits