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

> 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

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

> 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

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

> 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

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

> 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