Re: Decoding speculative insert with toast leaks memory

On 5/27/21 6:36 AM, Dilip Kumar wrote:
> On Thu, May 27, 2021 at 9:47 AM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>>
>> On Thu, May 27, 2021 at 9:40 AM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
>>
>> True, but if you do this clean-up in ReorderBufferCleanupTXN then you
>> don't need to take care at separate places. Also, toast_hash is stored
>> in txn so it appears natural to clean it up in while releasing TXN.
>
> Make sense, basically, IMHO we will have to do in TruncateTXN and
> ReturnTXN as attached?
>

Yeah, I've been working on a fix over the last couple days (because of a
customer issue), and I ended up with the reset in ReorderBufferReturnTXN
too - it does solve the issue in the case I've been investigating.

I'm not sure the reset in ReorderBufferTruncateTXN is correct, though.
Isn't it possible that we'll need the TOAST data after streaming part of
the transaction? After all, we're not resetting invalidations, tuplecids
and snapshot either ... And we'll eventually clean it after the streamed
transaction gets commited (ReorderBufferStreamCommit ends up calling
ReorderBufferReturnTXN too).

I wonder if there's a way to free the TOASTed data earlier, instead of
waiting until the end of the transaction (as this patch does). But I
suspect it'd be way more complex, harder to backpatch, and destroying
the hash table is a good idea anyway.

Also, I think the "if (txn->toast_hash != NULL)" checks are not needed,
because it's the first thing ReorderBufferToastReset does.

regards

--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company