Hi, On Thu, Oct 1, 2026 at 9:57 PM Michael Paquier <[email protected]> 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
