On Mon, May 25, 2020 at 04:30:34PM -0400, Alvaro Herrera wrote:
> 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.
Installing the fd in WALOpenSegment and reporting an error are not
related concepts though, no? segment_open could still report errors
and return the fd, where then xlogreader.c saves the returned fd in
ws_file.
> (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.)
I think that the assertions making sure that bad fds are not passed
down by segment_open are fine to live with.
> 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.
Hm. We require now that segment_open fails immediately if it cannot
have a correct fd, so it does not return an error message, it just
gives up. I am indeed not sure that moving around more WALReadError
is that interesting for that purpose. It could be interesting to
allow plugins to have a way to retry opening a new segment though
instead of giving up? But we don't really need that much now in
core.
> 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.
Yeah, I think that the patch I sent previously is attempting at doing
things in a middle ground, which felt more natural to me while merging
my own stuff: do not fill directly ws_file within segment_open, and
let xlogreader.c save the returned fd, with segment_open giving up
immediately if we cannot get one. If you wish to keep things as they
are now that's fine by me :)
NB: I found some incorrect comments as per the attached:
s/open_segment/segment_open/
s/close_segment/segment_close/
--
Michael