Horton

Thread

  1. pg_basebackup is not checking IDENTIFY_SYSTEM numbre of columns

    Jaime Casanova <jaime@2ndquadrant.com> — 2012-01-11T21:31:33Z

    Hi,

I just notice $SUBJECT and this could lead us to a segmentation fault
if by accident we point to a system with a different number of columns
in IDENTIFY_SYSTEM, at least i point pg_receivexlog from current head
to a 9.0 instalation and got that.

Any reason for not checking number of columns?

-- 
Jaime Casanova         www.2ndQuadrant.com
Professional PostgreSQL: Soporte 24x7 y capacitación

  2. Re: pg_basebackup is not checking IDENTIFY_SYSTEM numbre of columns

    Magnus Hagander <magnus@hagander.net> — 2012-01-11T22:11:46Z

    On Wed, Jan 11, 2012 at 22:31, Jaime Casanova <jaime@2ndquadrant.com> wrote:
> Hi,
>
> I just notice $SUBJECT and this could lead us to a segmentation fault
> if by accident we point to a system with a different number of columns
> in IDENTIFY_SYSTEM, at least i point pg_receivexlog from current head
> to a 9.0 instalation and got that.
>
> Any reason for not checking number of columns?

Hmm. I could've bet it did that. I must've taken that code out at some
point during refactoring.

No, no reason. Adding such a check would be a good idea.

-- 
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

  3. Re: pg_basebackup is not checking IDENTIFY_SYSTEM numbre of columns

    Jaime Casanova <jaime@2ndquadrant.com> — 2012-01-15T21:00:26Z

    On Wed, Jan 11, 2012 at 5:11 PM, Magnus Hagander <magnus@hagander.net> wrote:
>
> No, no reason. Adding such a check would be a good idea.
>

ok. patch attached, it also adds a few PQclear() calls before
disconnect_and_exit().
btw, in BaseBackup() in line 1149 (after the patch is applied) there
is an exit instead of disconnect_and_exit() and that is probably a
typo too

-- 
Jaime Casanova         www.2ndQuadrant.com
Professional PostgreSQL: Soporte 24x7 y capacitación

  4. Re: pg_basebackup is not checking IDENTIFY_SYSTEM numbre of columns

    Magnus Hagander <magnus@hagander.net> — 2012-01-20T13:00:45Z

    On Sun, Jan 15, 2012 at 22:00, Jaime Casanova <jaime@2ndquadrant.com> wrote:
> On Wed, Jan 11, 2012 at 5:11 PM, Magnus Hagander <magnus@hagander.net> wrote:
>>
>> No, no reason. Adding such a check would be a good idea.
>>
>
> ok. patch attached, it also adds a few PQclear() calls before
> disconnect_and_exit().

I don't think we need to care about all those PQclear() - it does an
exit() right after them anyway, so what's the point? (the disconnect
part is important of course, since otherwise we get a message in the
log on the server)

I've applied the patch without the PQclear()s, and changed it around
so that the error message shown is actually the same in all the
different places.

> btw, in BaseBackup() in line 1149 (after the patch is applied) there
> is an exit instead of disconnect_and_exit() and that is probably a
> typo too

It is, indeed. Fixed.

-- 
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/