Re: Remove page-read callback from XLogReaderState.

At Tue, 28 May 2019 04:45:24 -0700, Andres Freund <andres(at)anarazel(dot)de> wrote in <20190528114524(dot)dvj6ymap2virlzro(at)alap3(dot)anarazel(dot)de>
> Hi,
>
> On 2019-04-18 21:02:57 +0900, Kyotaro HORIGUCHI wrote:
> > Hello. As mentioned before [1], read_page callback in
> > XLogReaderState is a cause of headaches. Adding another
> > remote-controlling stuff to xlog readers makes things messier [2].
> >
> > I refactored XLOG reading functions so that we don't need the
> > callback. In short, ReadRecrod now calls XLogPageRead directly
> > with the attached patch set.
> >
> > | while (XLogReadRecord(xlogreader, RecPtr, &record, &errormsg)
> > | == XLREAD_NEED_DATA)
> > | XLogPageRead(xlogreader, fetching_ckpt, emode, randAccess);
> >
> > On the other hand, XLogReadRecord became a bit complex. The patch
> > makes XLogReadRecord a state machine. I'm not confident that the
> > additional complexity is worth doing. Anyway I'll gegister this
> > to the next CF.
>
> Just FYI, to me this doesn't clearly enough look like an improvement,
> for a change of this size.

Thanks for the opiniton. I kinda agree about size but it is a
decision between "having multiple callbacks called under the
hood" vs "just calling a series of functions". I think the
patched XlogReadRecord is easy to use in many situations.

It would be better if I could completely refactor the function
without the syntax tricks but I think the current patch is still
smaller and clearer than overhauling it.

If many of the folks think that adding a callback is better than
this refactoring, I will withdraw this..

reagrds.

--
Kyotaro Horiguchi
NTT Open Source Software Center