Re: WAL segment file descriptor leak on read errors can PANIC the server

Hi,

On Sun, Oct 4, 2026 at 10:52 PM Michael Paquier <michael(at)paquier(dot)xyz> 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.

Agreed. I would even say that my thinking on assuming every xlogreader
using the BasicOpenFile() itself was wrong.

> 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.

Right. If an external WAL reader uses the transient file API (for
whatever reasons), they anyway need to carry the fd in the xlogreader
state across, in which case, I would expect them to set it to -1 after
closing it in the normal paths. But as you mentioned, it's not quite
possible to do in the automatic cleanup paths that these file API
provides.

> > 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...
> >
> > I prefer this second approach over the optional reset callback to
> > XLogReaderRoutine. On the duplication, the registration code can go
> > into a common function that the segment open callbacks reuse. That
> > keeps the decision of whether the reset callback is needed close to
> > where and how the segment is opened...
>
> So, I have been looking at a softer approach, and finished with the
> attached.
> So, thoughts, tomatoes, or both of them?

Thanks. I reviewed v3 patch and it mostly aligns with my thinking
above. Please find the attached v4 patch.

I addressed Bertrand's review comments. I renamed the reset callback
to keep it generic (closing the open WAL segment file is one cleanup
that could be done here but it could be extended for any other
resources held by XLogReader). I adjusted the comments. I like the
comment around segment_close on not throwing errors, although the core
callbacks inherently follow that (they ignore errors from close()),
it's good to have it. I ran pgindent and tests. I wrote the commit
message.

As I mentioned upthread, I'm okay to not back-patch this fix (PG18 and
older). I don't have a strong opinion (unless anyone thinks otherwise)
for PG19 given less than two weeks left for release.

Thoughts?

--
Bharath Rupireddy
Amazon Web Services: https://aws.amazon.com