Re: Remove page-read callback from XLogReaderState. - Mailing list pgsql-hackers

On 12/07/2019 10:10, Kyotaro Horiguchi wrote:
> 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.

I like the idea of refactoring XLogReadRecord() to not use a callback, 
and return a XLREAD_NEED_DATA value instead. It feels like a nicer, 
easier-to-use, interface, given that all the page-read functions need 
quite a bit of state and internal logic themselves. I remember that I 
felt that that would be a nicer interface when we originally extracted 
xlogreader.c into a reusable module, but I didn't want to make such big 
changes to XLogReadRecord() at that point.

I don't much like the "continuation" style of implementing the state 
machine. Nothing wrong with such a style in principle, but we don't do 
that anywhere else, and the macros seem like overkill, and turning the 
local variables static is pretty ugly. But I think XLogReadRecord() 
could be rewritten into a more traditional state machine.

I started hacking on that, to get an idea of what it would look like and 
came up with the attached patch, to be applied on top of all your 
patches. It's still very messy, it needs quite a lot of cleanup before 
it can be committed, but I think the resulting switch-case state machine 
in XLogReadRecord() is quite straightforward at high level, with four 
states.

I made some further changes to the XLogReadRecord() interface:

* If you pass a valid ReadPtr (i.e. the starting point to read from) 
argument to XLogReadRecord(), it always restarts reading from that 
record, even if it was in the middle of reading another record 
previously. (Perhaps it would be more convenient to provide a separate 
function to set the starting point, and remove the RecPtr argument from 
XLogReadRecord altogther?)

* XLogReaderState->readBuf is now allocated and controlled by the 
caller, not by xlogreader.c itself. When XLogReadRecord() needs data, 
the caller makes the data available in readBuf, which can point to the 
same buffer in all calls, or the caller may allocate a new buffer, or it 
may point to a part of a larger buffer, whatever is convenient for the 
caller. (Currently, all callers just allocate a BLCKSZ'd buffer, 
though). The caller also sets readPagPtr, readLen and readPageTLI to 
tell XLogReadRecord() what's in the buffer. So all these read* fields 
are now set by the caller, XLogReadRecord() only reads them.

* In your patch, if XLogReadRecord() was called with state->readLen == 
-1, XLogReadRecord() returned an error. That seemed a bit silly; if an 
error happened while reading the data, why call XLogReadRecord() at all? 
You could just report the error directly. So I removed that.

I'm not sure how intelligible this patch is in its current state. But I 
think the general idea is good. I plan to clean it up further next week, 
but feel free to work on it before that, either based on this patch or 
by starting afresh from your patch set.

- Heikki