Re: pg_upgrade and PGPORT - Mailing list pgsql-hackers

Bruce Momjian wrote:
> > >> ? ? ? Performing Consistency Checks
> > >> ? ? ? -----------------------------
> > >> ? ? ? ignoring libpq environment variable PGPORT
> > >
> > > I haven't tried it, but I suppose option.c will now make use of PGPORT
> > > and then later you get that message that it was ignored?
> >
> > Either way, it hardly seems necessary to emit a log message stating
> > that you are unsetting an environment variable.
>
> I think the whole idea of worrying about libpq environment variables is
> useless.  I looked at the list of libpq environment variables and I saw
> a lot of useful ones, like PGUSER and PGPASSFILE, which we currently
> throw an error.
>
> I propose we only disable the use of PGHOST and even then that prevents
> users from controlling tcp vs. unix domain connections.

OK, it turns out the environment variable handling in pg_upgrade was
worse than I thought.  This patch:

o  disables only PGHOST and only if it is set to a non-local value;
   all other environment variables are honored;  PGDATA isn't even seen
   by libpq
o  push --user value into the PGUSER environment variable so pg_ctl -w
   uses it;  pg_ctl has no --user flag; this is important for pre-9.1
   pg_ctl binaries
o  move putenv() function to utils.c now that it is used by option.c
o  allow pg_ctl failure to continue with a connection request to get a
   possible error message, then exit
o  update document to be clearer and mention environment variables

Patch attached.

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

  + It's impossible for everything to be true. +
diff --git a/contrib/pg_upgrade/controldata.c b/contrib/pg_upgrade/controldata.c
new file mode 100644
index 3ac2180..5ce4b95
*** a/contrib/pg_upgrade/controldata.c
--- b/contrib/pg_upgrade/controldata.c
***************
*** 11,18 ****

  #include <ctype.h>

- static void putenv2(const char *var, const char *val);
-
  /*
   * get_control_data()
   *
--- 11,16 ----
*************** get_control_data(ClusterInfo *cluster, b
*** 85,105 ****
      if (getenv("LC_MESSAGES"))
          lc_messages = pg_strdup(getenv("LC_MESSAGES"));

!     putenv2("LC_COLLATE", NULL);
!     putenv2("LC_CTYPE", NULL);
!     putenv2("LC_MONETARY", NULL);
!     putenv2("LC_NUMERIC", NULL);
!     putenv2("LC_TIME", NULL);
!     putenv2("LANG",
  #ifndef WIN32
              NULL);
  #else
      /* On Windows the default locale cannot be English, so force it */
              "en");
  #endif
!     putenv2("LANGUAGE", NULL);
!     putenv2("LC_ALL", NULL);
!     putenv2("LC_MESSAGES", "C");

      snprintf(cmd, sizeof(cmd), SYSTEMQUOTE "\"%s/%s \"%s\"" SYSTEMQUOTE,
               cluster->bindir,
--- 83,103 ----
      if (getenv("LC_MESSAGES"))
          lc_messages = pg_strdup(getenv("LC_MESSAGES"));

!     pg_putenv("LC_COLLATE", NULL);
!     pg_putenv("LC_CTYPE", NULL);
!     pg_putenv("LC_MONETARY", NULL);
!     pg_putenv("LC_NUMERIC", NULL);
!     pg_putenv("LC_TIME", NULL);
!     pg_putenv("LANG",
  #ifndef WIN32
              NULL);
  #else
      /* On Windows the default locale cannot be English, so force it */
              "en");
  #endif
