Thread: pg_upgrade and PGPORT

pg_upgrade and PGPORT

From
Peter Eisentraut
Date:
pg_upgrade is a bit schizophrenic concerning the PGPORT environment
variable.  On the one hand, there is this code in option.c that wants to
make use of it:
   old_cluster.port = getenv("PGPORT") ? atoi(getenv("PGPORT")) : DEF_PGPORT;   new_cluster.port = getenv("PGPORT") ?
atoi(getenv("PGPORT")): DEF_PGPORT;
 
On the other hand, check.c will reject a set PGPORT because it's a libpq
environment variable.  Should we make an exception for PGPORT, like we
did for PGCLIENTENCODING?




Re: pg_upgrade and PGPORT

From
Bruce Momjian
Date:
Peter Eisentraut wrote:
> pg_upgrade is a bit schizophrenic concerning the PGPORT environment
> variable.  On the one hand, there is this code in option.c that wants to
> make use of it:
> 
>     old_cluster.port = getenv("PGPORT") ? atoi(getenv("PGPORT")) : DEF_PGPORT;
>     new_cluster.port = getenv("PGPORT") ? atoi(getenv("PGPORT")) : DEF_PGPORT;
>  
> On the other hand, check.c will reject a set PGPORT because it's a libpq
> environment variable.  Should we make an exception for PGPORT, like we
> did for PGCLIENTENCODING?

Wow, good question.  Passing stuff in via libpq is certainly complex.

I ran a test and it looks like the command-line flag overrides the
PGPORT environment variable:
$ export PGPORT=3333$ psql testpsql: could not connect to server: No such file or directory        Is the server
runninglocally and accepting        connections on Unix domain socket "/tmp/.s.PGSQL.3333"?$ psql -p 5432 testpsql
(9.1beta1)Type"help" for help.test=>
 

I assume it is just like PGCLIENTENCODING.  PGCLIENTENCODING was easier
to ignore because we need it for error messages.

Are there other cases we should allow too?

A larger question is whether we should just disable all the checks for
environment variables.  The C comment says:
* check_for_libpq_envvars()** tests whether any libpq environment variables are set.* Since pg_upgrade connects to both
theold and the new server,* it is potentially dangerous to have any of these set.** If any are found, will log them and
cancel.

I am not sure what to do.

--  Bruce Momjian  <bruce@momjian.us>        http://momjian.us EnterpriseDB
http://enterprisedb.com
 + It's impossible for everything to be true. +


Re: pg_upgrade and PGPORT

From
Tom Lane
Date:
Bruce Momjian <bruce@momjian.us> writes:
> A larger question is whether we should just disable all the checks for
> environment variables.  The C comment says:

>  * 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.

> I am not sure what to do.

Well, the risk mentioned in that comment certainly seems real.

An alternative solution that might be more user-friendly is to ensure
that the connection strings pg_upgrade uses specify all important
options, leaving nothing to be overridden by environment variables.
Then you don't need to make the user adjust his environment.

Or you could just "unsetenv" instead of complaining.

I would like to think that eventually pg_upgrade won't start a
postmaster at all, but connect using something more like a standalone
backend.  So someday the issue might go away --- but that someday
isn't especially close.
        regards, tom lane


Re: pg_upgrade and PGPORT

From
Bruce Momjian
Date:
Tom Lane wrote:
> Bruce Momjian <bruce@momjian.us> writes:
> > A larger question is whether we should just disable all the checks for
> > environment variables.  The C comment says:
> 
> >  * 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.
> 
> > I am not sure what to do.
> 
> Well, the risk mentioned in that comment certainly seems real.
> 
> An alternative solution that might be more user-friendly is to ensure
> that the connection strings pg_upgrade uses specify all important
> options, leaving nothing to be overridden by environment variables.
> Then you don't need to make the user adjust his environment.

Well, they can use the same port number for both servers.  In fact I
just use the compiled-default of 5432 when I am testing.  No reason they
could not supply value in an environment variable but it would have to
be the same for old and new server (the two servers never run at the
same time).

> Or you could just "unsetenv" instead of complaining.

I think it is really PGDATA that we certainly can't inherit from an
environment variable.

> I would like to think that eventually pg_upgrade won't start a
> postmaster at all, but connect using something more like a standalone
> backend.  So someday the issue might go away --- but that someday
> isn't especially close.

That standalone backend is going to have to understand pg_dump SQL
output, with \connect, etc.

--  Bruce Momjian  <bruce@momjian.us>        http://momjian.us EnterpriseDB
http://enterprisedb.com
 + It's impossible for everything to be true. +


Re: pg_upgrade and PGPORT

