On 2026-Mar-06, Heikki Linnakangas wrote: > I'm -1 on the last patch, "Expand xact SLRU IO-error to show epoch" though. > The epoch isn't used in addressing the SLRU, the patch just expands the > 32-bit XID into a full 64-bit XID using the current epoch. That seems > misleading.
I agree on that. 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. 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. -- Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/ "If you want to have good ideas, you must have many ideas. Most of them will be wrong, and what you have to learn is which ones to throw away." (Linus Pauling)
