Horton
Menu

Thread

  1. pg_upgrade del/rmdir path fix

    Andrew Dunstan <andrew@dunslane.net> — 2012-09-03T18:30:18Z

    Here is a patch against 9.2 sources (it applies with offsets to HEAD 
too) to fix the problem that pg_upgrade can write paths in arguments for 
Windows builtin commands (specifically DEL and RMDIR) with the wrong 
path separator style. This should be applied all the way back to 9.0.

cheers

andrew

  2. Re: pg_upgrade del/rmdir path fix

    Andrew Dunstan <andrew@dunslane.net> — 2012-09-03T18:57:08Z

    On 09/03/2012 02:30 PM, Andrew Dunstan wrote:
> Here is a patch against 9.2 sources (it applies with offsets to HEAD 
> too) to fix the problem that pg_upgrade can write paths in arguments 
> for Windows builtin commands (specifically DEL and RMDIR) with the 
> wrong path separator style. This should be applied all the way back to 
> 9.0.
>


This time with a patch.

cheers

andrew

  3. Re: pg_upgrade del/rmdir path fix

    Tom Lane <tgl@sss.pgh.pa.us> — 2012-09-03T19:22:31Z

    Andrew Dunstan <andrew@dunslane.net> writes:
> This time with a patch.

Nitpicky gripe: "fix_path" is a mighty generic name.  How about
"fix_path_for_windows" or something like that?  I don't think I'd
mark it inline, either.

More generally, the behavior of combining two (maybe) filename segments
seems overcomplicated and unnecessary.  Why not just have it take *one*
argument and back-slashify that, without the concatenation behavior?
Then you'd have two calls instead of one at some of the call sites,
but that doesn't seem like much of a loss.  The malloc'd strings are
getting leaked anyway.  The function itself would reduce to pg_strdup
and a backslashification loop.  Also, you could turn it into a complete
no-op (not even pg_strdup) on non-Windows.

			regards, tom lane

  4. Re: pg_upgrade del/rmdir path fix

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

    On 09/03/2012 03:22 PM, Tom Lane wrote:
> Andrew Dunstan <andrew@dunslane.net> writes:
>> This time with a patch.
> Nitpicky gripe: "fix_path" is a mighty generic name.  How about
> "fix_path_for_windows" or something like that?  I don't think I'd
> mark it inline, either.
>
> More generally, the behavior of combining two (maybe) filename segments
> seems overcomplicated and unnecessary.  Why not just have it take *one*
> argument and back-slashify that, without the concatenation behavior?
> Then you'd have two calls instead of one at some of the call sites,
> but that doesn't seem like much of a loss.  The malloc'd strings are
> getting leaked anyway.  The function itself would reduce to pg_strdup
> and a backslashification loop.  Also, you could turn it into a complete
> no-op (not even pg_strdup) on non-Windows.
>
> 			


OK, revised patch attached.

cheers

andrew

  5. Re: pg_upgrade del/rmdir path fix

    Tom Lane <tgl@sss.pgh.pa.us> — 2012-09-03T20:32:03Z

    Andrew Dunstan <andrew@dunslane.net> writes:
> OK, revised patch attached.

Looks good to me.

			regards, tom lane

  6. Re: pg_upgrade del/rmdir path fix

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

    On 09/03/2012 04:32 PM, Tom Lane wrote:
> Andrew Dunstan <andrew@dunslane.net> writes:
>> OK, revised patch attached.
> Looks good to me.
>
> 			


OK. I will try to get the test script wrapped up tonight or tomorrow, 
that's the only thing left.

cheers

andrew

  7. Re: pg_upgrade del/rmdir path fix

    Bruce Momjian <bruce@momjian.us> — 2012-09-04T02:41:36Z

    On Mon, Sep  3, 2012 at 02:30:18PM -0400, Andrew Dunstan wrote:
