On 06/03/2026 18:38, Heikki Linnakangas wrote:
> On 06/03/2026 18:22, Heikki Linnakangas wrote:
>> On 06/03/2026 17:33, Álvaro Herrera wrote:
>>> 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.
>
> Attached version that does that.
That version was a little rushed, with wrong message styles etc.
leftovers. Sorry about that, here's yet another, more cleaned-up version.
- Heikki
