On 2020-May-25, Michael Paquier wrote: > I have been playing with the new APIs of xlogreader.h, and while > merging some of my stuff with 13, I found the handling around > ->seg.ws_file overcomplicated and confusing as it is necessary for a > plugin to manipulate directly the fd of an opened segment in the WAL > segment open/close callbacks. > > Wouldn't it be cleaner to limit the exposition of ->seg.ws_file to the > user if possible? There are cases like a WAL sender where you cannot > do that, but something that came to my mind is to make > WALSegmentOpenCB return the fd of the opened segment, and pass down the > fd to close to WALSegmentCloseCB. Then xlogreader.c is in charge of > resetting the field when a segment is closed.
The original code did things as you suggest: the open_segment callback returned the FD, and the caller installed it in the struct. We then changed it in commit 850196b610d2 to have the CB install the FD in the struct directly. I didn't like this idea when I first saw it -- my reaction was pretty much the same as yours -- but eventually I settled on it because if we want xlogreader to be in charge of installing the FD, then we should also make it responsible for reacting properly when a bad FD is returned, and report errors correctly. (In the previous coding, xlogreader didn't tolerate bad FDs; it just blindly installed a bad FD if one was returned. Luckily, existing CBs never returned any bad FDs so there's no bug, but it seems hazardous API design.) In my ideal world, the open_segment CB would just open and return a valid FD, or return an error message if unable to; if WALRead sees that the returned FD is not valid, it installs the error message in *errinfo so its caller can report it. I'm not opposed to doing things that way, but it seemed more complexity to me than what we have now. Now maybe you wish for a middle ground: the CB returns the FD, or fails trying. Is that what you want? I didn't like that, as it seems unprincipled. I'd rather keep things as they're now. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services