On Sun, Oct 04, 2026 at 05:15:00PM -0700, Bharath Rupireddy wrote:
> I agree that a WAL reader could open the segment as a transient file
> descriptor (fd) rather than a plain kernel one with BasicOpenFile(),
> so it gets closed on error. IOW, not all WAL readers need the reset
> callback registered.
Just note that my main worry with the latest patch posted upthread is
just how non-flexible it is. If one has the idea to use
OpenTransientFile() in the segment_open callback of a xlogreader,
reset_cb would fire over a stale fd because of AtEOXact_Files().
That's even more problematic if the xlogreader is for example in a
TopTransactionContext, as AtEOXact_Files() fires *before*
AtCommit_Memory() and AtCleanup_Memory(), and we would attempt a
segment_close on what's a stale fd when reaching the reset callback.
> I can think of another approach, which is to register the reset
> callback inside each segment open callback, right after the segment is
> opened with a kernel fd. That keeps it out of the generic read path,
> but it duplicates the registration across all the core segment open
> callbacks, and external WAL readers opening a plain kernel fd would
> need to do the same.
So, I have been looking at a softer approach, and finished with the
attached. This is based on a helper routine that one can call to
define the reset callback, if required, relying also on the previous
idea of the callback being in the state data. reset_cb and
reset_cb_registered cannot be avoided, we need tracking within the
xlogreader state itself.
It still feels a bit weird to call a callback from another callback,
but I don't quite see how we can avoid that. Another option would be
some fancier and much more invasive CATCH/TRY blocks, but that's brr.
And at least the XLogReaderRegisterReset() calls I have added here are
localized close enough to the BasicOpenFile() that they are impossible
to miss, at least it feels so to me.
I have also double-checked your original test case, of course.
So, thoughts, tomatoes, or both of them?
--
Michael
