Re: pgsql-server: Use canonicalize_path for -D, GUC paths, - Mailing list pgsql-committers

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;
      }