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

On Mon, Sep 21, 2026 at 03:14:39PM -0500, Sami Imseih wrote:
> At first, I thought palloc_object() would do and be simpler, but keeping
> MCXT_ALLOC_NO_OOM gives the better message, since callers report
> errdetail("Failed while allocating a WAL reading processor."), and
> keeping this consistent with the other allocations is probably best.
> So I agree with what you have.
>
> I do not see anything else that stands out in the patch.

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.

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

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.

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