Re: Make some xlogreader messages more accurate

+1 for the changes.

>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.

I think I agree with Bharath on this: "wanted at least %u" sounds better
for user error than "wanted >=%u".

Regards,
Jeevan Ladhe

On Tue, 28 Feb 2023 at 11:46, Bharath Rupireddy <
bharath(dot)rupireddyforpostgres(at)gmail(dot)com> wrote:

> 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
>
>
>