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.
> 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.
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
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 (?).
--
Michael
