Hi, On Sun, Oct 4, 2026 at 10:52 PM Michael Paquier <[email protected]> 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
From 8ee0b91b3154d9095eb483bb783fdc4435492074 Mon Sep 17 00:00:00 2001 From: Bharath Rupireddy <[email protected]> Date: Wed, 7 Oct 2026 18:31:15 +0000 Subject: [PATCH v4] Fix WAL segment file descriptor leak on WAL read errors. When an XLogReader opens a WAL segment file with BasicOpenFile(), which doesn't have the automatic file descriptor cleanup that OpenTransientFile() provides, the fd can leak if an ERROR happens while reading WAL segments, for example when pg_walinspect is given buggy input or a logical decoding function errors out partway through. The fd leak can lead to more severe errors like a "Too many open files" PANIC, taking down the database instance. Fix this by registering a memory context reset callback in all the segment_open callbacks that the core XLogReaders have, which closes the fd that was left open before reaching XLogReaderFree(). The reset callback is placed in three of the core XLogReaders: xlogutils.c, the WAL summarizer and the WAL sender. This fix is not back-patched, for ABI compatibility reasons (XLogReaderState struct changes) and because the lack of a memory context unregister reset callback in the PG18 and older branches would require more invasive changes. Author: Bharath Rupireddy <[email protected]> Reviewed-by: Michael Paquier <[email protected]> Reviewed-by: Bertrand Drouvot <[email protected]> Reviewed-by: Sami Imseih <[email protected]> Discussion: CALj2ACVwDuOXXDjj2cVdnTKoxsgTSLDin4XoL63AnM6aUgMQaA@mail.gmail.com">https://postgr.es/m/CALj2ACVwDuOXXDjj2cVdnTKoxsgTSLDin4XoL63AnM6aUgMQaA@mail.gmail.com --- src/backend/access/transam/xlogreader.c | 47 +++++++++++++++++++++++++ src/backend/access/transam/xlogutils.c | 3 ++ src/backend/postmaster/walsummarizer.c | 1 + src/backend/replication/walsender.c | 3 ++ src/include/access/xlogreader.h | 15 +++++++- 5 files changed, 68 insertions(+), 1 deletion(-) diff --git a/src/backend/access/transam/xlogreader.c b/src/backend/access/transam/xlogreader.c index 7db7c273b0c..53025fcf64d 100644 --- a/src/backend/access/transam/xlogreader.c +++ b/src/backend/access/transam/xlogreader.c @@ -36,6 +36,7 @@ #ifndef FRONTEND #include "pgstat.h" #include "storage/bufmgr.h" +#include "utils/memutils.h" #include "utils/wait_event.h" #else #include "common/logging.h" @@ -55,6 +56,9 @@ static bool ValidXLogRecord(XLogReaderState *state, XLogRecord *record, static void ResetDecoder(XLogReaderState *state); static void WALOpenSegmentInit(WALOpenSegment *seg, WALSegmentContext *segcxt, int segsize, const char *waldir); +#ifndef FRONTEND +static void xlogreader_memory_context_reset_cb(void *arg); +#endif /* size of the buffer allocated for error message. */ #define MAX_ERRORMSG_LEN 1000 @@ -159,9 +163,52 @@ XLogReaderAllocate(int wal_segment_size, const char *waldir, return state; } +#ifndef FRONTEND +/* + * Memory context reset callback for an XLogReader. + */ +static void +xlogreader_memory_context_reset_cb(void *arg) +{ + XLogReaderState *state = (XLogReaderState *) arg; + + /* Close the WAL segment file that was left open */ + if (state->seg.ws_file >= 0) + state->routine.segment_close(state); +} + +/* + * Register a callback to perform cleanup or release resources when the memory + * context holding the XLogReader is reset or deleted. + * + * This is useful when opening a WAL segment file with BasicOpenFile(), which + * doesn't have the automatic file descriptor cleanup that OpenTransientFile() + * provides, to guarantee that the segment is closed before XLogReaderFree() is + * reached (on ERRORs, for example). + */ +void +XLogReaderRegisterResetCallback(XLogReaderState *state) +{ + if (state->reset_cb_registered) + return; + + state->reset_cb.func = xlogreader_memory_context_reset_cb; + state->reset_cb.arg = state; + MemoryContextRegisterResetCallback(GetMemoryChunkContext(state), + &state->reset_cb); + state->reset_cb_registered = true; +} +#endif + void XLogReaderFree(XLogReaderState *state) { +#ifndef FRONTEND + if (state->reset_cb_registered) + MemoryContextUnregisterResetCallback(GetMemoryChunkContext(state), + &state->reset_cb); +#endif + if (state->seg.ws_file != -1) state->routine.segment_close(state); diff --git a/src/backend/access/transam/xlogutils.c b/src/backend/access/transam/xlogutils.c index 58b9dab6a90..ff71e14015d 100644 --- a/src/backend/access/transam/xlogutils.c +++ b/src/backend/access/transam/xlogutils.c @@ -836,7 +836,10 @@ wal_segment_open(XLogReaderState *state, XLogSegNo nextSegNo, XLogFilePath(path, tli, nextSegNo, state->segcxt.ws_segsize); state->seg.ws_file = BasicOpenFile(path, O_RDONLY | PG_BINARY); if (state->seg.ws_file >= 0) + { + XLogReaderRegisterResetCallback(state); return; + } if (errno == ENOENT) ereport(ERROR, diff --git a/src/backend/postmaster/walsummarizer.c b/src/backend/postmaster/walsummarizer.c index ff246b07a21..8333a4eaaa3 100644 --- a/src/backend/postmaster/walsummarizer.c +++ b/src/backend/postmaster/walsummarizer.c @@ -1608,6 +1608,7 @@ summarizer_wal_segment_open(XLogReaderState *state, XLogSegNo nextSegNo, state->seg.ws_file = BasicOpenFile(path, O_RDONLY | PG_BINARY); if (state->seg.ws_file >= 0) { + XLogReaderRegisterResetCallback(state); *tli_p = tli; return; } diff --git a/src/backend/replication/walsender.c b/src/backend/replication/walsender.c index e9331de3df5..10bc3e0cae4 100644 --- a/src/backend/replication/walsender.c +++ b/src/backend/replication/walsender.c @@ -3346,7 +3346,10 @@ WalSndSegmentOpen(XLogReaderState *state, XLogSegNo nextSegNo, XLogFilePath(path, *tli_p, nextSegNo, state->segcxt.ws_segsize); state->seg.ws_file = BasicOpenFile(path, O_RDONLY | PG_BINARY); if (state->seg.ws_file >= 0) + { + XLogReaderRegisterResetCallback(state); return; + } /* * If the file is not found, assume it's because the standby asked for a diff --git a/src/include/access/xlogreader.h b/src/include/access/xlogreader.h index 4a9a687e879..45b8050a402 100644 --- a/src/include/access/xlogreader.h +++ b/src/include/access/xlogreader.h @@ -109,7 +109,9 @@ typedef struct XLogReaderRoutine /* * WAL segment close callback. ->seg.ws_file shall be set to a negative - * number. + * number. This shall not raise an error, as it may be called in a memory + * context reset callback if XLogReaderRegisterResetCallback() has been + * used. */ WALSegmentCloseCB segment_close; } XLogReaderRoutine; @@ -315,6 +317,12 @@ struct XLogReaderState * data. */ bool nonblocking; + +#ifndef FRONTEND + /* Reset callback for the memory context holding this reader. */ + MemoryContextCallback reset_cb; + bool reset_cb_registered; +#endif }; /* @@ -335,6 +343,11 @@ extern XLogReaderState *XLogReaderAllocate(int wal_segment_size, /* Free an XLogReader */ extern void XLogReaderFree(XLogReaderState *state); +#ifndef FRONTEND +/* Register a memory context reset callback */ +extern void XLogReaderRegisterResetCallback(XLogReaderState *state); +#endif + /* Optionally provide a circular decoding buffer to allow readahead. */ extern void XLogReaderSetDecodeBuffer(XLogReaderState *state, void *buffer, -- 2.47.3
