> On Feb 13, 2026, at 05:02, Andres Freund <[email protected]> wrote: > > > Hi, > > On 2026-02-12 20:25:19 +0000, Zsolt Parragi wrote: >>> No, I don't think so. This is just about errors on the bufmgr layer. >> >> I see. Looks like I misinterpreted the comment in md.c where it sets >> this flag when it reads 0 blocks. > > With AIO an IO can fail at multiple levels, so the error stored in the handle > is associated with the layer at which the error was "diagnosed". > > If we e.g. were to support am smgr implementation other than md.c it might > need very different IO error conditions (e.g. network failures if a networked > smgr implementation) than md.c, but the errors at the level of bufmgr.c would > stay the same as today. > > It's also conceivable that a higher layer could just ignore the error by a > lower layer and would thereby just "override" the lower layer's error. E.g. a > non-existant freespacemap could be recreated by freespacemap.c after a failure > to read, instead of having to check how large the on-disk FSM is before trying > to read it (check fsm_readbuf() for how it currently works). > > >>> I apparently may be alone in this, but I find 6 repetitions of ereports, >>> with >>> differently indented messages and arguments, depending on whether it's an >>> errmsg, errdetails, errhint way harder to scan and modify than something >>> that >>> just shows the different messages with consistent indentation. >> >> Is changing the messages to follow the same pattern an option? >> >> For example the error messages: >> >> "read error in block %u of relation \"%s\": %s" >> "%u read errors among blocks %u..%u of relation \"%s\": %s" >> >> When the last string is conditional: >> >> * invalid page(s) >> * zeroing out invalid page(s) >> * ignoring checksum error(s) > > That gets complicated with translation, because you need to translate the %s > arguments need to be translated separately, as the translation cannot be done > together with the error/detail message. Doable, but it's not an no-cost win. > > > >> or maybe >> >> "read error: %s in block %u of relation \"%s\"" > > That's not really following our message guidelines... I'm not sure I believe > in all the tenants stated in our guidelines, but we do try to follow them... > > Greetings, > > Andres Freund
The discussion has gone quite a bit beyond the original patch intention. I
always appreciate the insights from these exchanges. Since I started the
thread, let me try to follow up with an updated version.
PFA v3:
* I tried to simplify the logging logic in buffer_readv_report() by
leveraging errmsg_plural() and introducing a function-scoped macro to reduce
the repetitive ereport() structure. I kept in mind that we should not use “%s”
to inject the failure type (e.g., “invalid page”), as that would break
translation. But I am not entirely certain whether using a macro in this way
could have also broken translation; if that turns out to be a concern, I am
happy to rework it.
* I also removed the repeated wording in the comment that Andres pointed
out.
At this point, the only change that relates directly to the original patch
topic is the header comment adjustment for errmsg_internal(), which is
otherwise unrelated to the refactoring in bufmgr.c. I am fine with either
splitting that out into a separate patch or dropping it altogether, whichever
is preferred.
Please let me know if this direction makes sense, or if further simplification
would be better.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
v3-0001-Simplify-buffer_readv_report-error-message-genera.patch
Description: Binary data