!     pg_putenv("LANGUAGE", NULL);
!     pg_putenv("LC_ALL", NULL);
!     pg_putenv("LC_MESSAGES", "C");

      snprintf(cmd, sizeof(cmd), SYSTEMQUOTE "\"%s/%s \"%s\"" SYSTEMQUOTE,
               cluster->bindir,
*************** get_control_data(ClusterInfo *cluster, b
*** 374,388 ****
      /*
       * Restore environment variables
       */
!     putenv2("LC_COLLATE", lc_collate);
!     putenv2("LC_CTYPE", lc_ctype);
!     putenv2("LC_MONETARY", lc_monetary);
!     putenv2("LC_NUMERIC", lc_numeric);
!     putenv2("LC_TIME", lc_time);
!     putenv2("LANG", lang);
!     putenv2("LANGUAGE", language);
!     putenv2("LC_ALL", lc_all);
!     putenv2("LC_MESSAGES", lc_messages);

      pg_free(lc_collate);
      pg_free(lc_ctype);
--- 372,386 ----
      /*
       * Restore environment variables
       */
!     pg_putenv("LC_COLLATE", lc_collate);
!     pg_putenv("LC_CTYPE", lc_ctype);
!     pg_putenv("LC_MONETARY", lc_monetary);
!     pg_putenv("LC_NUMERIC", lc_numeric);
!     pg_putenv("LC_TIME", lc_time);
!     pg_putenv("LANG", lang);
!     pg_putenv("LANGUAGE", language);
!     pg_putenv("LC_ALL", lc_all);
!     pg_putenv("LC_MESSAGES", lc_messages);

      pg_free(lc_collate);
      pg_free(lc_ctype);
*************** rename_old_pg_control(void)
*** 529,568 ****
          pg_log(PG_FATAL, "Unable to rename %s to %s.\n", old_path, new_path);
      check_ok();
  }
-
-
- /*
-  *    putenv2()
-  *
-  *    This is like putenv(), but takes two arguments.
-  *    It also does unsetenv() if val is NULL.
-  */
- static void
- putenv2(const char *var, const char *val)
- {
-     if (val)
-     {
- #ifndef WIN32
-         char       *envstr = (char *) pg_malloc(strlen(var) +
-                                                 strlen(val) + 2);
-
-         sprintf(envstr, "%s=%s", var, val);
-         putenv(envstr);
-
-         /*
-          * Do not free envstr because it becomes part of the environment on
-          * some operating systems.    See port/unsetenv.c::unsetenv.
-          */
- #else
-         SetEnvironmentVariableA(var, val);
- #endif
-     }
-     else
-     {
- #ifndef WIN32
-         unsetenv(var);
- #else
-         SetEnvironmentVariableA(var, "");
- #endif
-     }
- }
--- 527,529 ----
diff --git a/contrib/pg_upgrade/option.c b/contrib/pg_upgrade/option.c
new file mode 100644
index 36561b9..e545458
*** a/contrib/pg_upgrade/option.c
--- b/contrib/pg_upgrade/option.c
*************** parseCommandLine(int argc, char *argv[])
*** 53,75 ****
      };
      int            option;            /* Command line option */
      int            optindex = 0;    /* used by getopt_long */
!     int            user_id;

!     if (getenv("PGUSER"))
!     {
!         pg_free(os_info.user);
!         os_info.user = pg_strdup(getenv("PGUSER"));
!     }

      os_info.progname = get_progname(argv[0]);
      old_cluster.port = getenv("PGPORT") ? atoi(getenv("PGPORT")) : DEF_PGPORT;
      new_cluster.port = getenv("PGPORT") ? atoi(getenv("PGPORT")) : DEF_PGPORT;
-     /* must save value, getenv()'s pointer is not stable */

!     user_opts.transfer_mode = TRANSFER_MODE_COPY;
!
!     /* user lookup and 'root' test must be split because of usage() */
!     user_id = get_user_info(&os_info.user);

      if (argc > 1)
      {
--- 53,76 ----
      };
      int            option;            /* Command line option */
      int            optindex = 0;    /* used by getopt_long */
!     int            os_user_effective_id;

!     user_opts.transfer_mode = TRANSFER_MODE_COPY;

      os_info.progname = get_progname(argv[0]);
+
+     /* Process libpq env. variables; load values here for usage() output */
      old_cluster.port = getenv("PGPORT") ? atoi(getenv("PGPORT")) : DEF_PGPORT;
      new_cluster.port = getenv("PGPORT") ? atoi(getenv("PGPORT")) : DEF_PGPORT;

