Re: Decoding speculative insert with toast leaks memory

On Tue, Jun 1, 2021 at 11:00 AM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
>
> On Tue, Jun 1, 2021 at 10:21 AM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> >
> > On Tue, Jun 1, 2021 at 9:59 AM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
> > >
> > > On Tue, Jun 1, 2021 at 9:53 AM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> > > >
> > > > On Mon, May 31, 2021 at 8:12 PM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
> > > > >
> > > > > On Mon, May 31, 2021 at 6:32 PM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
> > > > > >
> > > > > > I missed to do the test for streaming. I will to that tomorrow and reply back.
> > > > >
> > > > > For streaming transactions this issue is not there. Because this
> > > > > problem will only occur if the last change is *SPEC INSERT * and after
> > > > > that there is no other UPDATE/INSERT change because on that change we
> > > > > are resetting the toast table. Now, if the transaction has only *SPEC
> > > > > INSERT* without SPEC CONFIRM or any other INSERT/UPDATE then we will
> > > > > not stream it. And if we get any next INSERT/UPDATE then only we can
> > > > > select this for stream but in that case toast will be reset. So as of
> > > > > today for streaming mode we don't have this problem.
> > > > >
> > > >
> > > > What if the next change is a different SPEC_INSERT
> > > > (REORDER_BUFFER_CHANGE_INTERNAL_SPEC_INSERT)? I think in that case we
> > > > will stream but won't free the toast memory.
> > >
> > > But if the next change is again the SPEC INSERT then we will keep the
> > > PARTIAL change flag set and we will not select this transaction for
> > > stream right?
> > >
> >
> > Right, I think you can remove the change related to stream xact and
> > probably write some comments on why we don't need it for streamed
> > transactions. But, now I have another question related to fixing the
> > non-streamed case. What if after the missing spec_confirm we get the
> > delete operation in the transaction? It seems
> > ReorderBufferToastReplace always expects Insert/Update if we have
> > toast hash active in the transaction.
>
> Yeah, that looks like a problem, I will test this case.

I am able to hit that Assert after slight modification in the original
test case, basically, I added an extra delete in the spec abort
transaction and I got this assertion.

#0 0x00007f7b8cc3a387 in raise () from /lib64/libc.so.6
#1 0x00007f7b8cc3ba78 in abort () from /lib64/libc.so.6
#2 0x0000000000b027c7 in ExceptionalCondition (conditionName=0xcc11df
"change->data.tp.newtuple", errorType=0xcc0244 "FailedAssertion",
fileName=0xcc0290 "reorderbuffer.c", lineNumber=4601) at assert.c:69
#3 0x00000000008dfeaf in ReorderBufferToastReplace (rb=0x1a73e40,
txn=0x1b5d6e8, relation=0x7f7b8dab4d78, change=0x1b5fb68) at
reorderbuffer.c:4601
#4 0x00000000008db769 in ReorderBufferProcessTXN (rb=0x1a73e40,
txn=0x1b5d6e8, commit_lsn=24329048, snapshot_now=0x1b4b8d0,
command_id=0, streaming=false)
at reorderbuffer.c:2187
#5 0x00000000008dc1df in ReorderBufferReplay (txn=0x1b5d6e8,
rb=0x1a73e40, xid=748, commit_lsn=24329048, end_lsn=24329096,
commit_time=675842700629597,
origin_id=0, origin_lsn=0) at reorderbuffer.c:2601
#6 0x00000000008dc267 in ReorderBufferCommit (rb=0x1a73e40, xid=748,
commit_lsn=24329048, end_lsn=24329096, commit_time=675842700629597,
origin_id=0, origin_lsn=0)
at reorderbuffer.c:2625
#7 0x00000000008cc144 in DecodeCommit (ctx=0x1b319b0,
buf=0x7ffdf15fb0a0, parsed=0x7ffdf15faf00, xid=748, two_phase=false)
at decode.c:744

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?

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