Re: Decoding speculative insert with toast leaks memory

On Wed, Jun 2, 2021 at 11:25 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Tue, Jun 1, 2021 at 5:23 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> >
> > On Tue, Jun 1, 2021 at 12:25 PM Amit Kapila <amit.kapila16@gmail.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.  I am attaching the patches with both the approaches for
the reference.

Once we finalize on the approach then I will work on pending review
comments and also prepare the back branch patches.

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