Thanks for the comments! On Tue, Feb 24, 2026 at 5:00 AM Chao Li <[email protected]> wrote: > From a design perspective, I’m not sure we need to add a new errormsg > parameter to XLogFindNextRecord(). The new parameter ultimately just exposes > state->errormsg_buf, so the returned errormsg implicitly depends on the > lifetime of state, and we also need extra handling for cases like errormsg == > NULL. > > Instead, perhaps we could add a helper function, say > XLogReaderGetLastError(XLogReaderState *state). which internally pstrdup()s > state->errormsg_buf (after checking errormsg_deferred, etc.). That way the > caller owns the returned string explicitly, and there’s no hidden dependency > on the reader state’s lifetime. > > This would also avoid changing the XLogFindNextRecord() signature while > making the ownership semantics clearer.
One issue I see is that it introduces another way to get the error, with XLogReadRecord and XLogNextRecord using an errormsg parameter, and XLogFindNextRecord using the helper function. Maybe the solution would be to change both XLogReadRecord and XLogNextRecord to use this new function to stay consistent, but that means changing their signatures. Also, I see the errormsg parameter as a way to signal the caller that "this function can fail, the detailed error will be available here". With the XLogReaderGetLastError, it becomes the caller's responsibility to know which function may fill the error message and check it accordingly. The error message is likely printed shortly after the function's call, so I suspect the risk of using the errormsg after its intended lifetime is low. You bring up a good point about the errormsg's lifetime, which is definitely something to mention in the function's comments. I've updated the patch with the additional comments. Regards, Anthonin Bonnefoy
v6-0001-Propagate-errormsg-to-XLogFindNextRecord-caller.patch
Description: Binary data