!     os_user_effective_id = get_user_info(&os_info.user);
!     /* we override just the database user name;  we got the OS id above */
!     if (getenv("PGUSER"))
!     {
!         pg_free(os_info.user);
!         /* must save value, getenv()'s pointer is not stable */
!         os_info.user = pg_strdup(getenv("PGUSER"));
!     }

      if (argc > 1)
      {
*************** parseCommandLine(int argc, char *argv[])
*** 86,92 ****
          }
      }

!     if (user_id == 0)
          pg_log(PG_FATAL, "%s: cannot be run as root\n", os_info.progname);

      getcwd(os_info.cwd, MAXPGPATH);
--- 87,94 ----
          }
      }

!     /* Allow help and version to be run as root, so do the test here. */
!     if (os_user_effective_id == 0)
          pg_log(PG_FATAL, "%s: cannot be run as root\n", os_info.progname);

      getcwd(os_info.cwd, MAXPGPATH);
*************** parseCommandLine(int argc, char *argv[])
*** 96,109 ****
      {
          switch (option)
          {
-             case 'd':
-                 old_cluster.pgdata = pg_strdup(optarg);
-                 break;
-
-             case 'D':
-                 new_cluster.pgdata = pg_strdup(optarg);
-                 break;
-
              case 'b':
                  old_cluster.bindir = pg_strdup(optarg);
                  break;
--- 98,103 ----
*************** parseCommandLine(int argc, char *argv[])
*** 116,121 ****
--- 110,123 ----
                  user_opts.check = true;
                  break;

+             case 'd':
+                 old_cluster.pgdata = pg_strdup(optarg);
+                 break;
+
+             case 'D':
+                 new_cluster.pgdata = pg_strdup(optarg);
+                 break;
+
              case 'g':
                  pg_log(PG_REPORT, "Running in debug mode\n");
                  log_opts.debug = true;
*************** parseCommandLine(int argc, char *argv[])
*** 156,161 ****
--- 158,168 ----
              case 'u':
                  pg_free(os_info.user);
                  os_info.user = pg_strdup(optarg);
+                 /*
+                  * Push the user name into the environment so pre-9.1
+                  * pg_ctl/libpq uses it.
+                  */
+                 pg_putenv("PGUSER", os_info.user);
                  break;

              case 'v':
*************** parseCommandLine(int argc, char *argv[])
*** 197,210 ****
      }

      /* Get values from env if not already set */
-     validateDirectoryOption(&old_cluster.pgdata, "OLDDATADIR", "-d",
-                             "old cluster data resides");
-     validateDirectoryOption(&new_cluster.pgdata, "NEWDATADIR", "-D",
-                             "new cluster data resides");
      validateDirectoryOption(&old_cluster.bindir, "OLDBINDIR", "-b",
                              "old cluster binaries reside");
      validateDirectoryOption(&new_cluster.bindir, "NEWBINDIR", "-B",
                              "new cluster binaries reside");

      get_pkglibdirs();
  }
--- 204,217 ----
      }

      /* Get values from env if not already set */
      validateDirectoryOption(&old_cluster.bindir, "OLDBINDIR", "-b",
                              "old cluster binaries reside");
      validateDirectoryOption(&new_cluster.bindir, "NEWBINDIR", "-B",
                              "new cluster binaries reside");
+     validateDirectoryOption(&old_cluster.pgdata, "OLDDATADIR", "-d",
+                             "old cluster data resides");
+     validateDirectoryOption(&new_cluster.pgdata, "NEWDATADIR", "-D",
+                             "new cluster data resides");

      get_pkglibdirs();
  }
diff --git a/contrib/pg_upgrade/pg_upgrade.c b/contrib/pg_upgrade/pg_upgrade.c
new file mode 100644
index 857e829..6eaaa0f
*** a/contrib/pg_upgrade/pg_upgrade.c
--- b/contrib/pg_upgrade/pg_upgrade.c
*************** setup(char *argv0, bool live_check)
*** 149,155 ****
       * make sure the user has a clean environment, otherwise, we may confuse
       * libpq when we connect to one (or both) of the servers.
       */
!     check_for_libpq_envvars();

      verify_directories();

--- 149,155 ----
       * make sure the user has a clean environment, otherwise, we may confuse
       * libpq when we connect to one (or both) of the servers.
       */
