> On Feb 12, 2026, at 11:13, Tom Lane <[email protected]> wrote: > > Andres Freund <[email protected]> writes: >> On 2026-02-11 20:34:50 -0500, Tom Lane wrote: >>> I agree that that isn't great code, but what I don't like about it >>> is the separation between where the format string is defined and >>> where it is used. It'd be very easy for the %-escapes to get out of >>> sync with the types of the actual parameters, and if they did, the >>> compiler would not warn you. I think we ought to try to recast this >>> into the normal usage pattern where the format is literal within the >>> errmsg call. I see the comment about avoiding code duplication, but >>> to my mind this is a terrible solution. > >> The amount of code duplication it avoids is rather substantial. > > Really? By my count it's strictly fewer lines to do it the > straightforward way. Yes, I'm counting removal of the comments > defending doing it in the convoluted way, but on the other hand the > attached patch adds quite a few extra line breaks for readability, > and still comes out 4 lines shorter. Not to mention less fragile. > I do not see a reason why the existing code is good. > > regards, tom lane > > diff --git a/src/backend/storage/buffer/bufmgr.c > b/src/backend/storage/buffer/bufmgr.c > index d1babaff023..64220f8a72f 100644 > --- a/src/backend/storage/buffer/bufmgr.c > +++ b/src/backend/storage/buffer/bufmgr.c > @@ -8374,83 +8374,79 @@ buffer_readv_report(PgAioResult result, const > PgAioTargetData *td, > uint8 zeroed_or_error_count, > checkfail_count, > first_off; > - uint8 affected_count; > - const char *msg_one, > - *msg_mult, > - *det_mult, > - *hint_mult; > > buffer_readv_decode_error(result, &zeroed_any, &ignored_any, > &zeroed_or_error_count, > &checkfail_count, > &first_off); > > - /* > - * Treat a read that had both zeroed buffers *and* ignored checksums as a > - * special case, it's too irregular to be emitted the same way as the > - * other cases. > - */ > if (zeroed_any && ignored_any) > { > - Assert(zeroed_any && ignored_any); > Assert(nblocks > 1); /* same block can't be both zeroed and ignored */ > Assert(result.status != PGAIO_RS_ERROR); > - affected_count = zeroed_or_error_count; > - > ereport(elevel, > errcode(ERRCODE_DATA_CORRUPTED), > errmsg("zeroing %u page(s) and ignoring %u checksum failure(s) among blocks > %u..%u of relation \"%s\"", > - affected_count, checkfail_count, first, last, rpath.str), > - affected_count > 1 ? > + zeroed_or_error_count, checkfail_count, first, last, rpath.str), > + zeroed_or_error_count > 1 ? > errdetail("Block %u held the first zeroed page.", > first + first_off) : 0, > errhint_plural("See server log for details about the other %d invalid block.", > "See server log for details about the other %d invalid blocks.", > - affected_count + checkfail_count - 1, > - affected_count + checkfail_count - 1)); > - return; > + zeroed_or_error_count + checkfail_count - 1, > + zeroed_or_error_count + checkfail_count - 1)); > } > - > - /* > - * The other messages are highly repetitive. To avoid duplicating a long > - * and complicated ereport(), gather the translated format strings > - * separately and then do one common ereport. > - */ > - if (result.status == PGAIO_RS_ERROR) > + else if (result.status == PGAIO_RS_ERROR) > { > Assert(!zeroed_any); /* can't have invalid pages when zeroing them */ > - affected_count = zeroed_or_error_count; > - msg_one = _("invalid page in block %u of relation \"%s\""); > - msg_mult = _("%u invalid pages among blocks %u..%u of relation \"%s\""); > - det_mult = _("Block %u held the first invalid page."); > - hint_mult = _("See server log for the other %u invalid block(s)."); > + ereport(elevel, > + errcode(ERRCODE_DATA_CORRUPTED), > + zeroed_or_error_count == 1 ? > + errmsg("invalid page in block %u of relation \"%s\"", > + first + first_off, rpath.str) : > + errmsg("%u invalid pages among blocks %u..%u of relation \"%s\"", > + zeroed_or_error_count, first, last, rpath.str), > + zeroed_or_error_count > 1 ? > + errdetail("Block %u held the first invalid page.", > + first + first_off) : 0, > + zeroed_or_error_count > 1 ? > + errhint("See server log for the other %u invalid block(s).", > + zeroed_or_error_count - 1) : 0); > } > else if (zeroed_any && !ignored_any) > { > - affected_count = zeroed_or_error_count; > - msg_one = _("invalid page in block %u of relation \"%s\"; zeroing out > page"); > - msg_mult = _("zeroing out %u invalid pages among blocks %u..%u of relation > \"%s\""); > - det_mult = _("Block %u held the first zeroed page."); > - hint_mult = _("See server log for the other %u zeroed block(s)."); > + ereport(elevel, > + errcode(ERRCODE_DATA_CORRUPTED), > + zeroed_or_error_count == 1 ? > + errmsg("invalid page in block %u of relation \"%s\"; zeroing out page", > + first + first_off, rpath.str) : > + errmsg("zeroing out %u invalid pages among blocks %u..%u of relation > \"%s\"", > + zeroed_or_error_count, first, last, rpath.str), > + zeroed_or_error_count > 1 ? > + errdetail("Block %u held the first zeroed page.", > + first + first_off) : 0, > + zeroed_or_error_count > 1 ? > + errhint("See server log for the other %u zeroed block(s).", > + zeroed_or_error_count - 1) : 0); > } > else if (!zeroed_any && ignored_any) > { > - affected_count = checkfail_count; > - msg_one = _("ignoring checksum failure in block %u of relation \"%s\""); > - msg_mult = _("ignoring %u checksum failures among blocks %u..%u of relation > \"%s\""); > - det_mult = _("Block %u held the first ignored page."); > - hint_mult = _("See server log for the other %u ignored block(s)."); > + ereport(elevel, > + errcode(ERRCODE_DATA_CORRUPTED), > + checkfail_count == 1 ? > + errmsg("ignoring checksum failure in block %u of relation \"%s\"", > + first + first_off, rpath.str) : > + errmsg("ignoring %u checksum failures among blocks %u..%u of relation > \"%s\"", > + checkfail_count, first, last, rpath.str), > + checkfail_count > 1 ? > + errdetail("Block %u held the first ignored page.", > + first + first_off) : 0, > + checkfail_count > 1 ? > + errhint("See server log for the other %u ignored block(s).", > + checkfail_count - 1) : 0); > } > else > pg_unreachable(); > - > - ereport(elevel, > - errcode(ERRCODE_DATA_CORRUPTED), > - affected_count == 1 ? > - errmsg_internal(msg_one, first + first_off, rpath.str) : > - errmsg_internal(msg_mult, affected_count, first, last, rpath.str), > - affected_count > 1 ? errdetail_internal(det_mult, first + first_off) : 0, > - affected_count > 1 ? errhint_internal(hint_mult, affected_count - 1) : 0); > } > > static void
Thank you, Tom and Andres, for clarifying the translation mechanism, that helped me understand it better. Given that, the patch is no longer about any misuse of errmsg_internal(), but turns to be a refactoring. I’ve prepared a v2 accordingly. It incorporates the diff Tom attached, and as Tom suggested earlier, I’ve also added a brief clarification to the header comment of errmsg_internal(). I also noticed that Tom’s diff removes a redundant Assert(), so it ends up doing slightly more than a pure refactoring. Best regards, -- Chao Li (Evan) HighGo Software Co., Ltd. https://www.highgo.com/
v2-0001-bufmgr-Inline-error-messages-and-clarify-errmsg_i.patch
Description: Binary data
