Message-ID: <74f5dc14-29fd-46d9-ba8b-2844328d7bdf@iki.fi>
Date: Fri, 6 Mar 2026 18:22:42 +0200
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: Rework SLRU I/O errors handle
To: =?UTF-8?Q?=C3=81lvaro_Herrera?= <alvherre@kurilemu.de>
Cc: Maxim Orlov <orlovmg@gmail.com>,
 Postgres hackers <pgsql-hackers@lists.postgresql.org>
References: <202603061507.6ynh6qimlcma@alvherre.pgsql>
Content-Language: en-US
From: Heikki Linnakangas <hlinnaka@iki.fi>
In-Reply-To: <202603061507.6ynh6qimlcma@alvherre.pgsql>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Archived-At: 
 <https://www.postgresql.org/message-id/74f5dc14-29fd-46d9-ba8b-2844328d7bdf%40iki.fi>
Precedence: bulk

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