Thanks! At Fri, 17 Jan 2020 20:14:36 +0200, Heikki Linnakangas <hlinn...@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....@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