> Here is a patch against 9.2 sources (it applies with offsets to HEAD
> too) to fix the problem that pg_upgrade can write paths in arguments
> for Windows builtin commands (specifically DEL and RMDIR) with the
> wrong path separator style. This should be applied all the way back
> to 9.0.

Wow, my guess is that no one ever ran those auto-generated scripts on
Windows.  Thanks.

-- 
  Bruce Momjian  <bruce@momjian.us>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

  + It's impossible for everything to be true. +

  8. Re: pg_upgrade del/rmdir path fix

    Andrew Dunstan <andrew@dunslane.net> — 2012-09-04T03:05:40Z

    On 09/03/2012 10:41 PM, Bruce Momjian wrote:
> On Mon, Sep  3, 2012 at 02:30:18PM -0400, Andrew Dunstan wrote:
>> Here is a patch against 9.2 sources (it applies with offsets to HEAD
>> too) to fix the problem that pg_upgrade can write paths in arguments
>> for Windows builtin commands (specifically DEL and RMDIR) with the
>> wrong path separator style. This should be applied all the way back
>> to 9.0.
> Wow, my guess is that no one ever ran those auto-generated scripts on
> Windows.  Thanks.
>


Lucky for us the test script does or I'd never have noticed either.

cheers

andrew

  9. Re: pg_upgrade del/rmdir path fix

    Andrew Dunstan <andrew@dunslane.net> — 2012-09-04T04:16:39Z

    On 09/03/2012 11:05 PM, Andrew Dunstan wrote:
>
> On 09/03/2012 10:41 PM, Bruce Momjian wrote:
>> On Mon, Sep  3, 2012 at 02:30:18PM -0400, Andrew Dunstan wrote:
>>> Here is a patch against 9.2 sources (it applies with offsets to HEAD
>>> too) to fix the problem that pg_upgrade can write paths in arguments
>>> for Windows builtin commands (specifically DEL and RMDIR) with the
>>> wrong path separator style. This should be applied all the way back
>>> to 9.0.
>> Wow, my guess is that no one ever ran those auto-generated scripts on
>> Windows.  Thanks.
>>
>
>
> Lucky for us the test script does or I'd never have noticed either.


And here's the first Windows buildfarm check of pg_upgrade. 
<http://www.pgbuildfarm.org/cgi-bin/show_stage_log.pl?nm=pitta&dt=2012-09-04%2003%3A00%3A05&stg=check-pg_upgrade>

cheers

andrew

  10. Re: pg_upgrade del/rmdir path fix

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2012-09-04T14:42:58Z

    Excerpts from Andrew Dunstan's message of mar sep 04 01:16:39 -0300 2012:

> And here's the first Windows buildfarm check of pg_upgrade. 
> <http://www.pgbuildfarm.org/cgi-bin/show_stage_log.pl?nm=pitta&dt=2012-09-04%2003%3A00%3A05&stg=check-pg_upgrade>

Great, thanks.

Who's going to work now on porting the shell script to Perl? ;-)

Somehow the verbose reporting of user relation files being copied does
not seem exceedingly useful; and I don't remember seeing that on Linux.

Should this be tweaked to avoid outputting the status message?

c:\mingw\msys\1.0\home\pgrunner\bf\root\HEAD\pgsql.7020\contrib\pg_upgrade>echo
ECHO is on.

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

  11. Re: pg_upgrade del/rmdir path fix

    Bruce Momjian <bruce@momjian.us> — 2012-09-04T14:49:36Z

    On Tue, Sep  4, 2012 at 11:42:58AM -0300, Alvaro Herrera wrote:
> Excerpts from Andrew Dunstan's message of mar sep 04 01:16:39 -0300 2012:
> 
> > And here's the first Windows buildfarm check of pg_upgrade. 
> > <http://www.pgbuildfarm.org/cgi-bin/show_stage_log.pl?nm=pitta&dt=2012-09-04%2003%3A00%3A05&stg=check-pg_upgrade>
> 
> Great, thanks.
> 
> Who's going to work now on porting the shell script to Perl? ;-)

