Re: Avoid streaming the transaction which are skipped (in corner cases)

On Fri, Dec 2, 2022 at 4:58 PM Ashutosh Bapat
<ashutosh(dot)bapat(dot)oss(at)gmail(dot)com> wrote:
>
> Hi Dilip,
>
> On Tue, Nov 29, 2022 at 9:38 AM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
> >
> > >
> > > You are right we need this in ReorderBufferProcessPartialChange() as
> > > well. I will fix this in the next version.
> >
> > Fixed this in the attached patch.
> >
>
> I focused my attention on SnapBuildXactNeedsSkip() usages and I see
> they are using different end points of WAL record
> 1 decode.c logicalmsg_decode 594
> SnapBuildXactNeedsSkip(builder, buf->origptr)))
> 2 decode.c DecodeTXNNeedSkip 1250 return
> (SnapBuildXactNeedsSkip(ctx->snapshot_builder, buf->origptr) ||
> 3 reorderbuffer.c AssertTXNLsnOrder 897 if
> (SnapBuildXactNeedsSkip(ctx->snapshot_builder,
> ctx->reader->EndRecPtr))
> 4 reorderbuffer.c ReorderBufferCanStartStreaming 3922
> !SnapBuildXactNeedsSkip(builder, ctx->reader->EndRecPtr))
> 5 snapbuild.c SnapBuildXactNeedsSkip 429
> SnapBuildXactNeedsSkip(SnapBuild *builder, XLogRecPtr ptr)
>
> The first two are using origin ptr and the last two are using end ptr.
> you have fixed the fourth one. Do we need to fix the third one as
> well?
>

I think we can change the third one as well but I haven't tested it.
Adding Sawada-San for his inputs as it is added in commit 16b1fe0037.
In any case, I think we can do that as a separate patch because it is
not directly related to the streaming case we are trying to solve as
part of this patch.

> Probably we need to create two wrappers (macros) around
> SnapBuildXactNeedsSkip(), one which accepts a XLogRecordBuffer and
> other which accepts XLogReaderState. Then use those. That way at least
> we have logic unified as to which XLogRecPtr to use.
>

I don't know how that will be an improvement because both those have
the start and end locations of the record.

--
With Regards,
Amit Kapila.