Re: Remove page-read callback from XLogReaderState.

From: Heikki Linnakangas <hlinnaka(at)iki(dot)fi>
To: Kyotaro Horiguchi <horikyota(dot)ntt(at)gmail(dot)com>, andres(at)anarazel(dot)de
Cc: pgsql-hackers(at)postgresql(dot)org
Subject: Re: Remove page-read callback from XLogReaderState.
Date: 2019-07-29 19:39:57
Message-ID: e1ecb53b-663d-98ed-2249-dfa30a74f8c1@iki.fi
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

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

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

Attachment Content-Type Size
refactor-xlogreaderstate-callback-heikki.patch text/x-patch 48.5 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Peter Geoghegan 2019-07-29 20:01:54 Re: should there be a hard-limit on the number of transactions pending undo?
Previous Message Peter Geoghegan 2019-07-29 19:39:23 Re: should there be a hard-limit on the number of transactions pending undo?