Thread: pg_upgrade del/rmdir path fix
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
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
Attachment
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
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
Attachment
Andrew Dunstan <andrew@dunslane.net> writes:
> OK, revised patch attached.
Looks good to me.
regards, tom lane
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
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. +
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
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
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
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. +
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
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
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
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. +
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
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
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
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