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
signature.asc
Description: PGP signature
