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