Thread: pg_receivexlog: spurious error message connecting to 9.3
Hi, I was testing the compatibility of pg_receivexlog with the previous PostgreSQL versions and I've discovered that in 9.5 and9.6, although being compatible with 9.3, it prints an ugly but harmless error message. $ 9.5/bin/pg_receivexlog -D /tmp/testx -v -p 5493 *pg_receivexlog: could not identify system: got 1 rows and 3 fields, expected 1 rows and 4 or more fields* *column number 3 is out of range 0..2* pg_receivexlog: starting log streaming at 0/4000000 (timeline 1) After the error, the streaming starts and continue without issue, as it was printed by the code that checks if the connectionis not database specific, and this check is not needed on 9.3. I've attached a little patch that removes the errors when connected to 9.3. Regards, Marco -- Marco Nenciarini - 2ndQuadrant Italy PostgreSQL Training, Services and Support marco.nenciarini@2ndQuadrant.it | www.2ndQuadrant.it
Attachment
On 10 November 2015 at 01:47, Marco Nenciarini <marco.nenciarini@2ndquadrant.it> wrote: > I've attached a little patch that removes the errors when connected to 9.3. Looks good to me. No point confusing users. The other callers of RunIdentifySystem are pg_basebackup and pg_receivelogical. pg_basebackup doesn't ask for the db_name (passes null). pg_receivelogical handles it being null already (and if it didn't, it'd die with or without this patch). pg_receivexlog expects it to be null and fails gracefully if it isn't. So this change just removes some pointless noise. -- Craig Ringer http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
On 10/11/15 07:35, Craig Ringer wrote: > On 10 November 2015 at 01:47, Marco Nenciarini > <marco.nenciarini@2ndquadrant.it> wrote: > >> I've attached a little patch that removes the errors when connected to 9.3. > > Looks good to me. No point confusing users. > > The other callers of RunIdentifySystem are pg_basebackup and > pg_receivelogical. > > pg_basebackup doesn't ask for the db_name (passes null). > > pg_receivelogical handles it being null already (and if it didn't, > it'd die with or without this patch). > > pg_receivexlog expects it to be null and fails gracefully if it isn't. > > So this change just removes some pointless noise. > I've added it to the next commit fest to keep track of it. I've also set Craig as reviewer, as he has already sent a reviewfor it (and it's a really simple patch). Thanks, Marco -- Marco Nenciarini - 2ndQuadrant Italy PostgreSQL Training, Services and Support marco.nenciarini@2ndQuadrant.it | www.2ndQuadrant.it
On Tue, Nov 10, 2015 at 1:35 AM, Craig Ringer <craig@2ndquadrant.com> wrote: > On 10 November 2015 at 01:47, Marco Nenciarini > <marco.nenciarini@2ndquadrant.it> wrote: > >> I've attached a little patch that removes the errors when connected to 9.3. > > Looks good to me. No point confusing users. > > The other callers of RunIdentifySystem are pg_basebackup and > pg_receivelogical. > > pg_basebackup doesn't ask for the db_name (passes null). > > pg_receivelogical handles it being null already (and if it didn't, > it'd die with or without this patch). > > pg_receivexlog expects it to be null and fails gracefully if it isn't. > > So this change just removes some pointless noise. The fprintf(stderr, ...) does not cause a non-local exit, so the "else" just after it should be deleted. Otherwise, when that branch is taken, *db_name doesn't get initialized at all. Actually, I'd suggest doing it like the attached instead, which seems a bit tighter. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Attachment
Hi Robert, On 17/11/15 20:10, Robert Haas wrote: > On Tue, Nov 10, 2015 at 1:35 AM, Craig Ringer <craig@2ndquadrant.com> wrote: >> On 10 November 2015 at 01:47, Marco Nenciarini >> <marco.nenciarini@2ndquadrant.it> wrote: >> >>> I've attached a little patch that removes the errors when connected to 9.3. >> >> Looks good to me. No point confusing users. >> >> The other callers of RunIdentifySystem are pg_basebackup and >> pg_receivelogical. >> >> pg_basebackup doesn't ask for the db_name (passes null). >> >> pg_receivelogical handles it being null already (and if it didn't, >> it'd die with or without this patch). >> >> pg_receivexlog expects it to be null and fails gracefully if it isn't. >> >> So this change just removes some pointless noise. > > The fprintf(stderr, ...) does not cause a non-local exit, so the > "else" just after it should be deleted. Otherwise, when that branch > is taken, *db_name doesn't get initialized at all. > > Actually, I'd suggest doing it like the attached instead, which seems > a bit tighter. > I agree, your patch is better. Regards, Marco -- Marco Nenciarini - 2ndQuadrant Italy PostgreSQL Training, Services and Support marco.nenciarini@2ndQuadrant.it | www.2ndQuadrant.it
On Tue, Nov 24, 2015 at 7:00 PM, Marco Nenciarini
<marco.nenciarini@2ndquadrant.it> wrote:
> Hi Robert,
>
> On 17/11/15 20:10, Robert Haas wrote:
>> On Tue, Nov 10, 2015 at 1:35 AM, Craig Ringer <craig@2ndquadrant.com> wrote:
>>> On 10 November 2015 at 01:47, Marco Nenciarini
>>> <marco.nenciarini@2ndquadrant.it> wrote:
>>>
>>>> I've attached a little patch that removes the errors when connected to 9.3.
>>>
>>> Looks good to me. No point confusing users.
>>>
>>> The other callers of RunIdentifySystem are pg_basebackup and
>>> pg_receivelogical.
>>>
>>> pg_basebackup doesn't ask for the db_name (passes null).
>>>
>>> pg_receivelogical handles it being null already (and if it didn't,
>>> it'd die with or without this patch).
>>>
>>> pg_receivexlog expects it to be null and fails gracefully if it isn't.
>>>
>>> So this change just removes some pointless noise.
>>
>> The fprintf(stderr, ...) does not cause a non-local exit, so the
>> "else" just after it should be deleted. Otherwise, when that branch
>> is taken, *db_name doesn't get initialized at all.
>>
>> Actually, I'd suggest doing it like the attached instead, which seems
>> a bit tighter.
>>
>
> I agree, your patch is better.
+ else if (PQserverVersion(conn) >= 90400) fprintf(stderr, _("%s: could not
identifysystem: got %d rows and
%d fields, expected %d rows and %d or more fields\n"), progname, PQntuples(res), PQnfields(res), 1,
4); }
In the above case, PQclear(res) should be called and FALSE should be returned?
Regards,
--
Fujii Masao
On Tue, Nov 24, 2015 at 8:32 AM, Fujii Masao <masao.fujii@gmail.com> wrote:
> On Tue, Nov 24, 2015 at 7:00 PM, Marco Nenciarini
> <marco.nenciarini@2ndquadrant.it> wrote:
>> Hi Robert,
>>
>> On 17/11/15 20:10, Robert Haas wrote:
>>> On Tue, Nov 10, 2015 at 1:35 AM, Craig Ringer <craig@2ndquadrant.com> wrote:
>>>> On 10 November 2015 at 01:47, Marco Nenciarini
>>>> <marco.nenciarini@2ndquadrant.it> wrote:
>>>>
>>>>> I've attached a little patch that removes the errors when connected to 9.3.
>>>>
>>>> Looks good to me. No point confusing users.
>>>>
>>>> The other callers of RunIdentifySystem are pg_basebackup and
>>>> pg_receivelogical.
>>>>
>>>> pg_basebackup doesn't ask for the db_name (passes null).
>>>>
>>>> pg_receivelogical handles it being null already (and if it didn't,
>>>> it'd die with or without this patch).
>>>>
>>>> pg_receivexlog expects it to be null and fails gracefully if it isn't.
>>>>
>>>> So this change just removes some pointless noise.
>>>
>>> The fprintf(stderr, ...) does not cause a non-local exit, so the
>>> "else" just after it should be deleted. Otherwise, when that branch
>>> is taken, *db_name doesn't get initialized at all.
>>>
>>> Actually, I'd suggest doing it like the attached instead, which seems
>>> a bit tighter.
>>>
>>
>> I agree, your patch is better.
>
> + else if (PQserverVersion(conn) >= 90400)
> fprintf(stderr,
> _("%s: could not identify system: got %d rows and
> %d fields, expected %d rows and %d or more fields\n"),
> progname, PQntuples(res), PQnfields(res), 1, 4);
> }
>
> In the above case, PQclear(res) should be called and FALSE should be returned?
Hmm, yeah, it looks like that would be more consistent with what the
other parts of this function do.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On Wed, Nov 25, 2015 at 11:34 AM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Tue, Nov 24, 2015 at 8:32 AM, Fujii Masao <masao.fujii@gmail.com> wrote:
>> On Tue, Nov 24, 2015 at 7:00 PM, Marco Nenciarini
>> <marco.nenciarini@2ndquadrant.it> wrote:
>>> Hi Robert,
>>>
>>> On 17/11/15 20:10, Robert Haas wrote:
>>>> On Tue, Nov 10, 2015 at 1:35 AM, Craig Ringer <craig@2ndquadrant.com> wrote:
>>>>> On 10 November 2015 at 01:47, Marco Nenciarini
>>>>> <marco.nenciarini@2ndquadrant.it> wrote:
>>>>>
>>>>>> I've attached a little patch that removes the errors when connected to 9.3.
>>>>>
>>>>> Looks good to me. No point confusing users.
>>>>>
>>>>> The other callers of RunIdentifySystem are pg_basebackup and
>>>>> pg_receivelogical.
>>>>>
>>>>> pg_basebackup doesn't ask for the db_name (passes null).
>>>>>
>>>>> pg_receivelogical handles it being null already (and if it didn't,
>>>>> it'd die with or without this patch).
>>>>>
>>>>> pg_receivexlog expects it to be null and fails gracefully if it isn't.
>>>>>
>>>>> So this change just removes some pointless noise.
>>>>
>>>> The fprintf(stderr, ...) does not cause a non-local exit, so the
>>>> "else" just after it should be deleted. Otherwise, when that branch
>>>> is taken, *db_name doesn't get initialized at all.
>>>>
>>>> Actually, I'd suggest doing it like the attached instead, which seems
>>>> a bit tighter.
>>>>
>>>
>>> I agree, your patch is better.
>>
>> + else if (PQserverVersion(conn) >= 90400)
>> fprintf(stderr,
>> _("%s: could not identify system: got %d rows and
>> %d fields, expected %d rows and %d or more fields\n"),
>> progname, PQntuples(res), PQnfields(res), 1, 4);
>> }
>>
>> In the above case, PQclear(res) should be called and FALSE should be returned?
>
> Hmm, yeah, it looks like that would be more consistent with what the
> other parts of this function do.
The latest patch has another problem; pg_receivexlog trying to connect to
the PostgreSQL >= 9.4 always reports the following message unexpectedly.
could not identify system: got 1 rows and 4 fields, expected 1 rows
and 4 or more fields
This problem happens because the patch incorrectly treats the case where
IDENTIFY_SYSTEM command returns NULL as database name, as an error case.
Attached is the updated version of the patch, which fixes the problem.
Comments?
Regards,
--
Fujii Masao
Attachment
<div dir="ltr"><br /><div class="gmail_extra"><br /><div class="gmail_quote">On Mon, Dec 7, 2015 at 11:37 PM, Fujii Masao<span dir="ltr"><<a href="mailto:masao.fujii@gmail.com" target="_blank">masao.fujii@gmail.com</a>></span> wrote:<br/><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Thelatest patch has another problem; pg_receivexlog trying to connect to<br /> the PostgreSQL>= 9.4 always reports the following message unexpectedly.<br /><br /> could not identify system: got 1 rowsand 4 fields, expected 1 rows<br /><span class="">and 4 or more fields<br /><br /></span>This problem happens becausethe patch incorrectly treats the case where<br /> IDENTIFY_SYSTEM command returns NULL as database name, as an errorcase.<br /><br /> Attached is the updated version of the patch, which fixes the problem.<br /> Comments?<br /></blockquote></div><br/>The patch looks good. The top comment of RunIdentifySystem is incorrect though. It should mentionthat a database name is returned and not a plugin name.<br /></div><div class="gmail_extra">(not sure what happenedwith the last message, gmail went crazy for a second removing the subject).<br /></div><div class="gmail_extra">--<br /><div class="gmail_signature">Michael<br /></div></div></div>
Michael Paquier wrote: > On Mon, Dec 7, 2015 at 11:37 PM, Fujii Masao <masao.fujii@gmail.com> wrote: > > > The latest patch has another problem; pg_receivexlog trying to connect to > > the PostgreSQL >= 9.4 always reports the following message unexpectedly. > > > > could not identify system: got 1 rows and 4 fields, expected 1 rows > > and 4 or more fields > > > > This problem happens because the patch incorrectly treats the case where > > IDENTIFY_SYSTEM command returns NULL as database name, as an error case. > > > > Attached is the updated version of the patch, which fixes the problem. > > Comments? > > The patch looks good. The top comment of RunIdentifySystem is incorrect > though. It should mention that a database name is returned and not a plugin > name. Right. Pushed with that change to 9.5 and master. Thanks! -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services