Hi, On Thu, Oct 08, 2026 at 09:22:11AM -0700, Bharath Rupireddy wrote: > Hi, > > On Wed, Oct 7, 2026 at 11:55 PM Bertrand Drouvot > <[email protected]> wrote: > > > > As mentioned upthread [1], I think XLogReaderFree() should also check > > whether > > seg.ws_file >= 0. > > > > Please find attached a small patch doing that. > > > > [1]: https://postgr.es/m/asNr203eue0R4zpZ@bdtpg > > Thanks for sending the patch. I don't think we ever receive or set the > ws_file as a non-negative integer other than -1 (neither from the > BasicOpenFilePerm nor from the core's segment_close callbacks), so != > -1 or >=0 to mean that it is holding the valid fd are correct. Yes, an > external xlogreader can set it to -2 (for example) to mean invalid fd > in their segment_close callback, but I don't think we have anyone > doing that.
Right, there is no issue for the current in core callbacks. That said, the comment above segment_close() says that ws_file shall be set to a negative number, not specifically -1. So an external callback using -2 would follow the documented behavior, but XLogReaderFree() would invoke segment_close() again. Using >= 0 also matches the other ws_file checks in xlogreader.c. > PS: I looked at the slru_io.c which has a mix of both != -1 and >= 0 > for file descriptors, maybe leaving it as-is in xlogreader.c is fine. I don't think this is the same case. In slru_io.c, fd is initialized or reset to -1 (otherwise, it holds a valid file descriptor). So != -1 and >= 0 are equivalent there. Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
