Thread: pg_upgrade bugs

I have been wrestling for a couple of days trying to get pg_upgrade
testing working on Windows, with a view to having it tested on the
buildfarm. The test script has its own issues, which I'll deal with
separately, but there are two issues in pg_upgrade's exec.c that make me
suspect that if pg_upgrade has ever worked at all on Windows it is a
matter of sheer luck. The attached patch fixes these. The first issue is
a plain miscall to stlcpy(), where the length argument is wrong. The
second is where exec_prog tries to open a log file after the system call
returns. This will fail if the command was a 'pg_ctl start', as the
running postmaster will have the log file open, so I have simply
#ifdef'd it out for the Windows case, as the code does nothing except
add a couple of line feeds to the log, missing which won't affect
anything much.

Barring objection I will commit this before long.

cheers

andrew

Andrew Dunstan <andrew@dunslane.net> writes:
> I have been wrestling for a couple of days trying to get pg_upgrade 
> testing working on Windows, with a view to having it tested on the 
> buildfarm. The test script has its own issues, which I'll deal with 
> separately, but there are two issues in pg_upgrade's exec.c that make me 
> suspect that if pg_upgrade has ever worked at all on Windows it is a 
> matter of sheer luck. The attached patch fixes these. The first issue is 
> a plain miscall to stlcpy(), where the length argument is wrong. The 
> second is where exec_prog tries to open a log file after the system call 
> returns. This will fail if the command was a 'pg_ctl start', as the 
> running postmaster will have the log file open, so I have simply 
> #ifdef'd it out for the Windows case, as the code does nothing except 
> add a couple of line feeds to the log, missing which won't affect 
> anything much.

The strlcpy bug seems to be recently introduced --- I don't see it in
9.2.  I think the other bit has not been there very long either,
though it *is* in 9.2 branch so you'd better back-patch that part.
        regards, tom lane

Excerpts from Andrew Dunstan's message of dom sep 02 22:11:42 -0300 2012:
>
> I have been wrestling for a couple of days trying to get pg_upgrade
> testing working on Windows, with a view to having it tested on the
> buildfarm. The test script has its own issues, which I'll deal with
> separately, but there are two issues in pg_upgrade's exec.c that make me
> suspect that if pg_upgrade has ever worked at all on Windows it is a
> matter of sheer luck. The attached patch fixes these. The first issue is
> a plain miscall to stlcpy(), where the length argument is wrong. The
> second is where exec_prog tries to open a log file after the system call
> returns. This will fail if the command was a 'pg_ctl start', as the
> running postmaster will have the log file open, so I have simply
> #ifdef'd it out for the Windows case, as the code does nothing except
> add a couple of line feeds to the log, missing which won't affect
> anything much.

The first issue was clearly introduced by me in
088c065ce8e405fafbfa966937184ece9defcf20.  The other one is probably
also my fault.  Bruce has tested this on Windows previously and fixed
some previous bugs also introduced by me in the same area; so we know it
works.

Maybe, to reduce future backpatching pain, we could backpatch the change
to exec_prog() API now that you have fixed the implementation?

> Barring objection I will commit this before long.

Thanks.

--
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> Maybe, to reduce future backpatching pain, we could backpatch the change
> to exec_prog() API now that you have fixed the implementation?

I'm inclined to think this is a good idea, but keep in mind we're less
than four days from wrapping 9.2.0.  There's not a lot of margin for
error here.

At the same time, getting pg_upgrade to pass regression on Windows would
be a good thing, and ought to go a long way towards ameliorating worries
about this.
        regards, tom lane

On 09/03/2012 12:00 AM, Tom Lane wrote:
> Alvaro Herrera <alvherre@2ndquadrant.com> writes:
>> Maybe, to reduce future backpatching pain, we could backpatch the change
>> to exec_prog() API now that you have fixed the implementation?
> I'm inclined to think this is a good idea, but keep in mind we're less
> than four days from wrapping 9.2.0.  There's not a lot of margin for
> error here.
>
> At the same time, getting pg_upgrade to pass regression on Windows would
> be a good thing, and ought to go a long way towards ameliorating worries
> about this.

My first focus was on getting it working on HEAD, and I only got there 
fairly late last night.

I'll look at getting testing working on 9.2 today.

At this stage my plan is to make the absolute minimum changes necessary, 
but if Alvaro wants to do something a bit larger to align t code, I'll 
be happy to test it.

cheers

andrew