!     check_pghost_envvar();

      verify_directories();

diff --git a/contrib/pg_upgrade/pg_upgrade.h b/contrib/pg_upgrade/pg_upgrade.h
new file mode 100644
index 04f67e1..1f31dae
*** a/contrib/pg_upgrade/pg_upgrade.h
--- b/contrib/pg_upgrade/pg_upgrade.h
*************** PGresult   *executeQueryOrDie(PGconn *co
*** 361,367 ****
  void        start_postmaster(ClusterInfo *cluster);
  void        stop_postmaster(bool fast);
  uint32        get_major_server_version(ClusterInfo *cluster);
! void        check_for_libpq_envvars(void);


  /* util.c */
--- 361,367 ----
  void        start_postmaster(ClusterInfo *cluster);
  void        stop_postmaster(bool fast);
  uint32        get_major_server_version(ClusterInfo *cluster);
! void        check_pghost_envvar(void);


  /* util.c */
*************** void       *pg_malloc(int size);
*** 378,383 ****
--- 378,384 ----
  void        pg_free(void *ptr);
  const char *getErrorText(int errNum);
  unsigned int str2uint(const char *str);
+ void         pg_putenv(const char *var, const char *val);


  /* version.c */
diff --git a/contrib/pg_upgrade/server.c b/contrib/pg_upgrade/server.c
new file mode 100644
index 8fce305..df4b38b
*** a/contrib/pg_upgrade/server.c
--- b/contrib/pg_upgrade/server.c
*************** start_postmaster(ClusterInfo *cluster)
*** 145,150 ****
--- 145,151 ----
      char        cmd[MAXPGPATH];
      PGconn       *conn;
      bool        exit_hook_registered = false;
+     int            pg_ctl_return = 0;
  #ifndef WIN32
      char        *output_filename = log_opts.filename;
  #else
*************** start_postmaster(ClusterInfo *cluster)
*** 183,189 ****
                  "-c autovacuum=off -c autovacuum_freeze_max_age=2000000000",
               log_opts.filename);

!     exec_prog(true, "%s", cmd);

      /* Check to see if we can connect to the server; if not, report it. */
      if ((conn = get_db_conn(cluster, "template1")) == NULL ||
--- 184,194 ----
                  "-c autovacuum=off -c autovacuum_freeze_max_age=2000000000",
               log_opts.filename);

!     /*
!      * Don't throw an error right away, let connecting throw the error
!      * because it might supply a reason for the failure.
!      */
!     pg_ctl_return = exec_prog(false, "%s", cmd);

      /* Check to see if we can connect to the server; if not, report it. */
      if ((conn = get_db_conn(cluster, "template1")) == NULL ||
*************** start_postmaster(ClusterInfo *cluster)
*** 198,203 ****
--- 203,213 ----
      }
      PQfinish(conn);

+     /* If the connection didn't fail, fail now */
+     if (pg_ctl_return != 0)
+         pg_log(PG_FATAL, "pg_ctl failed to start the %s server\n",
+                 CLUSTER_NAME(cluster));
+
      os_info.running_cluster = cluster;
  }

*************** stop_postmaster(bool fast)
*** 241,260 ****


  /*
!  * check_for_libpq_envvars()
!  *
!  * tests whether any libpq environment variables are set.
!  * Since pg_upgrade connects to both the old and the new server,
!  * it is potentially dangerous to have any of these set.
   *
!  * If any are found, will log them and cancel.
   */
  void
