On 06/03/2026 17:33, Álvaro Herrera wrote:
I'm not a fan of the split. I think it all these patches should be pushed as a single commit, and avoid introducing xact_errmsg_for_io_error as an exposed function. I think that doesn't make a lot of sense. Each SLRU should have a correct and appropriate error reporting callback.
Agreed.
The comment added in 0005 is bogus too. It mentions InvalidTransactionId, but the problem is not that the value is 0 but rather that we get no pointer. Also, in all other callbacks the pointer is asserted to not be NULL, so why don't we do the same here, and avoid an error message that's not going to help anyone? I see however that in the patch we're passing a NULL to SlruReportIOError(), which means if you get an IO error with any SLRU other than xact, you're going to get either a crash on the assertion, or (on non-debug builds) a crash on dereferencing the NULL pointer.
Hmm, I thought we could just never pass a NULL pointer, but if a Write fails, slru.c has no context where to pull that opaque_data. Perhaps we should just not call the callback in that case.
I'm starting to feel that what SlruReportIOError() currently uses as the errdetail, could well be the main error message, and the callback could provide the errdetail. I.e. swap the errmsg and errdetail we have now. That way, we can just leave out the errdetail for Write failures. The current errmsg we have for Write failures is pretty bad: "ERROR: could not access status of transaction 0". What's currently the errdetail, e.g. "Could not read from file \"%s\" at offset %d: %m", is a lot more informative.
- Heikki
