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
diff --git a/src/include/access/xlogreader.h b/src/include/access/xlogreader.h index c21b0ba972..d930fe957d 100644 --- a/src/include/access/xlogreader.h +++ b/src/include/access/xlogreader.h @@ -21,7 +21,7 @@ * XLogReadRecord or XLogFindNextRecord; it can be passed in as NULL * otherwise. The WALRead function can be used as a helper to write * page_read callbacks, but it is not mandatory; callers that use it, - * must supply open_segment callbacks. The close_segment callback + * must supply segment_open callbacks. The segment_close callback * must always be supplied. * * After reading a record with XLogReadRecord(), it's decomposed into
signature.asc
Description: PGP signature
