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

Hi,

A failed WAL read leaks the open WAL segment file descriptor until the
backend exits. Once the fd limit is reached, all later queries in that
backend fail with "Too many open files". With a connection pooler the
leaks add up across clients. There is no warning or log message for
this. Reproducers are at [1] and [2].

I searched the archives and did not find any earlier report of this. It
should be fixed regardless.

I think the fix is to register a memory context reset callback on the
context the reader is allocated in. [...] Doing this in
XLogReaderAllocate() covers every caller, present and future, instead
of adding an error handler to each one.

I agree with the proposed fix.

Using your patch, I put Assert(state->seg.ws_file == -1) before the
close in the new callback and ran check-world on a cassert build. It
fires in two distinct places.

One is the logical decoding case, that is, errors mid-decode which means
XLogReaderFree() is never reached.

The other is the physical walsender, which brings me to this:

The walsender does not have this issue because it closes the file in
its own error cleanup.

This is not true in the normal exit case. StartReplication() never
calls XLogReaderFree(), so on a clean CopyDone the reader is discarded
with the segment file still open.

This is easy to see by logging in the new callback:

```
if (state->seg.ws_file != -1)
{
elog(LOG, "closing leaked WAL fd %d", state->seg.ws_file);
state->routine.segment_close(state);
}
```

Starting a pg_receivewal and stopping it with Ctrl-C, which ends
streaming cleanly rather than with an error:

```
LOG: closing leaked WAL fd 9
STATEMENT: START_REPLICATION SLOT "s1" 0/04000000 TIMELINE 1
```

So a segment file is still open when the reader goes away, on a path
with no error involved at all. Without your patch nothing closes it.

In terms of the patch, I was thinking about this allocation:

```

+ state->reset_cb = palloc_extended(sizeof(MemoryContextCallback),
+  MCXT_ALLOC_NO_OOM | MCXT_ALLOC_ZERO);

```

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.

--
Sami Imseih
Amazon Web Services (AWS)

On Sep 21, 2026, at 15:12, Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote:

Hi,

A failed WAL read leaks the open WAL segment file descriptor until the
backend exits. Once the fd limit is reached, all later queries in that
backend fail with "Too many open files". With a connection pooler the
leaks add up across clients. There is no warning or log message for
this. Reproducers are at [1] and [2].

It can happen with logical decoding that cancelled decode calls
exhaust the fds, and depending on which open fails first, the backend
dies with a FATAL or the instance restarts with a PANIC. [2] shows a
PANIC.

It can also happen that the leaked fd is on a WAL segment that has
since been removed. The file is unlinked but its space is not freed
until the backend goes away, so pg_wal still looks small while free
space on the disk keeps shrinking. On a small WAL volume I could fill
the disk this way, and the server then PANICed while writing a new WAL
file with no space left, which took down the database instance.

I noticed this while looking at the error paths in pg_walinspect for
the reported issue [3]. I then used Claude Code to check for the same
issue elsewhere, and it helped me find the other affected paths.

The WAL read paths open the segment file as a plain kernel fd. It is
not a virtual fd and not a transient file, and no resource owner owns
it, so nothing in the backend knows it is open. On an error, the
memory holding the WAL reader is freed with its context, but the
segment file it had open stays open. This leak seems to exist in
pg_walinspect functions, logical decoding functions, 2PC WAL read
code, and the WAL summarizer. All of these except the WAL summarizer
are reachable from SQL in simple ways. The walsender does not have
this issue because it closes the file in its own error cleanup. The
startup process is fine too, its reader does not keep a segment open
this way, and its read errors are FATAL anyway.

This is the same leak that commit 91c40548d5 fixed for 2PC by closing
the fd in XLogReaderFree(), but that close only runs on the normal
path, not when an error is thrown before it.

