Horton
Menu

Thread

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. pg_upgrade: Fix exec_prog API to be less flaky

  1. pg_upgrade bugs

    Andrew Dunstan <andrew@dunslane.net> — 2012-09-03T01:11:42Z

    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

  2. Re: pg_upgrade bugs

    Tom Lane <tgl@sss.pgh.pa.us> — 2012-09-03T01:39:41Z

    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

  3. Re: pg_upgrade bugs

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2012-09-03T03:29:02Z

    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

  4. Re: pg_upgrade bugs

    Tom Lane <tgl@sss.pgh.pa.us> — 2012-09-03T04:00:46Z

    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

  5. Re: pg_upgrade bugs

    Andrew Dunstan <andrew@dunslane.net> — 2012-09-03T13:14:20Z

    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