Horton

Thread

Commits

  1. Make some xlogreader messages more accurate

  1. Make some xlogreader messages more accurate

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2023-02-23T07:35:47Z

    Here is a small patch to make some invalid-record error messages in 
xlogreader a bit more accurate (IMO).

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 also added that detail to one message where 
it was available but not printed.

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.

  2. Re: Make some xlogreader messages more accurate

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2023-02-28T06:15:38Z

    On Thu, Feb 23, 2023 at 1:06 PM Peter Eisentraut
<peter.eisentraut@enterprisedb.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

  3. Re: Make some xlogreader messages more accurate

    Jeevan Ladhe <jeevanladhe.os@gmail.com> — 2023-02-28T10:19:18Z

    +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.rupireddyforpostgres@gmail.com> wrote:

> On Thu, Feb 23, 2023 at 1:06 PM Peter Eisentraut
> <peter.eisentraut@enterprisedb.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
>
>
>

  4. Re: Make some xlogreader messages more accurate

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2023-03-02T08:19:52Z

    On 28.02.23 11:19, Jeevan Ladhe wrote:
> +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".

I committed this with "at least", as suggested, and also changed 
"wanted" to "expected", which matches the usual error message style better.

  5. Re: Make some xlogreader messages more accurate

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2023-03-02T08:21:55Z

    On 28.02.23 07:15, Bharath Rupireddy wrote:
>> 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.

Right, this isn't handled very consistently.  I did a pass across all 
"{invalid|incorrect|wrong} {length|size}" messages and tried to make 
them more precise by adding more detail and using the appropriate word. 
What do you think about the attached patch?

  6. Re: Make some xlogreader messages more accurate

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2023-03-02T11:52:40Z

    On Thu, Mar 2, 2023 at 1:51 PM Peter Eisentraut
<peter.eisentraut@enterprisedb.com> wrote:
>
> On 28.02.23 07:15, Bharath Rupireddy wrote:
> >> 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.
>
> Right, this isn't handled very consistently.  I did a pass across all
> "{invalid|incorrect|wrong} {length|size}" messages and tried to make
> them more precise by adding more detail and using the appropriate word.
> What do you think about the attached patch?

Thanks. IMO, when any of the incorrect/invalid/wrong errors occur, the
wording may not matter much more than the error itself and why it
occurred. While the uniformity of this kind helps, I think it's hard
to enforce the same/similar wording in future. I prefer leaving the
code as-is. Therefore, -1 for these changes.

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