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
From d8e6ee3995e0f0596b3c72e6ea8833df62f0c4e7 Mon Sep 17 00:00:00 2001 From: Michael Paquier <[email protected]> Date: Mon, 5 Oct 2026 14:42:56 +0900 Subject: [PATCH v3] Fix WAL segment file descriptor leak on WAL read errors. The segment_open callback of an XLogReader uses BasicOpenFile() in three places of the code tree: xlogutils.c (most common callback), WAL summarizer and WAL sender. These would leak file descriptors if an ERROR happens while reading WAL segments, for example by using pg_walinspect with buggy input. This problem is tackled with the introduction of a memory reset callback, settable by segment_open callbacks when these require a more aggressive close of the fds opened. And a bit more blah. Author: Bharath Rupireddy <[email protected]> Reviewed-by: Michael Paquier <[email protected]> Discussion: https://postgr.es/m/calj2acvwduoxxdjj2cvdntkoxsgtsldin4xol63anm6augm...@mail.gmail.com --- src/include/access/xlogreader.h | 16 ++++++++- src/backend/access/transam/xlogreader.c | 46 +++++++++++++++++++++++++ src/backend/access/transam/xlogutils.c | 3 ++ src/backend/postmaster/walsummarizer.c | 1 + src/backend/replication/walsender.c | 3 ++ 5 files changed, 68 insertions(+), 1 deletion(-) diff --git a/src/include/access/xlogreader.h b/src/include/access/xlogreader.h index 4a9a687e8796..7c8e44fb4646 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 should not throw an error, as it may be called in a + * memory context reset callback if XLogReaderRegisterReset() has been + * used. */ WALSegmentCloseCB segment_close; } XLogReaderRoutine; @@ -315,6 +317,13 @@ struct XLogReaderState * data. */ bool nonblocking; + +#ifndef FRONTEND + + /* Reset callback on the memory context holding this reader. */ + MemoryContextCallback reset_cb; + bool reset_cb_registered; +#endif }; /* @@ -335,6 +344,11 @@ extern XLogReaderState *XLogReaderAllocate(int wal_segment_size, /* Free an XLogReader */ extern void XLogReaderFree(XLogReaderState *state); +#ifndef FRONTEND +/* Register a reset callback */ +extern void XLogReaderRegisterReset(XLogReaderState *state); +#endif + /* Optionally provide a circular decoding buffer to allow readahead. */ extern void XLogReaderSetDecodeBuffer(XLogReaderState *state, void *buffer, diff --git a/src/backend/access/transam/xlogreader.c b/src/backend/access/transam/xlogreader.c index 7db7c273b0c6..64fc1c9428de 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_close_segment(void *arg); +#endif /* size of the buffer allocated for error message. */ #define MAX_ERRORMSG_LEN 1000 @@ -159,9 +163,51 @@ XLogReaderAllocate(int wal_segment_size, const char *waldir, return state; } +#ifndef FRONTEND +/* + * Memory reset callback for an XLogReader. + * + * Close the WAL segment file when the memory context holding the reader is + * reset or deleted. + */ +static void +xlogreader_close_segment(void *arg) +{ + XLogReaderState *state = (XLogReaderState *) arg; + + if (state->seg.ws_file != -1) + state->routine.segment_close(state); +} + +/* + * 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) +{ + if (state->reset_cb_registered) + return; + + state->reset_cb.func = xlogreader_close_segment; + 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 58b9dab6a908..15ef279b1ed0 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) + { + XLogReaderRegisterReset(state); return; + } if (errno == ENOENT) ereport(ERROR, diff --git a/src/backend/postmaster/walsummarizer.c b/src/backend/postmaster/walsummarizer.c index ff246b07a212..76e50a7c97c5 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) { + XLogReaderRegisterReset(state); *tli_p = tli; return; } diff --git a/src/backend/replication/walsender.c b/src/backend/replication/walsender.c index e9331de3df54..030a6f5f5810 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) + { + XLogReaderRegisterReset(state); return; + } /* * If the file is not found, assume it's because the standby asked for a -- 2.55.0
signature.asc
Description: PGP signature