From
Robert Haas
Date:
On Wed, May 11, 2011 at 2:18 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Or you could just "unsetenv" instead of complaining.

+1 for that.

> I would like to think that eventually pg_upgrade won't start a
> postmaster at all, but connect using something more like a standalone
> backend.  So someday the issue might go away --- but that someday
> isn't especially close.

And +1 for that, too.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


Re: pg_upgrade and PGPORT

From
Bruce Momjian
Date:
Robert Haas wrote:
> On Wed, May 11, 2011 at 2:18 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > Or you could just "unsetenv" instead of complaining.
>
> +1 for that.

OK, the attached patch does this, but allows PGCLIENTENCODING to be
passed in.  The new output looks like:

    Performing Consistency Checks
    -----------------------------
    ignoring libpq environment variable PGPORT
    Checking old data directory (/u/pgsql.old/data)             ok
    Checking old bin directory (/u/pgsql.old/bin)               ok
    Checking new data directory (/u/pgsql/data)                 ok
    Checking new bin directory (/u/pgsql/bin)                   ok

--
  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/server.c b/contrib/pg_upgrade/server.c
new file mode 100644
index 8fce305..bf30dcd
*** a/contrib/pg_upgrade/server.c
--- b/contrib/pg_upgrade/server.c
*************** check_for_libpq_envvars(void)
*** 254,260 ****
  {
      PQconninfoOption *option;
      PQconninfoOption *start;
-     bool        found = false;

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

--- 254,259 ----
*************** check_for_libpq_envvars(void)
*** 264,290 ****
      {
          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");
  }
--- 263,287 ----
      {
          if (option->envvar)
          {
!             const char *value = getenv(option->envvar);

              /* This allows us to see error messages in the local encoding */
              if (strcmp(option->envvar, "PGCLIENTENCODING") == 0)
                  continue;

              if (value && strlen(value) > 0)
              {
! #ifndef WIN32
!                 unsetenv(option->envvar);
! #else
!                 SetEnvironmentVariableA(option->envvar, "");
! #endif
                  pg_log(PG_WARNING,
!                     "ignoring libpq environment variable %s\n", option->envvar);
              }
          }
      }

      /* Free the memory that libpq allocated on our behalf */
      PQconninfoFree(start);
  }

Re: pg_upgrade and PGPORT

From
Peter Eisentraut
Date:
On ons, 2011-05-11 at 18:36 -0400, Bruce Momjian wrote:
> Robert Haas wrote:
> > On Wed, May 11, 2011 at 2:18 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > > Or you could just "unsetenv" instead of complaining.
> > 
> > +1 for that.
> 
> OK, the attached patch does this, but allows PGCLIENTENCODING to be
> passed in.  The new output looks like:
> 
>     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?




Re: pg_upgrade and PGPORT

From
Robert Haas
Date:
On Thu, May 12, 2011 at 1:22 AM, Peter Eisentraut <peter_e@gmx.net> wrote:
> On ons, 2011-05-11 at 18:36 -0400, Bruce Momjian wrote:
>> Robert Haas wrote:
>> > On Wed, May 11, 2011 at 2:18 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> > > Or you could just "unsetenv" instead of complaining.
>> >
>> > +1 for that.
>>
>> OK, the attached patch does this, but allows PGCLIENTENCODING to be
>> passed in.  The new output looks like:
>>
>>       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.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


Re: pg_upgrade and PGPORT

From
Bruce Momjian
Date:
Robert Haas wrote:
> On Thu, May 12, 2011 at 1:22 AM, Peter Eisentraut <peter_e@gmx.net> wrote:
> > On ons, 2011-05-11 at 18:36 -0400, Bruce Momjian wrote:
> >> Robert Haas wrote:
> >> > On Wed, May 11, 2011 at 2:18 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >> > > Or you could just "unsetenv" instead of complaining.
> >> >
> >> > +1 for that.
> >>
> >> OK, the attached patch does this, but allows PGCLIENTENCODING to be
> >> passed in. ?The new output looks like:
> >>
> >> ? ? ? 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.

--  Bruce Momjian  <bruce@momjian.us>        http://momjian.us EnterpriseDB
http://enterprisedb.com
 + It's impossible for everything to be true. +


Re: pg_upgrade and PGPORT

From
Bruce Momjian
Date:
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>

Re: pg_upgrade and PGPORT

From
Bruce Momjian
Date:
Bruce Momjian wrote:
> 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.

I have applied the updated attached patch which allows pg_upgrade to
honor environment variable.  This updated version is clearer about
handling of PGHOST, and adds PGHOSTADDR checks.

--
  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..839f39f
*** 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,287 ----

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

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

      /* 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>