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