I think the fix is to register a memory context reset callback on the
context the reader is allocated in. If that context is reset or
deleted while a segment file is still open, the callback closes it.
Doing this in XLogReaderAllocate() covers every caller, present and
future, instead of adding an error handler to each one. On HEAD,
XLogReaderFree() unregisters the callback and the reader keeps a
pointer to it in a new field. On PG18 and older the reader struct
cannot grow because of ABI, and there is no
MemoryContextUnregisterResetCallback(), so there the callback stays
registered and its state lives in a small list local to the WAL
reader. I attached a nocfbot prefixed patch for the back branches
doing that.

There are alternative approaches that have some issues. Wrapping each
caller in its own error handler works but is easy to miss in future
callers. Opening the segment as a transient file, so that it gets
closed at transaction abort, adds a rule that a reader cannot outlive
a transaction, which does not work for the logical walsender since it
reads WAL outside any transaction. Tracking the fd with a resource
owner also fixes all callers in one place, but the extensible resource
owner API only exists in PG17 and later, so it cannot be used in the
older branches.

Please find the attached patches (v1 for HEAD and PG19, nocfbot for
PG18). If the approach looks good, I will prepare patches for all the
remaining back branches using the PG18 approach.

Thoughts?

[1]
$ ulimit -n 200

CREATE TABLE t1 (a int);
INSERT INTO t1 VALUES (1);

-- Each iteration errors at end of WAL and leaks one fd
DO $$
BEGIN
FOR i IN 1..200 LOOP
BEGIN
PERFORM * FROM
pg_get_wal_records_info(pg_current_wal_flush_lsn(),
'FFFFFFFF/FFFFFFFF');
EXCEPTION WHEN OTHERS THEN
NULL;
END;
END LOOP;
END $$;

-- Session can no longer open any file
postgres=# SELECT count(*) FROM t1;
ERROR: could not open file "base/5/2691": Too many open files
LINE 1: SELECT count(*) FROM t1;
^
postgres=# CREATE TABLE t2 (a int);
ERROR: could not create file "base/5/16398": Too many open files

-- Shows the fd table full of pg_wal segments
$ lsof -w -p <backend pid> | grep -c pg_wal
193

[2]
$ ulimit -n 200

SELECT pg_create_logical_replication_slot('s', 'test_decoding');
CREATE TABLE foo(a int, b text);
INSERT INTO foo SELECT i, repeat('x', 200) FROM generate_series(1, 300000) i;

-- Each cancelled decode errors mid-read and leaks one fd
SET statement_timeout = '10ms';
SELECT 'SELECT * FROM pg_logical_slot_peek_changes(''s'', NULL, NULL);'
FROM generate_series(1, 200) \gexec

ERROR: canceling statement due to statement timeout
ERROR: canceling statement due to statement timeout
..
..
ERROR: canceling statement due to statement timeout
ERROR: canceling statement due to statement timeout
PANIC: could not open file "pg_logical/snapshots/0-18692A8.snap": Too
many open files
server closed the connection unexpectedly
This probably means the server terminated abnormally
before or while processing the request.
The connection to the server was lost. Attempting reset: Failed.

[3] /messages/by-id/80E9F0AD-CFC5-4BE5-81DE-D8FE35E10A1C@gmail.com

--
Bharath Rupireddy
Amazon Web Services: https://aws.amazon.com
<v1-0001-Fix-WAL-segment-file-descriptor-leak-on-WAL-read-.patch><nocfbot-v1-0001-PG18-Fix-WAL-segment-file-descriptor-leak.patch>

I have objection on releasing the FD by a callback. A few comments on v1:

1
```
+ * Reset callbacks run before the context's memory is freed, so the reader is
+ * still valid here. segment_close must not throw an error.
+ */
+static void
+xlogreader_reset_callback(void *arg)
```

This patch seems to introduce a new requirement for segment_close: it must not throw an error when invoked from the reset callback.

Should this requirement also be documented in the header comment for segment_close itself, so that it becomes part of the callback contract?

2
```
+	/*
+	 * Reset callback on the memory context holding this reader, which closes
+	 * the open WAL segment file if that context goes away before
+	 * XLogReaderFree() is reached. Backend only, NULL in frontend code.
+	 */
+	struct MemoryContextCallback *reset_cb;
```

