Decoding speculative insert with toast leaks memory

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
60 messages Options
123
Reply | Threaded
Open this post in threaded view
|

Decoding speculative insert with toast leaks memory

Ashutosh Bapat-2
CONTENTS DELETED
The author has deleted this message.
Reply | Threaded
Open this post in threaded view
|

Re: Decoding speculative insert with toast leaks memory

Peter Geoghegan-4
CONTENTS DELETED
The author has deleted this message.
Reply | Threaded
Open this post in threaded view
|

Re: Decoding speculative insert with toast leaks memory

akapila
In reply to this post by Ashutosh Bapat-2
On Thu, Mar 25, 2021 at 11:04 AM Ashutosh Bapat
<[hidden email]> 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.

--
With Regards,
Amit Kapila.


Reply | Threaded
Open this post in threaded view
|

Re: Decoding speculative insert with toast leaks memory

akapila
In reply to this post by Peter Geoghegan-4
On Thu, May 27, 2021 at 8:27 AM Peter Geoghegan <[hidden email]> wrote:

>
> On Wed, Mar 24, 2021 at 10:34 PM Ashutosh Bapat
> <[hidden email]> 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?
>

This seems to be a pre-existing bug. This should be reproduced in
PG-13 and or prior to that commit. Ashutosh can confirm?

--
With Regards,
Amit Kapila.


Reply | Threaded
Open this post in threaded view
|

Re: Decoding speculative insert with toast leaks memory

akapila
In reply to this post by akapila
On Thu, May 27, 2021 at 9:02 AM Amit Kapila <[hidden email]> wrote:

>
> On Thu, Mar 25, 2021 at 11:04 AM Ashutosh Bapat
> <[hidden email]> 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.


Reply | Threaded
Open this post in threaded view
|

Re: Decoding speculative insert with toast leaks memory

Dilip Kumar-2
In reply to this post by akapila
CONTENTS DELETED
The author has deleted this message.
Reply | Threaded
Open this post in threaded view
|

Re: Decoding speculative insert with toast leaks memory

Dilip Kumar-2
In reply to this post by akapila
CONTENTS DELETED
The author has deleted this message.
Reply | Threaded
Open this post in threaded view
|

Re: Decoding speculative insert with toast leaks memory

akapila
On Thu, May 27, 2021 at 9:40 AM Dilip Kumar <[hidden email]> wrote:

>
> On Thu, May 27, 2021 at 9:26 AM Amit Kapila <[hidden email]> 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?
>

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.

--
With Regards,
Amit Kapila.


Reply | Threaded
Open this post in threaded view
|

Re: Decoding speculative insert with toast leaks memory

Dilip Kumar-2
CONTENTS DELETED
The author has deleted this message.
Reply | Threaded
Open this post in threaded view
|

Re: Decoding speculative insert with toast leaks memory

Tomas Vondra-6
CONTENTS DELETED
The author has deleted this message.
Reply | Threaded
Open this post in threaded view
|

Re: Decoding speculative insert with toast leaks memory

Dilip Kumar-2
CONTENTS DELETED
The author has deleted this message.
Reply | Threaded
Open this post in threaded view
|

Re: Decoding speculative insert with toast leaks memory

Tomas Vondra-6
CONTENTS DELETED
The author has deleted this message.
Reply | Threaded
Open this post in threaded view
|

Re: Decoding speculative insert with toast leaks memory

akapila
On Fri, May 28, 2021 at 6:01 PM Tomas Vondra
<[hidden email]> wrote:

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


Reply | Threaded
Open this post in threaded view
|

Re: Decoding speculative insert with toast leaks memory

akapila
In reply to this post by Tomas Vondra-6
On Fri, May 28, 2021 at 5:16 PM Tomas Vondra
<[hidden email]> 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.

--
With Regards,
Amit Kapila.


Reply | Threaded
Open this post in threaded view
|

Re: Decoding speculative insert with toast leaks memory

Tomas Vondra-6
CONTENTS DELETED
The author has deleted this message.
Reply | Threaded
Open this post in threaded view
|

Re: Decoding speculative insert with toast leaks memory

akapila
On Sat, May 29, 2021 at 5:45 PM Tomas Vondra
<[hidden email]> wrote:

>
> On 5/29/21 6:29 AM, Amit Kapila wrote:
> > On Fri, May 28, 2021 at 5:16 PM Tomas Vondra
> > <[hidden email]> 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.
>

Right.

> So let's get this committed, perhaps with a comment explaining that it
> might be possible to reset earlier if needed.
>

Okay, I think it would be better if we can test this once for the
streaming case as well. Dilip, would you like to do that and send the
updated patch as per one of the comments by Tomas?


--
With Regards,
Amit Kapila.


Reply | Threaded
Open this post in threaded view
|

Re: Decoding speculative insert with toast leaks memory

Dilip Kumar-2
CONTENTS DELETED
The author has deleted this message.
Reply | Threaded
Open this post in threaded view
|

Re: Decoding speculative insert with toast leaks memory

Dilip Kumar-2
CONTENTS DELETED
The author has deleted this message.
Reply | Threaded
Open this post in threaded view
|

Re: Decoding speculative insert with toast leaks memory

Dilip Kumar-2
CONTENTS DELETED
The author has deleted this message.
Reply | Threaded
Open this post in threaded view
|

Re: Decoding speculative insert with toast leaks memory

Dilip Kumar-2
CONTENTS DELETED
The author has deleted this message.
123