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

Hi,

On Fri, Sep 25, 2026 at 1:54 AM Michael Paquier <michael(at)paquier(dot)xyz> wrote:
>
> While going quickly through the patch and the thread, my first
> reaction was: why not generalizing more the use of transaction-safe
> fds where we could for the WAL segment open, as well?
>
> That does not stand in the logical decoding path, as I am pretty sure
> that we may lost an fd we could still need. Looking at the segment
> open callbacks, the second case where it does not fit well is a WAL
> sender (WalSndSegmentOpen) with the segments that would be opened
> outside the transaction realm.

Right.

> However, could we consider the option of transient fds for anything
> else, particularly the users of wal_segment_open() in xlogutils.c?

I would prefer to avoid having two mechanisms to fix the same issue.

> With these two cases in mind, I can get behind the idea of a reset
> callback. However, I am not convinced that forcing the presence of a
> reset callback when the xlogreader state is *allocated* is a correct
> design, because it forces the decision to close a fd for everybody who
> uses a XLogReaderAllocate, unconditionally, all the time, and it may
> not even be required to open an fd at all in some cases (all WAL in
> some buffers, heh). The reason why we need to close these fds in the
> first place is due to the calls of segment_open, because we open a
> segment. I would imagine here that the sane move is to register a
> callback *iff* we open a segment. So, add a boolean flag in the state
> tracking if the reset callback is registered, and use
> GetMemoryChunkContext(state) to save the callback in the memory
> context of the xlogreader state, not the CurrentMemoryContext where
> the segment is opened.

Agreed. A reader whose page_read callback reads everything from WAL
buffers, for example with WALReadFromBuffers(), may not call
segment_open at all, so it has no file to close, and registering a
callback for it at allocation time is wasted. Registering it lazily on
the first segment_open call avoids that. I will do it that way in the
next version, with the callback on the reader's own context.

> Note that xlogreader.h declares a new variable that makes no sense in
> FRONTEND code. This needs an #ifdef.

Ah, missed that. I will fix it.

I will address the comments and post new patches soon.

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