Re: Remove page-read callback from XLogReaderState.

On 29/11/2019 10:14, Kyotaro Horiguchi wrote:
> At Thu, 28 Nov 2019 21:37:03 +0900 (JST), Kyotaro Horiguchi <horikyota(dot)ntt(at)gmail(dot)com> wrote in
>>>> 0dc8ead463 hit this. Rebased.
>>>
>>> Please review the pg_waldump.c hunks in 0001; they revert recent changes.
>>
>> Ughhh! I'l check it. Thank you for noticing!!
>
> Fixed that, re-rebased and small comment and cosmetic changes in this
> version.

Thanks! I finally got around to look at this again. A lot has happened
since I last looked at this. Notably, commit 0dc8ead463 introduced
another callback function into the XLogReader interface. It's not
entirely clear what the big picture with the new callback was and how
that interacts with the refactoring here. I'll have to spend some time
to make myself familiar with those changes.

Earlier in this thread, you wrote:
>
> - Changed calling convention of XLogReadRecord
>
> To make caller loop simple, XLogReadRecord now allows to specify
> the same valid value while reading the a record. No longer need
> to change lsn to invalid after the first call in the following
> reader loop.
>
> while (XLogReadRecord(state, lsn, &record, &errormsg) == XLREAD_NEED_DATA)
> {
> if (!page_reader(state))
> break;
> }

Actually, I had also made a similar change in the patch version I posted
at
https://www.postgresql.org/message-id/4f7a5fad-fa04-b0a3-231b-56d200b646dc%40iki.fi.
Maybe you missed that email? It looks like you didn't include any of the
changes from that in the patch series. In any case, clearly that idea
has some merit, since we both independently made a change in that
calling convention :-).

I changed that by adding new function, XLogBeginRead(), that repositions
the reader, and removed the 'lsn' argument from XLogReadRecord()
altogether. All callers except one in findLastCheckPoint() pg_rewind.c
positioned the reader once, and then just read sequentially from there,
so I think that API is more convenient. With that, the usage looks
something like this:

state = XLogReaderAllocate (...)
XLogBeginRead(state, start_lsn);
while (ctx->reader->EndRecPtr < end_of_wal)
{
while (XLogReadRecord(state, &record, &errormsg) == XLREAD_NEED_DATA)
{
if (!page_reader(state))
break;
}
/* do stuff */
....
}

Actually, I propose that we make that change first, and then continue
reviewing the rest of these patches. I think it's a more convenient
interface, independently of the callback refactoring. What do you think
of the attached patch?

- Heikki