This field is only used in backend code. Should it also be wrapped in #ifndef FRONTEND?

3
```
+	/*
+	 * The WAL segment file is opened with BasicOpenFile(), so nothing but
+	 * XLogReaderFree() ever closes it. An error thrown while reading WAL does
+	 * not get that far, and the descriptor would then be leaked for the life
+	 * of the process, so close it on a reset of the context we are allocated
+	 * in as well.
+	 */
+	state->reset_cb = palloc_extended(sizeof(MemoryContextCallback),
+									  MCXT_ALLOC_NO_OOM | MCXT_ALLOC_ZERO);
```

I think MCXT_ALLOC_NO_OOM is important here, because at this point the reset callback has not been registered yet. If this allocation itself threw an ERROR, there would still be no callback available to clean up the reader's resources.

Would it be worth mentioning this in the comment, so the reason for using MCXT_ALLOC_NO_OOM here is explicit?

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

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

Hi,

On Fri, Sep 25, 2026 at 1:54 AM Michael Paquier <michael@paquier.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

On Sep 22, 2026, at 16:15, Chao Li <li.evan.chao@gmail.com> wrote:

On Sep 21, 2026, at 15:12, Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote:

Hi,

A failed WAL read leaks the open WAL segment file descriptor until the
backend exits. Once the fd limit is reached, all later queries in that
backend fail with "Too many open files". With a connection pooler the
leaks add up across clients. There is no warning or log message for
this. Reproducers are at [1] and [2].

It can happen with logical decoding that cancelled decode calls
exhaust the fds, and depending on which open fails first, the backend
dies with a FATAL or the instance restarts with a PANIC. [2] shows a
PANIC.

It can also happen that the leaked fd is on a WAL segment that has
since been removed. The file is unlinked but its space is not freed
until the backend goes away, so pg_wal still looks small while free
space on the disk keeps shrinking. On a small WAL volume I could fill
the disk this way, and the server then PANICed while writing a new WAL
file with no space left, which took down the database instance.

I noticed this while looking at the error paths in pg_walinspect for
the reported issue [3]. I then used Claude Code to check for the same
issue elsewhere, and it helped me find the other affected paths.

The WAL read paths open the segment file as a plain kernel fd. It is
not a virtual fd and not a transient file, and no resource owner owns
it, so nothing in the backend knows it is open. On an error, the
memory holding the WAL reader is freed with its context, but the
segment file it had open stays open. This leak seems to exist in
pg_walinspect functions, logical decoding functions, 2PC WAL read
code, and the WAL summarizer. All of these except the WAL summarizer
are reachable from SQL in simple ways. The walsender does not have
this issue because it closes the file in its own error cleanup. The
startup process is fine too, its reader does not keep a segment open
this way, and its read errors are FATAL anyway.

This is the same leak that commit 91c40548d5 fixed for 2PC by closing
the fd in XLogReaderFree(), but that close only runs on the normal
path, not when an error is thrown before it.

I think the fix is to register a memory context reset callback on the
context the reader is allocated in. If that context is reset or
deleted while a segment file is still open, the callback closes it.
Doing this in XLogReaderAllocate() covers every caller, present and
future, instead of adding an error handler to each one. On HEAD,
XLogReaderFree() unregisters the callback and the reader keeps a
pointer to it in a new field. On PG18 and older the reader struct
cannot grow because of ABI, and there is no
MemoryContextUnregisterResetCallback(), so there the callback stays
registered and its state lives in a small list local to the WAL
reader. I attached a nocfbot prefixed patch for the back branches
doing that.

There are alternative approaches that have some issues. Wrapping each
caller in its own error handler works but is easy to miss in future
callers. Opening the segment as a transient file, so that it gets
closed at transaction abort, adds a rule that a reader cannot outlive
a transaction, which does not work for the logical walsender since it
reads WAL outside any transaction. Tracking the fd with a resource
owner also fixes all callers in one place, but the extensible resource
owner API only exists in PG17 and later, so it cannot be used in the
older branches.

