Re: Decoding speculative insert with toast leaks memory

On 5/28/21 2:17 PM, Dilip Kumar wrote:
> On Fri, May 28, 2021 at 5:16 PM Tomas Vondra
> <tomas(dot)vondra(at)enterprisedb(dot)com> wrote:
>> 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
>
> Actually, as per the current design, we don't need the toast data
> after the streaming. Because currently, we don't allow to stream the
> transaction if we need to keep the toast across stream e.g. if we only
> have toast insert without the main insert we assure this as partial
> changes, another case is if we have multi-insert with toast then we
> keep the transaction as mark partial until we get the last insert of
> the multi-insert. So with the current design we don't have any case
> where we need to keep toast data across streams.
>
> ... And we'll eventually clean it after the streamed
>> transaction gets commited (ReorderBufferStreamCommit ends up calling
>> ReorderBufferReturnTXN too).
>
> Right, but generally after streaming we assert that txn->size should
> be 0. That could be changed if we change the above design but this is
> what it is today.
>

Can we add some assert to enforce this?

>> 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.
>
> Right.
>
>> Also, I think the "if (txn->toast_hash != NULL)" checks are not needed,
>> because it's the first thing ReorderBufferToastReset does.
>
> I see, I will change this. If we all agree with this idea.
>

+1 from me. I think it's good enough to do the cleanup at the end, and
it's an improvement compared to current state. There might be cases of
transactions doing many such speculative inserts and accumulating a lot
of data in the TOAST hash, but I find it very unlikely.

regards

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