Thread: Re: BUG #18224: message bug in libpqwalreceiver.c.

Re: BUG #18224: message bug in libpqwalreceiver.c.

From
king tomo
Date:
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-SYSTEM

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

Attachment

Re: BUG #18224: message bug in libpqwalreceiver.c.

From
Daniel Gustafsson
Date:
> 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


Attachment

Re: BUG #18224: message bug in libpqwalreceiver.c.

From
Tom Lane
Date:
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



Re: BUG #18224: message bug in libpqwalreceiver.c.

From
Daniel Gustafsson
Date:
> 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




Re: BUG #18224: message bug in libpqwalreceiver.c.

From
Tom Lane
Date:
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



Re: BUG #18224: message bug in libpqwalreceiver.c.

From
king tomo
Date:
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

Re: BUG #18224: message bug in libpqwalreceiver.c.

From
Daniel Gustafsson
Date:
> 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




Re: BUG #18224: message bug in libpqwalreceiver.c.

From
Alexander Lakhin
Date:
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



Re: BUG #18224: message bug in libpqwalreceiver.c.

From
Daniel Gustafsson
Date:
> 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