! check_for_libpq_envvars(void)
  {
      PQconninfoOption *option;
      PQconninfoOption *start;
-     bool        found = false;

      /* Get valid libpq env vars from the PQconndefaults function */

--- 251,265 ----


  /*
!  * check_pghost_envvar()
   *
!  * Tests that PGHOST does not point to a non-local server
   */
  void
! check_pghost_envvar(void)
  {
      PQconninfoOption *option;
      PQconninfoOption *start;

      /* Get valid libpq env vars from the PQconndefaults function */

*************** check_for_libpq_envvars(void)
*** 262,290 ****

      for (option = start; option->keyword != NULL; option++)
      {
!         if (option->envvar)
          {
!             const char *value;
!
!             /* This allows us to see error messages in the local encoding */
!             if (strcmp(option->envvar, "PGCLIENTENCODING") == 0)
!                 continue;
!
!             value = getenv(option->envvar);
!             if (value && strlen(value) > 0)
!             {
!                 found = true;

!                 pg_log(PG_WARNING,
!                        "libpq env var %-20s is currently set to: %s\n", option->envvar, value);
!             }
          }
      }

      /* Free the memory that libpq allocated on our behalf */
      PQconninfoFree(start);
-
-     if (found)
-         pg_log(PG_FATAL,
-                "libpq env vars have been found and listed above, please unset them for pg_upgrade\n");
  }
--- 267,284 ----

      for (option = start; option->keyword != NULL; option++)
      {
!         if (option->envvar && strcmp(option->envvar, "PGHOST") == 0)
          {
!             const char *value = getenv(option->envvar);

!             if (value && strlen(value) > 0 &&
!                 /* check for 'local' values */
!                 (strcmp(value, "localhost") != 0 && strcmp(value, "::1") != 0))
!                 pg_log(PG_FATAL,
!                     "libpq environment PGHOST points to a non-local server %s\n", option->envvar);
          }
      }

      /* Free the memory that libpq allocated on our behalf */
      PQconninfoFree(start);
  }
diff --git a/contrib/pg_upgrade/util.c b/contrib/pg_upgrade/util.c
new file mode 100644
index 9b0bf0f..4094895
*** a/contrib/pg_upgrade/util.c
--- b/contrib/pg_upgrade/util.c
*************** str2uint(const char *str)
*** 244,246 ****
--- 244,284 ----
  {
      return strtoul(str, NULL, 10);
  }
+
+
+ /*
+  *    pg_putenv()
+  *
+  *    This is like putenv(), but takes two arguments.
+  *    It also does unsetenv() if val is NULL.
+  */
+ void
+ pg_putenv(const char *var, const char *val)
+ {
+     if (val)
+     {
+ #ifndef WIN32
+         char       *envstr = (char *) pg_malloc(strlen(var) +
+                                                 strlen(val) + 2);
+
+         sprintf(envstr, "%s=%s", var, val);
+         putenv(envstr);
+
+         /*
+          * Do not free envstr because it becomes part of the environment on
+          * some operating systems.    See port/unsetenv.c::unsetenv.
+          */
+ #else
+         SetEnvironmentVariableA(var, val);
+ #endif
+     }
+     else
+     {
+ #ifndef WIN32
+         unsetenv(var);
+ #else
+         SetEnvironmentVariableA(var, "");
+ #endif
+     }
+ }
+
diff --git a/doc/src/sgml/pgupgrade.sgml b/doc/src/sgml/pgupgrade.sgml
new file mode 100644
index 1713bf0..8bd5f74
*** a/doc/src/sgml/pgupgrade.sgml
--- b/doc/src/sgml/pgupgrade.sgml
***************
*** 58,71 ****

       <varlistentry>
        <term><option>-b</option> <replaceable>old_bindir</></term>
!       <term><option>--old-bindir=</option><replaceable>OLDBINDIR</></term>
!       <listitem><para>specify the old cluster executable directory</para></listitem>
       </varlistentry>

       <varlistentry>
        <term><option>-B</option> <replaceable>new_bindir</></term>
!       <term><option>--new-bindir=</option><replaceable>NEWBINDIR</></term>
!       <listitem><para>specify the new cluster executable directory</para></listitem>
       </varlistentry>

       <varlistentry>
--- 58,73 ----

       <varlistentry>
        <term><option>-b</option> <replaceable>old_bindir</></term>
!       <term><option>--old-bindir=</option><replaceable>old_bindir</></term>
!       <listitem><para>the old cluster executable directory;
!       environment variable <envar>OLDBINDIR</></para></listitem>
       </varlistentry>

       <varlistentry>
        <term><option>-B</option> <replaceable>new_bindir</></term>
!       <term><option>--new-bindir=</option><replaceable>new_bindir</></term>
!       <listitem><para>the new cluster executable directory;
!       environment variable <envar>NEWBINDIR</></para></listitem>
       </varlistentry>

       <varlistentry>
