On Fri, Dec 2, 2022 at 4:58 PM Ashutosh Bapat <ashutosh.bapat....@gmail.com> wrote: > > Hi Dilip, > > On Tue, Nov 29, 2022 at 9:38 AM Dilip Kumar <dilipbal...@gmail.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.
