Re: pg_ctl -D canonicalization - Mailing list pgsql-patches
From | Bruce Momjian |
---|---|
Subject | Re: pg_ctl -D canonicalization |
Date | |
Msg-id | 200410271716.i9RHGXg22084@candle.pha.pa.us Whole thread Raw |
In response to | Re: pg_ctl -D canonicalization ("Magnus Hagander" <mha@sollentuna.net>) |
List | pgsql-patches |
OK, attached patch applied. Your original patch didn't canonicalize the PGDATA environment variable. '-D' takes precidence but it should be accurate. I also updated the comments in canonicalize_path because it does a lot more now that just win to unix path conversion. --------------------------------------------------------------------------- Magnus Hagander wrote: > > > It seems pg_ctl calls canonicalize_path() only on the path as being > > > used to access for example the pid file, and not the path > > that is sent > > > along to the postmaster. > > > Specifically, this causes failure on win32 when a path is > > passed with > > > a trailing backslash, when it's inside quotes such as: > > > pg_ctl start -D "c:\program fiels\postgresql\data\" > > > > > > The quotes are of course necessary since there are spaces in the > > > filename. In my specific case the trailing backslash is > > automatically > > > added by windows installer. but other cases when the backslash is > > > added can easily be seen... > > > > > > Attached patch makes pg_ctl call canonicalize_path() on the path as > > > passed on the commandline as well. > > > > > > I have only tested this on win32, where it appears to work fine. I > > > think it would be good on other platsforms as well which is why I > > > didn't #ifdef it. If not, then please add #ifdefs. > > > > Uh, isn't the proper fix to fix the postmaster's handling of -D paths? > > Nope. > > The problem is that in this case, pg_ctl gets the path: > c:\program files\postgresql\data\" > > (not unbalanced quotes). Yes, win32 commandline quoting is horrible. > Anyway. then it properly quotes this for the commandline: > "c:\program files\postgresql\data\"" > > and you can see where that breaks down... > > Another option would be simple check to simply strip the " (and the > trailing backslash too, probably, to make things safer at the next > step). I just thought it'd be cleaner to use canonicalize_path(). But > it's just the trailing quote stuff that causes the problem. (See the > second step in canonicalize_path() where this has been noticed befoer, > followed by the trim-trailing part which would also be nice). > > (Everything works if you register it as a service because then it uses > pg_data and not pgdata_opt. This is just when you start things manually, > which is why we missed it before...) > > //Magnus > > > ---------------------------(end of broadcast)--------------------------- > TIP 6: Have you searched our list archives? > > http://archives.postgresql.org > -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square, Pennsylvania 19073 Index: src/bin/pg_ctl/pg_ctl.c =================================================================== RCS file: /cvsroot/pgsql/src/bin/pg_ctl/pg_ctl.c,v retrieving revision 1.42 diff -c -c -r1.42 pg_ctl.c *** src/bin/pg_ctl/pg_ctl.c 22 Oct 2004 00:24:18 -0000 1.42 --- src/bin/pg_ctl/pg_ctl.c 27 Oct 2004 17:10:58 -0000 *************** *** 1279,1297 **** { case 'D': { ! int len = strlen(optarg); ! char *env_var; ! env_var = xmalloc(len + 8); ! snprintf(env_var, len + 8, "PGDATA=%s", optarg); putenv(env_var); /* ! * Show -D for easier postmaster 'ps' ! * identification */ ! pgdata_opt = xmalloc(len + 7); ! snprintf(pgdata_opt, len + 7, "-D \"%s\" ", optarg); break; } case 'l': --- 1279,1301 ---- { case 'D': { ! char *pgdata_D = xmalloc(strlen(optarg)); ! char *env_var = xmalloc(strlen(optarg) + 8); ! strcpy(pgdata_D, optarg); ! canonicalize_path(pgdata_D); ! snprintf(env_var, strlen(pgdata_D) + 8, "PGDATA=%s", ! pgdata_D); putenv(env_var); /* ! * We could pass PGDATA just in an environment ! * variable but we do -D too for clearer ! * postmaster 'ps' display */ ! pgdata_opt = xmalloc(strlen(pgdata_D) + 7); ! snprintf(pgdata_opt, strlen(pgdata_D) + 7, "-D \"%s\" ", ! pgdata_D); break; } case 'l': Index: src/port/path.c =================================================================== RCS file: /cvsroot/pgsql/src/port/path.c,v retrieving revision 1.37 diff -c -c -r1.37 path.c *** src/port/path.c 24 Oct 2004 22:08:19 -0000 1.37 --- src/port/path.c 27 Oct 2004 17:11:01 -0000 *************** *** 115,121 **** /* ! * Make all paths look like Unix */ void canonicalize_path(char *path) --- 115,126 ---- /* ! * Clean up path by: ! * o make Win32 path use Unix slashes ! * o remove trailling quote on Win32 ! * o remove trailling slash ! * o remove trailing '.' ! * o process trailing '..' ourselves */ void canonicalize_path(char *path) *************** *** 145,157 **** /* * Removing the trailing slash on a path means we never get ugly ! * double slashes. Also, Win32 can't stat() a directory with a ! * trailing slash. Don't remove a leading slash, though. */ trim_trailing_separator(path); /* ! * Remove any trailing uses of "." or "..", too. */ for (;;) { --- 150,162 ---- /* * Removing the trailing slash on a path means we never get ugly ! * double trailing slashes. Also, Win32 can't stat() a directory ! * with a trailing slash. Don't remove a leading slash, though. */ trim_trailing_separator(path); /* ! * Remove any trailing uses of "." and process ".." ourselves */ for (;;) { *************** *** 165,171 **** else if (len >= 3 && strcmp(path + len - 3, "/..") == 0) { trim_directory(path); ! trim_directory(path); trim_trailing_separator(path); } else --- 170,176 ---- else if (len >= 3 && strcmp(path + len - 3, "/..") == 0) { trim_directory(path); ! trim_directory(path); /* remove directory above */ trim_trailing_separator(path); } else
pgsql-patches by date: