Re: Remove page-read callback from XLogReaderState.

On Wed, Mar 31, 2021 at 7:17 PM Kyotaro Horiguchi
<horikyota(dot)ntt(at)gmail(dot)com> wrote:
> At Wed, 31 Mar 2021 10:00:02 +1300, Thomas Munro <thomas(dot)munro(at)gmail(dot)com> wrote in
> > On Thu, Mar 4, 2021 at 3:29 PM Kyotaro Horiguchi
> > <horikyota(dot)ntt(at)gmail(dot)com> wrote:
> > > A recent commot about LSN_FORMAT_ARGS conflicted this.
> > > Just rebased.
> >
> > FYI I've been looking at this, and I think it's a very nice
> > improvement. I'll post some review comments and a rebase shortly.
>
> Thanks for looking at this!

I rebased and pgindent-ed the first three patches and did some
testing. I think it looks pretty good, though I still need to check
the code coverage when running the recovery tests. There are three
compiler warnings from GCC when not using --enable-cassert, including
uninitialized variables: pageHeader and targetPagePtr. It looks like
they could be silenced as follows, or maybe you see a better way?

- XLogPageHeader pageHeader;
+ XLogPageHeader pageHeader = NULL;
uint32 pageHeaderSize;
- XLogRecPtr targetPagePtr;
+ XLogRecPtr targetPagePtr =
InvalidXLogRecPtr;

To summarise the patches:

0001 + 0002 get rid of the callback interface and replace it with a
state machine, making it the client's problem to supply data when it
returns XLREAD_NEED_DATA. I found this interface nicer to work with,
for my WAL decoding buffer patch (CF 2410), and I understand that the
encryption patch set can also benefit from it. Certainly when I
rebased my project on this patch set, I prefered the result.

0003 is nice global variable cleanup.

I haven't looked at 0004.