pg.ddx.io pgsql-bugs@postgresql.org mailing list archive
help / color / mirror / Atom feedBUG #18224: message bug in libpqwalreceiver.c.
10+ messages / 5 participants
[nested] [flat]
Commits from this thread:
2023-12-12 c5962ad21a 5902ba5b21 00b222226f 810c9609f8 419b6cb885 d86038bd4d Fix typo in comment
* BUG #18224: message bug in libpqwalreceiver.c.
@ 2023-12-04 11:24 PG Bug reporting form <noreply@postgresql.org>
0 siblings, 1 reply; 10+ messages in thread
From: PG Bug reporting form @ 2023-12-04 11:24 UTC (permalink / raw)
To: pgsql-bugs@lists.postgresql.org; +Cc: t.katsumata1122@gmail.com
The following bug has been logged on the website:
Bug reference: 18224
Logged by: Tomonari Katsumata
Email address: t.katsumata1122@gmail.com
PostgreSQL version: 16.1
Operating system: any
Description:
Hello
I found a small bug in the error message of libpqwalreceiver.c.
According to the manual, identify_system returns one row and
four columns. (This is also the actual behavior)
https://www.postgresql.org/docs/current/protocol-replication.html#PROTOCOL-REPLICATION-IDENTIFY-SYST...
However, when data other than one row and four columns is returned,
incorrect values are reported as expected values as shown below.
＞Could not identify system: got X rows and Y fields, expected 3 rows and 1
or more fields.
It looks like the arguments are in the wrong order.
Also, it seems that the decision to return 4 fields in 9.4 was not
reflected.
This is a trivial issue, but I was curious about it so I reported it.
Best regards,
----
Tomonari Katsumata
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: BUG #18224: message bug in libpqwalreceiver.c.
@ 2023-12-04 11:28 king tomo <t.katsumata1122@gmail.com>
parent: PG Bug reporting form <noreply@postgresql.org>
0 siblings, 1 reply; 10+ messages in thread
From: king tomo @ 2023-12-04 11:28 UTC (permalink / raw)
To: t.katsumata1122@gmail.com; pgsql-bugs@lists.postgresql.org
Hello
This is a simplest patch.
Please check it.
Best regards,
----
Tomonari Katsumata
2023年12月4日(月) 20:25 PG Bug reporting form <noreply@postgresql.org>:
> The following bug has been logged on the website:
>
> Bug reference: 18224
> Logged by: Tomonari Katsumata
> Email address: t.katsumata1122@gmail.com
> PostgreSQL version: 16.1
> Operating system: any
> Description:
>
> Hello
>
> I found a small bug in the error message of libpqwalreceiver.c.
>
> According to the manual, identify_system returns one row and
> four columns. (This is also the actual behavior)
>
> https://www.postgresql.org/docs/current/protocol-replication.html#PROTOCOL-REPLICATION-IDENTIFY-SYST...
>
> However, when data other than one row and four columns is returned,
> incorrect values are reported as expected values as shown below.
>
> ＞Could not identify system: got X rows and Y fields, expected 3 rows and 1
> or more fields.
>
> It looks like the arguments are in the wrong order.
> Also, it seems that the decision to return 4 fields in 9.4 was not
> reflected.
>
> This is a trivial issue, but I was curious about it so I reported it.
>
> Best regards,
> ----
> Tomonari Katsumata
>
>
Attachments:
identify_system_message_revise.patch (application/octet-stream, 1.2K) download
diff --git a/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c b/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
index 60d5c1fc40..44b08ff772 100644
--- a/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
+++ b/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
@@ -381,7 +381,7 @@ libpqrcv_identify_system(WalReceiverConn *conn, TimeLineID *primary_tli)
"the primary server: %s",
pchomp(PQerrorMessage(conn->streamConn)))));
}
- if (PQnfields(res) < 3 || PQntuples(res) != 1)
+ if (PQnfields(res) < 4 || PQntuples(res) != 1)
{
int ntuples = PQntuples(res);
int nfields = PQnfields(res);
@@ -390,8 +390,8 @@ libpqrcv_identify_system(WalReceiverConn *conn, TimeLineID *primary_tli)
ereport(ERROR,
(errcode(ERRCODE_PROTOCOL_VIOLATION),
errmsg("invalid response from primary server"),
- errdetail("Could not identify system: got %d rows and %d fields, expected %d rows and %d or more fields.",
- ntuples, nfields, 3, 1)));
+ errdetail("Could not identify system: got %d rows and %d fields, expected %d row and %d or more fields.",
+ ntuples, nfields, 1, 4)));
}
primary_sysid = pstrdup(PQgetvalue(res, 0, 0));
*primary_tli = pg_strtoint32(PQgetvalue(res, 0, 1));
* Re: BUG #18224: message bug in libpqwalreceiver.c.
@ 2023-12-04 12:12 Daniel Gustafsson <daniel@yesql.se>
parent: king tomo <t.katsumata1122@gmail.com>
0 siblings, 1 reply; 10+ messages in thread
From: Daniel Gustafsson @ 2023-12-04 12:12 UTC (permalink / raw)
To: king tomo <t.katsumata1122@gmail.com>; +Cc: pgsql-bugs@lists.postgresql.org
> On 4 Dec 2023, at 12:28, king tomo <t.katsumata1122@gmail.com> wrote:
> This is a simplest patch.
> Please check it.
This dates back to 5a991ef8692e from March 2014 which has this hunk:
- errdetail("Expected 1 tuple with 3 fields, got %d tuples with %d fields.",
- ntuples, nfields)));
+ errdetail("Could not identify system: Got %d rows and %d fields, expected %d rows and %d or more fields.",
+ ntuples, nfields, 3, 1)));
> Also, it seems that the decision to return 4 fields in 9.4 was not
> reflected.
Actually it was, 083d29c65b78 changed it like your patch but it was then later
reverted in c4762886539b since it prevents using replication related utils
against older servers. Maybe it's time to drop this backwards compatibility
now but that shouldn't be hidden in an error message fixup patch. I've changed
it to add a comment instead which explains why we check for < 3 and write that
we expect 4 in the error message. It could be argued that we should say that
we expect "3 or more" fields but given the age of the change we clearly do
expect 4 at this point.
I'll apply this backpatched all the way down unless there are objections.
--
Daniel Gustafsson
Attachments:
identify_system_msg.diff (application/octet-stream, 1.1K) download
diff --git a/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c b/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
index 60d5c1fc40..b402379d12 100644
--- a/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
+++ b/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
@@ -381,6 +381,10 @@ libpqrcv_identify_system(WalReceiverConn *conn, TimeLineID *primary_tli)
"the primary server: %s",
pchomp(PQerrorMessage(conn->streamConn)))));
}
+ /*
+ * IDENTIFY_SERVER returns 3 columns in 9.3 and earlier, and 4 columns in
+ * 9.4 and onwards.
+ */
if (PQnfields(res) < 3 || PQntuples(res) != 1)
{
int ntuples = PQntuples(res);
@@ -391,7 +395,7 @@ libpqrcv_identify_system(WalReceiverConn *conn, TimeLineID *primary_tli)
(errcode(ERRCODE_PROTOCOL_VIOLATION),
errmsg("invalid response from primary server"),
errdetail("Could not identify system: got %d rows and %d fields, expected %d rows and %d or more fields.",
- ntuples, nfields, 3, 1)));
+ ntuples, nfields, 1, 4)));
}
primary_sysid = pstrdup(PQgetvalue(res, 0, 0));
*primary_tli = pg_strtoint32(PQgetvalue(res, 0, 1));
* Re: BUG #18224: message bug in libpqwalreceiver.c.
@ 2023-12-04 14:07 Tom Lane <tgl@sss.pgh.pa.us>
parent: Daniel Gustafsson <daniel@yesql.se>
0 siblings, 1 reply; 10+ messages in thread
From: Tom Lane @ 2023-12-04 14:07 UTC (permalink / raw)
To: Daniel Gustafsson <daniel@yesql.se>; +Cc: king tomo <t.katsumata1122@gmail.com>; pgsql-bugs@lists.postgresql.org
Daniel Gustafsson <daniel@yesql.se> writes:
> I've changed
> it to add a comment instead which explains why we check for < 3 and write that
> we expect 4 in the error message. It could be argued that we should say that
> we expect "3 or more" fields but given the age of the change we clearly do
> expect 4 at this point.
I disagree with making this error message lie about what the test was.
Clearly we need to s/3, 1/1, 3/ but I don't think the number should
be different from what we actually allow.
Having said that, maybe there's a case for requiring 4 columns now.
I agree it's pretty unlikely that current releases would see a pre-9.4
server on the other end of the line.
regards, tom lane
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: BUG #18224: message bug in libpqwalreceiver.c.
@ 2023-12-04 14:10 Daniel Gustafsson <daniel@yesql.se>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 10+ messages in thread
From: Daniel Gustafsson @ 2023-12-04 14:10 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: king tomo <t.katsumata1122@gmail.com>; pgsql-bugs@lists.postgresql.org
> On 4 Dec 2023, at 15:07, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Daniel Gustafsson <daniel@yesql.se> writes:
>> I've changed
>> it to add a comment instead which explains why we check for < 3 and write that
>> we expect 4 in the error message. It could be argued that we should say that
>> we expect "3 or more" fields but given the age of the change we clearly do
>> expect 4 at this point.
>
> I disagree with making this error message lie about what the test was.
> Clearly we need to s/3, 1/1, 3/ but I don't think the number should
> be different from what we actually allow.
Fair enough.
> Having said that, maybe there's a case for requiring 4 columns now.
> I agree it's pretty unlikely that current releases would see a pre-9.4
> server on the other end of the line.
How about fixing the error message in the backbranches, with a comment why we
expect 3 and not 4 fields, and requiring 4 thus restricting to 9.4+ in HEAD?
--
Daniel Gustafsson
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: BUG #18224: message bug in libpqwalreceiver.c.
@ 2023-12-04 14:18 Tom Lane <tgl@sss.pgh.pa.us>
parent: Daniel Gustafsson <daniel@yesql.se>
0 siblings, 2 replies; 10+ messages in thread
From: Tom Lane @ 2023-12-04 14:18 UTC (permalink / raw)
To: Daniel Gustafsson <daniel@yesql.se>; +Cc: king tomo <t.katsumata1122@gmail.com>; pgsql-bugs@lists.postgresql.org
Daniel Gustafsson <daniel@yesql.se> writes:
> How about fixing the error message in the backbranches, with a comment why we
> expect 3 and not 4 fields, and requiring 4 thus restricting to 9.4+ in HEAD?
OK by me.
regards, tom lane
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: BUG #18224: message bug in libpqwalreceiver.c.
@ 2023-12-05 11:50 king tomo <t.katsumata1122@gmail.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
1 sibling, 0 replies; 10+ messages in thread
From: king tomo @ 2023-12-05 11:50 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Daniel Gustafsson <daniel@yesql.se>; pgsql-bugs@lists.postgresql.org
Hello
Thank you for researching the past and for discussing it.
There is no problem with the modification policy suggested by Daniel.
Thank you for your continued support.
Best regards,
----
Tomonari Katsumata
2023年12月4日(月) 23:18 Tom Lane <tgl@sss.pgh.pa.us>:
> Daniel Gustafsson <daniel@yesql.se> writes:
> > How about fixing the error message in the backbranches, with a comment
> why we
> > expect 3 and not 4 fields, and requiring 4 thus restricting to 9.4+ in
> HEAD?
>
> OK by me.
>
> regards, tom lane
>
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: BUG #18224: message bug in libpqwalreceiver.c.
@ 2023-12-05 13:44 Daniel Gustafsson <daniel@yesql.se>
parent: Tom Lane <tgl@sss.pgh.pa.us>
1 sibling, 1 reply; 10+ messages in thread
From: Daniel Gustafsson @ 2023-12-05 13:44 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: king tomo <t.katsumata1122@gmail.com>; pgsql-bugs@lists.postgresql.org
> On 4 Dec 2023, at 15:18, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Daniel Gustafsson <daniel@yesql.se> writes:
>> How about fixing the error message in the backbranches, with a comment why we
>> expect 3 and not 4 fields, and requiring 4 thus restricting to 9.4+ in HEAD?
>
> OK by me.
Following the bouncing ball it turns out that restricting to 9.4+ (effectively
reapplying 083d29c65b78 adapted to the callsites moved to a common function)
across the board isn't doable since pg_basebackup requires IDENTIFY_SYSTEM for
9.1->9.3 to get the starttli. Thus I opted for just fixing the error message
and left the restriction on 9.4+ as future work for when it can be done across
all callers.
--
Daniel Gustafsson
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: BUG #18224: message bug in libpqwalreceiver.c.
@ 2023-12-10 04:00 Alexander Lakhin <exclusion@gmail.com>
parent: Daniel Gustafsson <daniel@yesql.se>
0 siblings, 1 reply; 10+ messages in thread
From: Alexander Lakhin @ 2023-12-10 04:00 UTC (permalink / raw)
To: Daniel Gustafsson <daniel@yesql.se>; Tom Lane <tgl@sss.pgh.pa.us>; +Cc: king tomo <t.katsumata1122@gmail.com>; pgsql-bugs@lists.postgresql.org
Hello Daniel,
05.12.2023 16:44, Daniel Gustafsson wrote:
> Following the bouncing ball it turns out that restricting to 9.4+ (effectively
> reapplying 083d29c65b78 adapted to the callsites moved to a common function)
> across the board isn't doable since pg_basebackup requires IDENTIFY_SYSTEM for
> 9.1->9.3 to get the starttli. Thus I opted for just fixing the error message
> and left the restriction on 9.4+ as future work for when it can be done across
> all callers.
Isn't IDENTIFY_SERVER a mistake in the comment added?:
+ * IDENTIFY_SERVER returns 3 columns in 9.3 and earlier, and 4 columns in
+ * 9.4 and onwards.
Best regards,
Alexander
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: BUG #18224: message bug in libpqwalreceiver.c.
@ 2023-12-12 10:20 Daniel Gustafsson <daniel@yesql.se>
parent: Alexander Lakhin <exclusion@gmail.com>
0 siblings, 0 replies; 10+ messages in thread
From: Daniel Gustafsson @ 2023-12-12 10:20 UTC (permalink / raw)
To: Alexander Lakhin <exclusion@gmail.com>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; king tomo <t.katsumata1122@gmail.com>; pgsql-bugs@lists.postgresql.org
> On 10 Dec 2023, at 05:00, Alexander Lakhin <exclusion@gmail.com> wrote:
>
> Hello Daniel,
>
> 05.12.2023 16:44, Daniel Gustafsson wrote:
>> Following the bouncing ball it turns out that restricting to 9.4+ (effectively
>> reapplying 083d29c65b78 adapted to the callsites moved to a common function)
>> across the board isn't doable since pg_basebackup requires IDENTIFY_SYSTEM for
>> 9.1->9.3 to get the starttli. Thus I opted for just fixing the error message
>> and left the restriction on 9.4+ as future work for when it can be done across
>> all callers.
>
> Isn't IDENTIFY_SERVER a mistake in the comment added?:
>
> + * IDENTIFY_SERVER returns 3 columns in 9.3 and earlier, and 4 columns in
> + * 9.4 and onwards.
Ugh, yes, fixing.
--
Daniel Gustafsson
^ permalink raw reply [nested|flat] 10+ messages in thread
end of thread, other threads:[~2023-12-12 10:20 UTC | newest]
Thread overview: 10+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2023-12-04 11:24 BUG #18224: message bug in libpqwalreceiver.c. PG Bug reporting form <noreply@postgresql.org>
2023-12-04 11:28 ` king tomo <t.katsumata1122@gmail.com>
2023-12-04 12:12 ` Daniel Gustafsson <daniel@yesql.se>
2023-12-04 14:07 ` Tom Lane <tgl@sss.pgh.pa.us>
2023-12-04 14:10 ` Daniel Gustafsson <daniel@yesql.se>
2023-12-04 14:18 ` Tom Lane <tgl@sss.pgh.pa.us>
2023-12-05 11:50 ` king tomo <t.katsumata1122@gmail.com>
2023-12-05 13:44 ` Daniel Gustafsson <daniel@yesql.se>
2023-12-10 04:00 ` Alexander Lakhin <exclusion@gmail.com>
2023-12-12 10:20 ` Daniel Gustafsson <daniel@yesql.se>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox