Thread: pgsql-server: Use canonicalize_path for -D, GUC paths, and paths coming
pgsql-server: Use canonicalize_path for -D, GUC paths, and paths coming
From
momjian@svr1.postgresql.org (Bruce Momjian)
Date:
Log Message:
-----------
Use canonicalize_path for -D, GUC paths, and paths coming in from
environment variables.
Modified Files:
--------------
pgsql-server/src/backend/postmaster:
postmaster.c (r1.407 -> r1.408)
(http://developer.postgresql.org/cvsweb.cgi/pgsql-server/src/backend/postmaster/postmaster.c.diff?r1=1.407&r2=1.408)
pgsql-server/src/backend/utils/misc:
guc.c (r1.214 -> r1.215)
(http://developer.postgresql.org/cvsweb.cgi/pgsql-server/src/backend/utils/misc/guc.c.diff?r1=1.214&r2=1.215)
pgsql-server/src/bin/psql:
command.c (r1.119 -> r1.120)
(http://developer.postgresql.org/cvsweb.cgi/pgsql-server/src/bin/psql/command.c.diff?r1=1.119&r2=1.120)
copy.c (r1.49 -> r1.50)
(http://developer.postgresql.org/cvsweb.cgi/pgsql-server/src/bin/psql/copy.c.diff?r1=1.49&r2=1.50)
pgsql-server/src/port:
path.c (r1.22 -> r1.23)
(http://developer.postgresql.org/cvsweb.cgi/pgsql-server/src/port/path.c.diff?r1=1.22&r2=1.23)
momjian@svr1.postgresql.org (Bruce Momjian) writes: > pgsql-server/src/backend/postmaster: > postmaster.c (r1.407 -> r1.408) > (http://developer.postgresql.org/cvsweb.cgi/pgsql-server/src/backend/postmaster/postmaster.c.diff?r1=1.407&r2=1.408) You can't do that. In the first place it will dump core if PGDATA isn't set, and in the second place it is not kosher to scribble on environment values. This is the wrong place to do it anyway. It is necessary, sufficient, and already done to do it in SetDataDir. > pgsql-server/src/backend/utils/misc: > guc.c (r1.214 -> r1.215) > (http://developer.postgresql.org/cvsweb.cgi/pgsql-server/src/backend/utils/misc/guc.c.diff?r1=1.214&r2=1.215) Could we not have FATAL here, please? > pgsql-server/src/bin/psql: > command.c (r1.119 -> r1.120) > (http://developer.postgresql.org/cvsweb.cgi/pgsql-server/src/bin/psql/command.c.diff?r1=1.119&r2=1.120) None of these are correct. canonicalize_path is only intended for directory names not file names. (I think the same problem applies to several of your GUC variable changes, too.) > copy.c (r1.49 -> r1.50) > (http://developer.postgresql.org/cvsweb.cgi/pgsql-server/src/bin/psql/copy.c.diff?r1=1.49&r2=1.50) As above. regards, tom lane
Tom Lane wrote: > momjian@svr1.postgresql.org (Bruce Momjian) writes: > > pgsql-server/src/backend/postmaster: > > postmaster.c (r1.407 -> r1.408) > > (http://developer.postgresql.org/cvsweb.cgi/pgsql-server/src/backend/postmaster/postmaster.c.diff?r1=1.407&r2=1.408) > > You can't do that. In the first place it will dump core if PGDATA isn't > set, and in the second place it is not kosher to scribble on environment > values. OK, I added a strdup to make a copy of the environment variable. I also moved the canonicialize_path down so it will process both PGDATA and -D. Patch attached and applied. > This is the wrong place to do it anyway. It is necessary, sufficient, > and already done to do it in SetDataDir. The problem is that checkDataDir() (called before SetDataDir()) does a stat() on that value, and this is the failure Magnus was seeing. This requirement is new because onlyConfigSpecified() does a stat() to test to see if we are are pointing to a config file/dir or the data directory before calling SetDataDir(). However, we can't remove the canonicalize_path() from SetDataDir() because postgres and bootstrap call that directly. > > pgsql-server/src/backend/utils/misc: > > guc.c (r1.214 -> r1.215) > > (http://developer.postgresql.org/cvsweb.cgi/pgsql-server/src/backend/utils/misc/guc.c.diff?r1=1.214&r2=1.215) > > Could we not have FATAL here, please? OK, changed to ERROR, but there are alot of similar calls which used FATAL for that call in that file. > > pgsql-server/src/bin/psql: > > command.c (r1.119 -> r1.120) > > (http://developer.postgresql.org/cvsweb.cgi/pgsql-server/src/bin/psql/command.c.diff?r1=1.119&r2=1.120) > > None of these are correct. canonicalize_path is only intended for > directory names not file names. (I think the same problem applies > to several of your GUC variable changes, too.) canonicalize_path changes \ to /, and trims the trailing slash. Filenames do not need trimming, but they might need the slash change. I think we found that a filename with mixed / and \ will not work. > > copy.c (r1.49 -> r1.50) > > (http://developer.postgresql.org/cvsweb.cgi/pgsql-server/src/bin/psql/copy.c.diff?r1=1.49&r2=1.50) > > As above. > > regards, tom lane > -- 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/backend/postmaster/postmaster.c =================================================================== RCS file: /cvsroot/pgsql-server/src/backend/postmaster/postmaster.c,v retrieving revision 1.408 diff -c -c -r1.408 postmaster.c *** src/backend/postmaster/postmaster.c 11 Jul 2004 21:33:59 -0000 1.408 --- src/backend/postmaster/postmaster.c 11 Jul 2004 23:46:38 -0000 *************** *** 372,378 **** InitializeGUCOptions(); userPGDATA = getenv("PGDATA"); /* default value */ - canonicalize_path(userPGDATA); opterr = 1; --- 372,377 ---- *************** *** 524,529 **** --- 523,534 ---- write_stderr("Try \"%s --help\" for more information.\n", progname); ExitPostmaster(1); + } + + if (userPGDATA) + { + userPGDATA = strdup(userPGDATA); + canonicalize_path(userPGDATA); } if (onlyConfigSpecified(userPGDATA)) Index: src/backend/utils/init/miscinit.c =================================================================== RCS file: /cvsroot/pgsql-server/src/backend/utils/init/miscinit.c,v retrieving revision 1.127 diff -c -c -r1.127 miscinit.c *** src/backend/utils/init/miscinit.c 18 Jun 2004 06:13:54 -0000 1.127 --- src/backend/utils/init/miscinit.c 11 Jul 2004 23:46:39 -0000 *************** *** 207,216 **** errmsg("out of memory"))); } - /* - * Strip any trailing slash. Not strictly necessary, but avoids - * generating funny-looking paths to individual files. - */ canonicalize_path(new); if (DataDir) --- 207,212 ---- Index: src/backend/utils/misc/guc.c =================================================================== RCS file: /cvsroot/pgsql-server/src/backend/utils/misc/guc.c,v retrieving revision 1.216 diff -c -c -r1.216 guc.c *** src/backend/utils/misc/guc.c 11 Jul 2004 21:48:25 -0000 1.216 --- src/backend/utils/misc/guc.c 11 Jul 2004 23:46:43 -0000 *************** *** 5441,5447 **** if (doit) { /* We have to create a new pointer to force the change */ ! char *canon_val = guc_strdup(FATAL, newval); canonicalize_path(canon_val); return canon_val; } --- 5441,5447 ---- if (doit) { /* We have to create a new pointer to force the change */ ! char *canon_val = guc_strdup(ERROR, newval); canonicalize_path(canon_val); return canon_val; }
Bruce Momjian <pgman@candle.pha.pa.us> writes:
>> None of these are correct. canonicalize_path is only intended for
>> directory names not file names. (I think the same problem applies
>> to several of your GUC variable changes, too.)
> canonicalize_path changes \ to /, and trims the trailing slash.
... and probably breaks the GUC variables that represent search paths,
rather than single file/directory names. Certainly canonicalizing those
values in toto will not have the desired effects; you'd need to
canonicalize the path elements after they are extracted.
regards, tom lane
Tom Lane wrote: > Bruce Momjian <pgman@candle.pha.pa.us> writes: > >> None of these are correct. canonicalize_path is only intended for > >> directory names not file names. (I think the same problem applies > >> to several of your GUC variable changes, too.) > > > canonicalize_path changes \ to /, and trims the trailing slash. > > ... and probably breaks the GUC variables that represent search paths, > rather than single file/directory names. Certainly canonicalizing those > values in toto will not have the desired effects; you'd need to > canonicalize the path elements after they are extracted. OK, the only GUC I saw that was a list was preload_libraries, and this patch fixes that. -- 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/backend/utils/init/miscinit.c =================================================================== RCS file: /cvsroot/pgsql-server/src/backend/utils/init/miscinit.c,v retrieving revision 1.128 diff -c -c -r1.128 miscinit.c *** src/backend/utils/init/miscinit.c 11 Jul 2004 23:49:48 -0000 1.128 --- src/backend/utils/init/miscinit.c 12 Jul 2004 00:08:12 -0000 *************** *** 926,931 **** --- 926,932 ---- funcname = NULL; } + canonicalize_path(filename); initfunc = (func_ptr) load_external_function(filename, funcname, true, NULL); if (initfunc) Index: src/backend/utils/misc/guc.c =================================================================== RCS file: /cvsroot/pgsql-server/src/backend/utils/misc/guc.c,v retrieving revision 1.217 diff -c -c -r1.217 guc.c *** src/backend/utils/misc/guc.c 11 Jul 2004 23:49:51 -0000 1.217 --- src/backend/utils/misc/guc.c 12 Jul 2004 00:08:19 -0000 *************** *** 1557,1563 **** GUC_LIST_INPUT | GUC_LIST_QUOTE }, &preload_libraries_string, ! "", assign_canonical_path, NULL }, { --- 1557,1563 ---- GUC_LIST_INPUT | GUC_LIST_QUOTE }, &preload_libraries_string, ! "", NULL, NULL }, {
Bruce Momjian <pgman@candle.pha.pa.us> writes:
> Tom Lane wrote:
>> ... and probably breaks the GUC variables that represent search paths,
> OK, the only GUC I saw that was a list was preload_libraries, and this
> patch fixes that.
Nope, Dynamic_library_path is broken too.
regards, tom lane
OK, got it. THanks. --------------------------------------------------------------------------- Tom Lane wrote: > Bruce Momjian <pgman@candle.pha.pa.us> writes: > > Tom Lane wrote: > >> ... and probably breaks the GUC variables that represent search paths, > > > OK, the only GUC I saw that was a list was preload_libraries, and this > > patch fixes that. > > Nope, Dynamic_library_path is broken too. > > regards, tom lane > > ---------------------------(end of broadcast)--------------------------- > TIP 9: the planner will ignore your desire to choose an index scan if your > joining column's datatypes do not match > -- 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