Re: Make some xlogreader messages more accurate

On Thu, Feb 23, 2023 at 1:06 PM Peter Eisentraut
<peter(dot)eisentraut(at)enterprisedb(dot)com> wrote:
>
> Here is a small patch to make some invalid-record error messages in
> xlogreader a bit more accurate (IMO).

+1 for these changes.

> My starting point was that when you have some invalid WAL, you often get
> a message like "wanted 24, got 0". This is a bit incorrect, since it
> really wanted *at least* 24, not exactly 24. So I have updated the
> messages to that effect, and

Yes, it's not exactly "wanted", but "wanted at least" because
xl_tot_len is the total length of the entire record including header
and payload.

> also added that detail to one message where
> it was available but not printed.

Looks okay.

> Going through the remaining report_invalid_record() calls I then
> adjusted the use of "invalid" vs. "incorrect" in one case. The message
> "record with invalid length" makes it sound like the length was
> something like -5, but really we know what the length should be and what
> we got wasn't it, so "incorrect" sounded better and is also used in
> other error messages in that file.

I have no strong opinion about this change. We seem to be using
"invalid length" and "incorrect length" interchangeably [1] without
distinguishing between "invalid" if length is < 0 and "incorrect" if
length >= 0 and not something we're expecting.

Another comment on the patch:
1. Why is "wanted >=%u" any better than "wanted at least %u"? IMO, the
wording as opposed to >= symbol in the user-facing messages works
better.
+ report_invalid_record(state, "invalid record offset at %X/%X:
wanted >=%u, got %u",
+ "invalid record length at %X/%X:
wanted >=%u, got %u",
+ "invalid record length at %X/%X: wanted
>=%u, got %u",

[1]
elog(ERROR, "incorrect length %d in streaming transaction's changes
file \"%s\"",
"record with invalid length at %X/%X",
(errmsg("invalid length of checkpoint record")));
errmsg("invalid length of startup packet")));
errmsg("invalid length of startup packet")));
elog(ERROR, "invalid zero-length dimension array in MCVList");
elog(ERROR, "invalid length (%d) dimension array in MCVList",
errmsg("invalid length in external \"%s\" value",
errmsg("invalid length in external bit string")));
libpq_append_conn_error(conn, "certificate contains IP address with
invalid length %zu

--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com