Re: Decoding speculative insert with toast leaks memory

On Thu, May 27, 2021 at 9:02 AM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Thu, Mar 25, 2021 at 11:04 AM Ashutosh Bapat
> <ashutosh(dot)bapat(dot)oss(at)gmail(dot)com> 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.
>

The other possibility could be to clean it up when we clean the spec
insert change in the below code:
/*
* 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.

--
With Regards,
Amit Kapila.