Decoding speculative insert with toast leaks memory

>
> but by then we might have reused the toast_hash and thus can not be
> destroyed. But that isn't the problem since the reused toast_hash will
> be destroyed eventually.
>
> It's only when the change next to speculative insert is something
> other than INSERT/UPDATE/DELETE that we have to worry about a
> speculative insert that was never confirmed. So may be for those
> cases, we check whether specinsert != null and destroy toast_hash if
> it exists.
>

>
> On Wed, Mar 24, 2021 at 10:34 PM Ashutosh Bapat
> < > wrote:
> > Hi All,
> > We saw OOM in a system where WAL sender consumed Gigabttes of memory
> > which was never released. Upon investigation, we found out that there
> > were many ReorderBufferToastHash memory contexts linked to
> > ReorderBuffer context, together consuming gigs of memory. They were
> > running INSERT ... ON CONFLICT .. among other things. A similar report
> > at [1]
>
> What is the relationship between this bug and commit 7259736a6e5,
> dealt specifically with TOAST and speculative insertion resource
> management issues within reorderbuffer.c? Amit?
>

>
> On Thu, Mar 25, 2021 at 11:04 AM Ashutosh Bapat
> < > wrote:
> >
> > Hi All,
> > We saw OOM in a system where WAL sender consumed Gigabttes of memory
> > which was never released. Upon investigation, we found out that there
> > were many ReorderBufferToastHash memory contexts linked to
> > ReorderBuffer context, together consuming gigs of memory. They were
> > running INSERT ... ON CONFLICT .. among other things. A similar report
> > at [1]
> >
> ..
> >
> > but by then we might have reused the toast_hash and thus can not be
> > destroyed. But that isn't the problem since the reused toast_hash will
> > be destroyed eventually.
> >
> > It's only when the change next to speculative insert is something
> > other than INSERT/UPDATE/DELETE that we have to worry about a
> > speculative insert that was never confirmed. So may be for those
> > cases, we check whether specinsert != null and destroy toast_hash if
> > it exists.
> >
>
> Can we consider the possibility to destroy the toast_hash in
> ReorderBufferCleanupTXN/ReorderBufferTruncateTXN? It will delay the
> clean up of memory till the end of stream or txn but there won't be
> any memory leak.
>

>
> On Thu, May 27, 2021 at 9:26 AM Amit Kapila < > wrote:
> >
> > >
> > > Can we consider the possibility to destroy the toast_hash in
> > > ReorderBufferCleanupTXN/ReorderBufferTruncateTXN? It will delay the
> > > clean up of memory till the end of stream or txn but there won't be
> > > any memory leak.
> > >
> >
> > The other possibility could be to clean it up when we clean the spec
> > insert change in the below code:
>
> Yeah that could be done.
>
> > /*
> > * There's a speculative insertion remaining, just clean in up, it
> > * can't have been successful, otherwise we'd gotten a confirmation
> > * record.
> > */
> > if (specinsert)
> > {
> > ReorderBufferReturnChange(rb, specinsert, true);
> > specinsert = NULL;
> > }
> >
> > But I guess we might miss cleaning it up in case of an error. A
> > similar problem could be there in the idea where we will try to tie
> > the clean up with the next change.
>
> In error case also we can handle it in the CATCH block no?
>

>
> On 5/28/21 2:17 PM, Dilip Kumar wrote:
> > On Fri, May 28, 2021 at 5:16 PM Tomas Vondra
> > < > wrote:
> >> On 5/27/21 6:36 AM, Dilip Kumar wrote:
> >>> On Thu, May 27, 2021 at 9:47 AM Amit Kapila < > wrote:
> >>>>
> >>>> On Thu, May 27, 2021 at 9:40 AM Dilip Kumar < > 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?
>

>
> On 5/29/21 6:29 AM, Amit Kapila wrote:
> > On Fri, May 28, 2021 at 5:16 PM Tomas Vondra
> > < > wrote:
> >>
> >> 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).
> >>
> >
> > IIUC we are anyway freeing the toasted data at the next
> > insert/update/delete. We can try to free at other change message types
> > like REORDER_BUFFER_CHANGE_MESSAGE but as you said that may make the
> > patch more complex, so it seems better to do the fix on the lines of
> > what is proposed in the patch.
> >
>
> +1
>
> Even if we started doing what you mention (freeing the hash for other
> change types), we'd still need to do what the patch proposes because the
> speculative insert may be the last change in the transaction. For the
> other cases it works as a mitigation, so that we don't leak the memory
> forever.
>