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
