Thanks!
At Fri, 17 Jan 2020 20:14:36 +0200, Heikki Linnakangas <hlinnaka@iki.fi> wrote in
> On 29/11/2019 10:14, Kyotaro Horiguchi wrote:
> > At Thu, 28 Nov 2019 21:37:03 +0900 (JST), Kyotaro Horiguchi
> > <horikyota.ntt@gmail.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'm very sorry but I missed that...
> 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?
Separating XLogBeginRead seems reasonable. The annoying recptr trick
is no longer needed. In particular some logical-decoding stuff become
far cleaner by the patch.
fetching_ckpt of ReadRecord is a bit annoying but that coundn't be
moved outside to, say, ReadCheckpointRecord in a clean way. (Anyway
XLogBeginRead is not the place, of course).
As I looked through it, it looks good to me but give me some more time
to look closer.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
