At Mon, 25 May 2020 14:19:34 +0900, Michael Paquier <mich...@paquier.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