Horton

Thread

Commits

  1. Fix typo in comment

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

    The Post Office <noreply@postgresql.org> — 2023-12-04T11:24:36Z

    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

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

    Tomonari Katsumata <t.katsumata1122@gmail.com> — 2023-12-04T11:28:37Z

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

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

    Daniel Gustafsson <daniel@yesql.se> — 2023-12-04T12:12:27Z

    > 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

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-12-04T14:07:29Z

    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

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

    Daniel Gustafsson <daniel@yesql.se> — 2023-12-04T14:10:02Z

    > 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

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-12-04T14:18:12Z

    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

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

    Tomonari Katsumata <t.katsumata1122@gmail.com> — 2023-12-05T11:50:00Z

    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
>

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

    Daniel Gustafsson <daniel@yesql.se> — 2023-12-05T13:44:20Z

    > 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

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

    Alexander Law <exclusion@gmail.com> — 2023-12-10T04:00:00Z

    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

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

    Daniel Gustafsson <daniel@yesql.se> — 2023-12-12T10:20:31Z

    > 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