Re: WAL reader APIs and WAL segment open/close callbacks

At Mon, 25 May 2020 14:19:34 +0900, Michael Paquier <michael(at)paquier(dot)xyz> wrote in
> On Mon, May 25, 2020 at 11:17:06AM +0900, Kyotaro Horiguchi wrote:
> > That depends on where we draw responsibility border, or who is
> > responsible to the value of ws_file. I think that this API change was
> > assuming the callbacks having full-knowledge of the xlogreader struct
> > and are responsible to maintain related struct members, and I agree to
> > that direction.
>
> Sure. Still I am skeptical that the interface of HEAD is the most
> instinctive choice as designed. We assume that plugins using
> xlogreader.c have to set the flag all the way down for something which
> is mostly an internal state. WAL senders need to be able to use the
> fd directly to close the segment in some code paths, but the only
> thing where we give, and IMO, should give access to the information of
> WALOpenSegment is for the error path after a failed WALRead(). And it
> seems more natural to me to return the opened fd to xlogreader.c, that
> is then the part in charge of assigning the fd to the correct part of
> XLogReaderState. This reminds me a bit of what we did for
> libpqrcv_receive() a few years ago where we manipulate directly a fd
> to wait on instead of setting it directly in some internal structure.

I agree that it's generally natural that open callback returns an fd
and close takes an fd. However, for the xlogreader case, the
xlogreader itself doesn't know much about files. WALRead is the
exception as a convenient routine for reading WAL files in a generic
way, which can be thought as belonging to the caller side. The
callbacks (that is, the caller side of xlogreader) are in charge of
opening, reading and closing segment files. That is the same for the
case of XLogPageRead, which is the caller of xlogreader and is
directly manipulating ws_file and ws_tli.

Further, I think that xlogreader shouldn't know about file handling at
all. The reason that xlogreaderstate has fd and tli is that it is
needed by file-handling callbacks, which belongs to the caller of
xlogreader.

If the call structure were upside-down, that is, callers handled files
privately then call xlogreader to retrieve records from the page data,
things would be simpler in the caller's view. That is the patch I'm
proposing as a xlog reader refactoring [1].

> > If we are going to hide the struct from the callbacks, we shouldn't
> > pass to the callbacks a pointer to the complete XLogReaderState
> > struct.
>
> It still seems to me that it is helpful to pass down the whole thing
> to the close and open callbacks, for at least debugging purposes. I
> found that helpful when debugging my tool through my rebase with
> v13.

Why do you not looking into upper stack-frames?

> As a side note, it was actually tricky to find out that you have to
> call WALRead() to force the opening of a new segment when calling
> XLogFindNextRecord() in the block read callback after WAL reader
> allocation. Perhaps we should call segment_open() at the beginning of
> XLogFindNextRecord() if no segment is opened yet? I would bet that

The API change was mere a refactoring and didn't change the whole
logic largely.

The segment open callback is not a part of xlogreader but only a part
of the WALRead function. As I mentioned above, file handling is the
matter of the caller side (and WALRead, which is a part of
caller-side). ReadPageInternal doesn't know about underlying files at
all.

> not everything is aimed at using WALRead() even if that's a useful
> wrapper, and as shaped the block-read callbacks are mostly useful to
> give the callers the ability to adjust to a maximum record length that
> can be read, which looks limited (?).

I'm not sure. The reason for the fact that the page-read callbacks
that don't use WALRead doesn't need open callback is it's not actually
belongs to xlogreader, I think.

[1] https://www.postgresql.org/message-id/20200422.101246.331162888498679491.horikyota.ntt%40gmail.com

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center