Re: Decoding speculative insert with toast leaks memory

On Fri, May 28, 2021 at 6:01 PM Tomas Vondra
<tomas(dot)vondra(at)enterprisedb(dot)com> wrote:
>
> 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?
>

There is already an Assert for this. See ReorderBufferCheckMemoryLimit.

--
With Regards,
Amit Kapila.