Well, we require Perl for development, but not for usage, at least not
yet.  There was talk of needing Perl for doing standby pg_upgrade, but
there were too many concerns about that idea.

> Somehow the verbose reporting of user relation files being copied does
> not seem exceedingly useful; and I don't remember seeing that on Linux.
> 
> Should this be tweaked to avoid outputting the status message?
> 
> c:\mingw\msys\1.0\home\pgrunner\bf\root\HEAD\pgsql.7020\contrib\pg_upgrade>echo
> ECHO is on.

Probably.

-- 
  Bruce Momjian  <bruce@momjian.us>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

  + It's impossible for everything to be true. +

  12. Re: pg_upgrade del/rmdir path fix

    Tom Lane <tgl@sss.pgh.pa.us> — 2012-09-04T15:06:50Z

    Bruce Momjian <bruce@momjian.us> writes:
> On Tue, Sep  4, 2012 at 11:42:58AM -0300, Alvaro Herrera wrote:
>> Who's going to work now on porting the shell script to Perl? ;-)

> Well, we require Perl for development, but not for usage, at least not
> yet.

This is a regression-test script, so that complaint doesn't seem to me
to have a lot of force ... especially not when set against the fact that
the shell script is useless on non-mingw Windows.

			regards, tom lane

  13. Re: pg_upgrade del/rmdir path fix

    Andrew Dunstan <andrew@dunslane.net> — 2012-09-04T15:10:50Z

    On 09/04/2012 10:42 AM, Alvaro Herrera wrote:
> Excerpts from Andrew Dunstan's message of mar sep 04 01:16:39 -0300 2012:
>
>> And here's the first Windows buildfarm check of pg_upgrade.
>> <http://www.pgbuildfarm.org/cgi-bin/show_stage_log.pl?nm=pitta&dt=2012-09-04%2003%3A00%3A05&stg=check-pg_upgrade>
> Great, thanks.
>
> Who's going to work now on porting the shell script to Perl? ;-)

Probably me, one day ...

>
> Somehow the verbose reporting of user relation files being copied does
> not seem exceedingly useful; and I don't remember seeing that on Linux.

Yes, it's a pain. Not sure what causes it.

>
> Should this be tweaked to avoid outputting the status message?
>
> c:\mingw\msys\1.0\home\pgrunner\bf\root\HEAD\pgsql.7020\contrib\pg_upgrade>echo
> ECHO is on.
>


Already fixed.


cheers

andrew

  14. Re: pg_upgrade del/rmdir path fix

    Andrew Dunstan <andrew@dunslane.net> — 2012-09-04T15:12:52Z

    On 09/04/2012 10:49 AM, Bruce Momjian wrote:
> On Tue, Sep  4, 2012 at 11:42:58AM -0300, Alvaro Herrera wrote:
>> Excerpts from Andrew Dunstan's message of mar sep 04 01:16:39 -0300 2012:
>>
>>> And here's the first Windows buildfarm check of pg_upgrade.
>>> <http://www.pgbuildfarm.org/cgi-bin/show_stage_log.pl?nm=pitta&dt=2012-09-04%2003%3A00%3A05&stg=check-pg_upgrade>
>> Great, thanks.
>>
>> Who's going to work now on porting the shell script to Perl? ;-)
> Well, we require Perl for development, but not for usage, at least not
> yet.  There was talk of needing Perl for doing standby pg_upgrade, but
> there were too many concerns about that idea.


This is a test script, not what you should use in production. I don't 
see any reason why we shouldn't require Perl for running the standard test.


cheers

andrew

  15. Re: pg_upgrade del/rmdir path fix

    Bruce Momjian <bruce@momjian.us> — 2012-09-04T15:13:26Z

    On Tue, Sep  4, 2012 at 11:12:52AM -0400, Andrew Dunstan wrote:
