Re: Decoding speculative insert with toast leaks memory

On Wed, Jun 2, 2021 at 11:52 AM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Wed, Jun 2, 2021 at 11:38 AM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
> >
> > On Wed, Jun 2, 2021 at 11:25 AM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> > >
> > > I think the same relation case might not create a problem because it
> > > won't find the entry for it in the toast_hash, so it will return from
> > > there but the other two problems will be there.
> >
> > Right
> >
> > So, one idea could be
> > > to just avoid those two cases (by simply adding return for those
> > > cases) and still we can rely on toast clean up on the next
> > > insert/update/delete. However, your approach looks more natural to me
> > > as that will allow us to clean up memory in all cases instead of
> > > waiting till the transaction end. So, I still vote for going with your
> > > patch's idea of cleaning at spec_abort but I am fine if you and others
> > > decide not to process spec_abort message. What do you think? Tomas, do
> > > you have any opinion on this matter?
> >
> > I agree that processing with spec abort looks more natural and ideally
> > the current code expects it to be getting cleaned after the change,
> > that's the reason we have those assertions and errors.
> >

Okay, so, let's go with that approach. I have thought about whether it
creates any problem in back-branches but couldn't think of any
primarily because we are not going to send anything additional to
plugin/subscriber. Do you see any problems with back branches if we go
with this approach?

> > OTOH I agree
> > that we can just return from those conditions because now we know that
> > with the current code those situations are possible. My vote is with
> > handling the spec abort option (Option1) because that looks more
> > natural way of handling these issues and we also don't have to clean
> > up the hash in "ReorderBufferReturnTXN" if no followup change after
> > spec abort.
> >
>
> Even, if we decide to go with spec_abort approach, it might be better
> to still keep the toastreset call in ReorderBufferReturnTXN so that it
> can be freed in case of error.
>

Please take care of this as well.

--
With Regards,
Amit Kapila.