Re: Rework SLRU I/O errors handle

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