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
