Make some xlogreader messages more accurate
Hackorum builds and tests every patch posted to the lists, not only commitfest submissions. This is Hackorum's own CI rather than the PostgreSQL project's, and it is still under testing - please report anything that looks wrong.
You can run a PostgreSQL built from this patch straight from Docker, with no checkout and no build:
docker run --rm -p 5432:5432 ghcr.io/hackorum-dev/postgres-patch:t47506psql -h localhost -U postgresBuilt from patchset v5 (message #5), August 18, 2026 at 03:31 PM.
Every patchset is also pushed to a branch of our PostgreSQL fork, so you can check out the same tree CI built. Without a PostgreSQL checkout:
git clone --branch t47506_5 https://github.com/hackorum-dev/postgres.gitIn a checkout you already have, add the fork once:
git remote add hackorum https://github.com/hackorum-dev/postgres.gitthen, for this patchset and every later one:
git fetch hackorum t47506_5 && git checkout t47506_5Patchset v5 (message #5) is on t47506_5
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.
Attachments:
0001-Make-some-xlogreader-messages-more-accurate.patchtext/plain; charset=UTF-8; name=0001-Make-some-xlogreader-messages-more-accurate.patchDownload+6-6
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]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 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
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
+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:
Show quoted text
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, andYes, 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
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.
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?
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