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
