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

Hi,

On Thu, Oct 1, 2026 at 9:57 PM Michael Paquier <michael(at)paquier(dot)xyz> wrote:
>
> Er, why is xlogreader_close_segment() registered in WalRead()?? It
> looks like a layer violation to refer to xlogreader_close_segment() in
> a rather generic code path. BasicOpenFile() is one method to open a
> segment *within* the .segment_open() callback. Something else may be
> used to open the fd, like something transactionally safe, where the
> reset callback would not be needed. Perhaps we should take a step
> back and think more widely here, handling this callback in an optional
> manner like the segment open and close bits.

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.

IIUC, the idea is to add a new optional reset callback to
XLogReaderRoutine, similar to the segment open and close callbacks.
Each WAL reader would supply its own reset function, and the generic
read path would use that instead of one specific function. WAL readers
that don't open a segment, or open it with a transient fd, would pass
nothing and skip the registration. Am I missing anything here?

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.

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 the two cannot get out of
sync. It is also less invasive overall, both in core, since there is
no need to change all the WAL reader initialization callsites, and
externally, since there is no new callback for external WAL readers to
pass at all.

> Then comes the point of what we should do in the back branches. I am
> not really cool with changing the size of XLogReaderState on ABI
> ground, which is a very popular structure out there. An alternative
> would be some static variables englobed in a set of non-FRONTEND
> blocks, but I cannot get really excited with this perspective, either.
> I'd like to think that we should just do something on HEAD and call it
> a day. The failure mode based on pg_get_wal_records_info() (revoked
> from public by default) is artistic as fds are freed once a session
> exits.

Agreed. The back-branch changes look invasive, mainly because there's
no memory context unregister reset callback there. The logical
decoding functions also need a replication role, and I haven't seen
this reported from the field. So I'm fine fixing only HEAD and not
back-patching to all the supported branches. That said, if there's
time, I would back-patch to PG19 too. It hasn't been released yet, so
the struct change is okay there, and it already has the unregister
reset callback. That would keep the version diff small, though I don't
have a strong opinion on it.

Thoughts?

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