Re: Decoding speculative insert with toast leaks memory

On Mon, 7 Jun 2021 at 8:30 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:

> On Wed, Jun 2, 2021 at 11:52 AM Amit Kapila <amit.kapila16@gmail.com>
> wrote:
> >
> > On Wed, Jun 2, 2021 at 11:38 AM Dilip Kumar <dilipbalaut@gmail.com>
> wrote:
> > >
> > > On Wed, Jun 2, 2021 at 11:25 AM Amit Kapila <amit.kapila16@gmail.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?


I will check this and let you know.


> > >  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.


Ok

> --
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com