> 
> On 09/04/2012 10:49 AM, Bruce Momjian wrote:
> >On Tue, Sep  4, 2012 at 11:42:58AM -0300, Alvaro Herrera wrote:
> >>Excerpts from Andrew Dunstan's message of mar sep 04 01:16:39 -0300 2012:
> >>
> >>>And here's the first Windows buildfarm check of pg_upgrade.
> >>><http://www.pgbuildfarm.org/cgi-bin/show_stage_log.pl?nm=pitta&dt=2012-09-04%2003%3A00%3A05&stg=check-pg_upgrade>
> >>Great, thanks.
> >>
> >>Who's going to work now on porting the shell script to Perl? ;-)
> >Well, we require Perl for development, but not for usage, at least not
> >yet.  There was talk of needing Perl for doing standby pg_upgrade, but
> >there were too many concerns about that idea.
> 
> 
> This is a test script, not what you should use in production. I
> don't see any reason why we shouldn't require Perl for running the
> standard test.

Oh, I thought he was talking about the scripts pg_upgrade creates for
users to run.  Sorry.

-- 
  Bruce Momjian  <bruce@momjian.us>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

  + It's impossible for everything to be true. +

  16. Re: pg_upgrade del/rmdir path fix

    Tom Lane <tgl@sss.pgh.pa.us> — 2012-09-04T15:21:53Z

    Andrew Dunstan <andrew@dunslane.net> writes:
> This is a test script, not what you should use in production. I don't 
> see any reason why we shouldn't require Perl for running the standard test.

But on the third hand ... we've taken pains to ensure that you don't
*have* to have Perl to build from a tarball, and I think it is not
unreasonable that "build" should include being able to do "make check".

Maybe we have to carry both this shell script and a Perl equivalent
for Windows.

			regards, tom lane

  17. Re: pg_upgrade del/rmdir path fix

    Andrew Dunstan <andrew@dunslane.net> — 2012-09-04T16:22:13Z

    On 09/04/2012 11:21 AM, Tom Lane wrote:
> Andrew Dunstan <andrew@dunslane.net> writes:
>> This is a test script, not what you should use in production. I don't
>> see any reason why we shouldn't require Perl for running the standard test.
> But on the third hand ... we've taken pains to ensure that you don't
> *have* to have Perl to build from a tarball, and I think it is not
> unreasonable that "build" should include being able to do "make check".
>
> Maybe we have to carry both this shell script and a Perl equivalent
> for Windows.


Yeah. I think it will just be another target in vcregress.pl

cheers

andrew

  18. Re: pg_upgrade del/rmdir path fix

    Andrew Dunstan <andrew@dunslane.net> — 2012-09-04T18:25:27Z

    On 09/04/2012 10:42 AM, Alvaro Herrera wrote:

>
> Somehow the verbose reporting of user relation files being copied does
> not seem exceedingly useful; and I don't remember seeing that on Linux.
>


Yeah, and it does something odd anyway when it's not writing to a 
terminal. Can we get rid of it, or make it only work in verbose mode?

cheers

andrew

  19. Re: pg_upgrade del/rmdir path fix

    Andrew Dunstan <andrew@dunslane.net> — 2012-09-05T00:54:22Z

    On 09/04/2012 02:25 PM, Andrew Dunstan wrote:
>
> On 09/04/2012 10:42 AM, Alvaro Herrera wrote:
>
>>
>> Somehow the verbose reporting of user relation files being copied does
>> not seem exceedingly useful; and I don't remember seeing that on Linux.
>>
>
>
> Yeah, and it does something odd anyway when it's not writing to a 
> terminal. Can we get rid of it, or make it only work in verbose mode?
>
>

The attached is an attempt to fix this. I think it handles most of 
what's wrong with this. (The patch is bigger because the code currently 
uses a variable called "fileno" - not a good idea.)

cheers

andrew