At Tue, 28 May 2019 04:45:24 -0700, Andres Freund <andres@anarazel.de> wrote in
<20190528114524.dvj6ymap2virlzro@alap3.anarazel.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
