Re: Decoding speculative insert with toast leaks memory

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.

> 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.

--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com