***************
*** 76,89 ****

       <varlistentry>
        <term><option>-d</option> <replaceable>old_datadir</></term>
!       <term><option>--old-datadir=</option><replaceable>OLDDATADIR</></term>
!       <listitem><para>specify the old cluster data directory</para></listitem>
       </varlistentry>

       <varlistentry>
        <term><option>-D</option> <replaceable>new_datadir</></term>
!       <term><option>--new-datadir=</option><replaceable>NEWDATADIR</></term>
!       <listitem><para>specify the new cluster data directory</para></listitem>
       </varlistentry>

       <varlistentry>
--- 78,93 ----

       <varlistentry>
        <term><option>-d</option> <replaceable>old_datadir</></term>
!       <term><option>--old-datadir=</option><replaceable>old_datadir</></term>
!       <listitem><para>the old cluster data directory; environment
!       variable <envar>OLDDATADIR</></para></listitem>
       </varlistentry>

       <varlistentry>
        <term><option>-D</option> <replaceable>new_datadir</></term>
!       <term><option>--new-datadir=</option><replaceable>new_datadir</></term>
!       <listitem><para>the new cluster data directory; environment
!       variable <envar>NEWDATADIR</></para></listitem>
       </varlistentry>

       <varlistentry>
***************
*** 94,100 ****

       <varlistentry>
        <term><option>-G</option> <replaceable>debug_filename</></term>
!       <term><option>--debugfile=</option><replaceable>DEBUGFILENAME</></term>
        <listitem><para>output debugging activity to file</para></listitem>
       </varlistentry>

--- 98,104 ----

       <varlistentry>
        <term><option>-G</option> <replaceable>debug_filename</></term>
!       <term><option>--debugfile=</option><replaceable>debug_filename</></term>
        <listitem><para>output debugging activity to file</para></listitem>
       </varlistentry>

***************
*** 106,131 ****

       <varlistentry>
        <term><option>-l</option> <replaceable>log_filename</></term>
!       <term><option>--logfile=</option><replaceable>LOGFILENAME</></term>
        <listitem><para>log session activity to file</para></listitem>
       </varlistentry>

       <varlistentry>
!       <term><option>-p</option> <replaceable>old_portnum</></term>
!       <term><option>--old-port=</option><replaceable>portnum</></term>
!       <listitem><para>specify the old cluster port number</para></listitem>
       </varlistentry>

       <varlistentry>
!       <term><option>-P</option> <replaceable>new_portnum</></term>
!       <term><option>--new-port=</option><replaceable>portnum</></term>
!       <listitem><para>specify the new cluster port number</para></listitem>
       </varlistentry>

       <varlistentry>
!       <term><option>-u</option> <replaceable>username</></term>
!       <term><option>--user=</option><replaceable>username</></term>
!       <listitem><para>clusters superuser</para></listitem>
       </varlistentry>

       <varlistentry>
--- 110,138 ----

       <varlistentry>
        <term><option>-l</option> <replaceable>log_filename</></term>
!       <term><option>--logfile=</option><replaceable>log_filename</></term>
        <listitem><para>log session activity to file</para></listitem>
       </varlistentry>

       <varlistentry>
!       <term><option>-p</option> <replaceable>old_port_number</></term>
!       <term><option>--old-port=</option><replaceable>old_portnum</></term>
!       <listitem><para>the old cluster port number; environment
!       variable <envar>PGPORT</></para></listitem>
       </varlistentry>

       <varlistentry>
!       <term><option>-P</option> <replaceable>new_port_number</></term>
!       <term><option>--new-port=</option><replaceable>new_portnum</></term>
!       <listitem><para>the new cluster port number; environment
!       variable <envar>PGPORT</></para></listitem>
       </varlistentry>

       <varlistentry>
!       <term><option>-u</option> <replaceable>user_name</></term>
!       <term><option>--user=</option><replaceable>user_name</></term>
!       <listitem><para>cluster's super user name; environment
!       variable <envar>PGUSER</></para></listitem>
       </varlistentry>

       <varlistentry>