Please find the attached patches (v1 for HEAD and PG19, nocfbot for
PG18). If the approach looks good, I will prepare patches for all the
remaining back branches using the PG18 approach.

Thoughts?

[1]
$ ulimit -n 200

CREATE TABLE t1 (a int);
INSERT INTO t1 VALUES (1);

-- Each iteration errors at end of WAL and leaks one fd
DO $$
BEGIN
FOR i IN 1..200 LOOP
BEGIN
PERFORM * FROM
pg_get_wal_records_info(pg_current_wal_flush_lsn(),
'FFFFFFFF/FFFFFFFF');
EXCEPTION WHEN OTHERS THEN
NULL;
END;
END LOOP;
END $$;

-- Session can no longer open any file
postgres=# SELECT count(*) FROM t1;
ERROR: could not open file "base/5/2691": Too many open files
LINE 1: SELECT count(*) FROM t1;
^
postgres=# CREATE TABLE t2 (a int);
ERROR: could not create file "base/5/16398": Too many open files

-- Shows the fd table full of pg_wal segments
$ lsof -w -p <backend pid> | grep -c pg_wal
193

[2]
$ ulimit -n 200

SELECT pg_create_logical_replication_slot('s', 'test_decoding');
CREATE TABLE foo(a int, b text);
INSERT INTO foo SELECT i, repeat('x', 200) FROM generate_series(1, 300000) i;

-- Each cancelled decode errors mid-read and leaks one fd
SET statement_timeout = '10ms';
SELECT 'SELECT * FROM pg_logical_slot_peek_changes(''s'', NULL, NULL);'
FROM generate_series(1, 200) \gexec

ERROR: canceling statement due to statement timeout
ERROR: canceling statement due to statement timeout
..
..
ERROR: canceling statement due to statement timeout
ERROR: canceling statement due to statement timeout
PANIC: could not open file "pg_logical/snapshots/0-18692A8.snap": Too
many open files
server closed the connection unexpectedly
This probably means the server terminated abnormally
before or while processing the request.
The connection to the server was lost. Attempting reset: Failed.

[3] /messages/by-id/80E9F0AD-CFC5-4BE5-81DE-D8FE35E10A1C@gmail.com

--
Bharath Rupireddy
Amazon Web Services: https://aws.amazon.com
<v1-0001-Fix-WAL-segment-file-descriptor-leak-on-WAL-read-.patch><nocfbot-v1-0001-PG18-Fix-WAL-segment-file-descriptor-leak.patch>

I have objection on releasing the FD by a callback. A few comments on v1:

I just noticed this stupid typo. I meant to say “NO objection”.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

Hi,

On Fri, Sep 25, 2026 at 4:25 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:

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.

Please find attached the v2 patches implementing lazy registration of
the reset callback, only when a WAL segment is opened. v2-0001 is for
HEAD and PG19. The nocfbot versions are for the back branches.

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

On Wed, Sep 30, 2026 at 09:42:00PM -0700, Bharath Rupireddy wrote:

Please find attached the v2 patches implementing lazy registration of
the reset callback, only when a WAL segment is opened. v2-0001 is for
HEAD and PG19. The nocfbot versions are for the back branches.

+#ifndef FRONTEND
+
+            /*
+             * The WAL segment file is opened with BasicOpenFile(), so nothing
+             * but XLogReaderFree() ever closes it. An error thrown while
+             * reading WAL does not get that far, and the descriptor would
+             * then be leaked for the life of the process, so close it on a
+             * reset of the context the reader was allocated in as well.
+             */
+            if (!state->reset_cb_registered)
+            {
+                state->reset_cb.func = xlogreader_close_segment;
+                state->reset_cb.arg = state;
+                MemoryContextRegisterResetCallback(GetMemoryChunkContext(state),
+                                                   &state->reset_cb);
+                state->reset_cb_registered = true;
+            }
+#endif

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.

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

Hi,

On Thu, Oct 1, 2026 at 9:57 PM Michael Paquier <michael@paquier.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

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

Hi,

On Mon, Oct 05, 2026 at 02:52:41PM +0900, Michael Paquier wrote:

