Re: Decoding speculative insert with toast leaks memory

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:
> >
> > On Tue, Jun 1, 2021 at 5:23 PM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
> > >
> > > On Tue, Jun 1, 2021 at 12:25 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> > >
> > > > >
> > > > > IMHO, as I stated earlier one way to fix this problem is that we add
> > > > > the spec abort operation (DELETE + XLH_DELETE_IS_SUPER flag) to the
> > > > > queue, maybe with action name
> > > > > "REORDER_BUFFER_CHANGE_INTERNAL_SPEC_ABORT" and as part of processing
> > > > > that just cleans up the toast and specinsert tuple and nothing else.
> > > > > If we think that makes sense then I can work on that patch?
> > > > >
> > > >
> > > > I think this should solve the problem but let's first try to see if we
> > > > have any other problems. Because, say, if we don't have any other
> > > > problem, then maybe removing Assert might work but I guess we still
> > > > need to process the tuple to find that we don't need to assemble toast
> > > > for it which again seems like overkill.
> > >
> > > Yeah, other operation will also fail, basically, if txn->toast_hash is
> > > not NULL then we assume that we need to assemble the tuple using
> > > toast, but if there is next insert in another relation and if that
> > > relation doesn't have a toast table then it will fail with below
> > > error. And otherwise also, if it is the same relation, then the toast
> > > chunks of previous tuple will be used for constructing this new tuple.
> > >
> >
> > 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. 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.

--
With Regards,
Amit Kapila.