Re: Decoding speculative insert with toast leaks memory

On Mon, Jun 7, 2021 at 8:46 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:

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

I have fixed all pending review comments and also added a test case which
is working fine.  I haven't yet checked on the back branches.  Let's
discuss if we think this patch looks fine then I can apply and test on the
back branches.


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