It still feels a bit weird to call a callback from another callback,
but I don't quite see how we can avoid that.

FWIW, a similar pattern already exists in shutdown_validator_library(), where a
memory context reset callback calls ValidatorCallbacks->shutdown_cb().

It makes sense to me here too: the reset callback decides when cleanup is required,
while segment_close knows how to perform it, avoiding duplication of the cleanup
logic.

=== 1

+       if (state->seg.ws_file != -1)
+               state->routine.segment_close(state);

The comment above segment_close() says that ws_file shall be set to a negative
number, while both this callback and XLogReaderFree() check for -1. I wonder if
they should test >= 0 instead?

=== 2

+/*
+ * Register a memory reset callback, closing a segment, if necessary.
+ *
+ * This is useful when opening a segment with BasicOpenFile(), to guarantee
+ * that the segment is closed before XLogReaderFree() is reached.
+ */
+void
+XLogReaderRegisterReset(XLogReaderState *state)

Worth mentioning that this is intended for descriptors not managed by another
cleanup mechanism, such as those returned by BasicOpenFile()? Otherwise, using
it with OpenTransientFile() could result in segment_close() being called with a
stale descriptor.

Regards,

--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

On Mon, Oct 5, 2026 at 10:20 AM Bertrand Drouvot <
bertranddrouvot.pg@gmail.com> wrote:

Worth mentioning that this is intended for descriptors not managed by
another
cleanup mechanism, such as those returned by BasicOpenFile()? Otherwise,
using
it with OpenTransientFile() could result in segment_close() being called
with a
stale descriptor.

BasicOpenFile() called repeatedly will accumulate stale descriptors
indefinitely.

int
BasicOpenFile(const char *fileName, int fileFlags)
{
return BasicOpenFilePerm(fileName, fileFlags, pg_file_create_mode);
}

int
BasicOpenFilePerm(const char *fileName, int fileFlags, mode_t fileMode)

{

int fd;

fd = open(fileName, fileFlags & ~PG_O_DIRECT, fileMode);

if (fd >= 0)

{

return fd; /* success! */

While
PathNameOpenFilePerm(const char *fileName, int fileFlags, mode_t fileMode)

Is tracked by a LRU list, and the number of open files in that list is
capped.

I wrote about a potential relation with this before in
/messages/by-id/CAE8JnxOyYyv6m=Mz+NwQb=Qj6yFkXD2a+n=+Nes6mhmH6L5PoQ@mail.gmail.com

Regards,
Alexandre

On Mon, Oct 05, 2026 at 10:59:16AM +0100, Alexandre Felipe wrote:

BasicOpenFile() called repeatedly will accumulate stale descriptors
indefinitely.

I don't really see a reason to care about this kind of complexity in
the scope of xlogreader.h: XLogReaderState.WALOpenSegment.ws_file is
the only fd that we need to track.
--
Michael

On Mon, Oct 05, 2026 at 09:20:27AM +0000, Bertrand Drouvot wrote:

The comment above segment_close() says that ws_file shall be set to a negative
number, while both this callback and XLogReaderFree() check for -1. I wonder if
they should test >= 0 instead?

Probably just checking for negative is fine here, yes.

Worth mentioning that this is intended for descriptors not managed by another
cleanup mechanism, such as those returned by BasicOpenFile()? Otherwise, using
it with OpenTransientFile() could result in segment_close() being called with a
stale descriptor.

Hmm. How would you reword this comment?

I am also interested in hearing from Bharath about the latest version
of the patch posted.
--
Michael

Hi,

On Tue, Oct 06, 2026 at 07:27:24AM +0900, Michael Paquier wrote:

On Mon, Oct 05, 2026 at 09:20:27AM +0000, Bertrand Drouvot wrote:

The comment above segment_close() says that ws_file shall be set to a negative
number, while both this callback and XLogReaderFree() check for -1. I wonder if
they should test >= 0 instead?

Probably just checking for negative is fine here, yes.

Yeah.

Worth mentioning that this is intended for descriptors not managed by another
cleanup mechanism, such as those returned by BasicOpenFile()? Otherwise, using
it with OpenTransientFile() could result in segment_close() being called with a
stale descriptor.

Hmm. How would you reword this comment?

Something like?

"
/*
* Register a memory reset callback to close an open WAL segment.
*
* This is intended for unmanaged file descriptors, such as those returned by
* BasicOpenFile(). It should not be used for descriptors subject to another
* cleanup mechanism, such as those returned by OpenTransientFile(), as those
* may already be closed when this callback runs.
*/
"

Regards,

--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

Hi,

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

On Wed, Oct 07, 2026 at 11:34:40AM -0700, Bharath Rupireddy wrote:

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.

Sounds fine to me code-wise. Applied on HEAD after more review and a
few tweaks.
--
Michael

Hi,

On Thu, Oct 08, 2026 at 12:13:39PM +0900, Michael Paquier wrote:

On Wed, Oct 07, 2026 at 11:34:40AM -0700, Bharath Rupireddy wrote:

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.

Sounds fine to me code-wise. Applied on HEAD after more review and a
few tweaks.

Thanks!

As mentioned upthread [1]/messages/by-id/asNr203eue0R4zpZ@bdtpg, I think XLogReaderFree() should also check whether
seg.ws_file >= 0.

Please find attached a small patch doing that.

[1]: /messages/by-id/asNr203eue0R4zpZ@bdtpg

Regards,

--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

Hi,

On Wed, Oct 7, 2026 at 11:55 PM Bertrand Drouvot
<bertranddrouvot.pg@gmail.com> wrote:

As mentioned upthread [1], I think XLogReaderFree() should also check whether
seg.ws_file >= 0.

Please find attached a small patch doing that.

[1]: /messages/by-id/asNr203eue0R4zpZ@bdtpg

Thanks for sending the patch. I don't think we ever receive or set the
ws_file as a non-negative integer other than -1 (neither from the
BasicOpenFilePerm nor from the core's segment_close callbacks), so !=
-1 or >=0 to mean that it is holding the valid fd are correct. Yes, an
external xlogreader can set it to -2 (for example) to mean invalid fd
in their segment_close callback, but I don't think we have anyone
doing that.

I don't have a strong opinion on changing the existing code as it's
not a correctness issue, for consistency reasons across xlogreader.c,
maybe yes. Others may have different thoughts though.

PS: I looked at the slru_io.c which has a mix of both != -1 and >= 0
for file descriptors, maybe leaving it as-is in xlogreader.c is fine.

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

Hi,

On Thu, Oct 08, 2026 at 09:22:11AM -0700, Bharath Rupireddy wrote:

Hi,

On Wed, Oct 7, 2026 at 11:55 PM Bertrand Drouvot
<bertranddrouvot.pg@gmail.com> wrote:

As mentioned upthread [1], I think XLogReaderFree() should also check whether
seg.ws_file >= 0.

Please find attached a small patch doing that.

[1]: /messages/by-id/asNr203eue0R4zpZ@bdtpg

Thanks for sending the patch. I don't think we ever receive or set the
ws_file as a non-negative integer other than -1 (neither from the
BasicOpenFilePerm nor from the core's segment_close callbacks), so !=
-1 or >=0 to mean that it is holding the valid fd are correct. Yes, an
external xlogreader can set it to -2 (for example) to mean invalid fd
in their segment_close callback, but I don't think we have anyone
doing that.

Right, there is no issue for the current in core callbacks. That said, the comment
above segment_close() says that ws_file shall be set to a negative number, not
specifically -1. So an external callback using -2 would follow the documented behavior,
but XLogReaderFree() would invoke segment_close() again.

Using >= 0 also matches the other ws_file checks in xlogreader.c.

PS: I looked at the slru_io.c which has a mix of both != -1 and >= 0
for file descriptors, maybe leaving it as-is in xlogreader.c is fine.

I don't think this is the same case. In slru_io.c, fd is initialized or reset to
-1 (otherwise, it holds a valid file descriptor). So != -1 and >= 0 are equivalent
there.

Regards,

--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com