Re: Yet another failure mode in pg_upgrade - Mailing list pgsql-hackers

Bruce Momjian <bruce@momjian.us> writes:
> Updated patch attached.

[ looks at that for a bit... ]  Now I see why you were on about that:
the method you used here requires both clusters to have the same socket
directory.  Which is silly and unnecessary.  Revised patch attached.

            regards, tom lane

diff --git a/contrib/pg_upgrade/check.c b/contrib/pg_upgrade/check.c
index 0fec73e..efb080b 100644
*** a/contrib/pg_upgrade/check.c
--- b/contrib/pg_upgrade/check.c
*************** issue_warnings(char *sequence_script_fil
*** 184,191 ****
          {
              prep_status("Adjusting sequences");
              exec_prog(UTILITY_LOG_FILE, NULL, true,
!                       "\"%s/psql\" " EXEC_PSQL_ARGS " --port %d --username \"%s\" -f \"%s\"",
!                       new_cluster.bindir, new_cluster.port, os_info.user,
                        sequence_script_file_name);
              unlink(sequence_script_file_name);
              check_ok();
--- 184,191 ----
          {
              prep_status("Adjusting sequences");
              exec_prog(UTILITY_LOG_FILE, NULL, true,
!                       "\"%s/psql\" " EXEC_PSQL_ARGS " %s -f \"%s\"",
!                       new_cluster.bindir, cluster_conn_opts(&new_cluster),
                        sequence_script_file_name);
              unlink(sequence_script_file_name);
              check_ok();
diff --git a/contrib/pg_upgrade/dump.c b/contrib/pg_upgrade/dump.c
index cfc4017..b905ab0 100644
*** a/contrib/pg_upgrade/dump.c
--- b/contrib/pg_upgrade/dump.c
*************** generate_old_dump(void)
*** 24,31 ****
       * restores the frozenid's for databases and relations.
       */
      exec_prog(UTILITY_LOG_FILE, NULL, true,
!               "\"%s/pg_dumpall\" --port %d --username \"%s\" --schema-only --binary-upgrade %s -f %s",
!               new_cluster.bindir, old_cluster.port, os_info.user,
                log_opts.verbose ? "--verbose" : "",
                ALL_DUMP_FILE);
      check_ok();
--- 24,31 ----
       * restores the frozenid's for databases and relations.
       */
      exec_prog(UTILITY_LOG_FILE, NULL, true,
!               "\"%s/pg_dumpall\" %s --schema-only --binary-upgrade %s -f %s",
!               new_cluster.bindir, cluster_conn_opts(&old_cluster),
                log_opts.verbose ? "--verbose" : "",
                ALL_DUMP_FILE);
      check_ok();
diff --git a/contrib/pg_upgrade/option.c b/contrib/pg_upgrade/option.c
index 94bce50..80f7d34 100644
*** a/contrib/pg_upgrade/option.c
--- b/contrib/pg_upgrade/option.c
*************** adjust_data_dir(ClusterInfo *cluster)
*** 376,378 ****
--- 376,439 ----

      check_ok();
  }
+
+
+ /*
+  * get_sock_dir
+  *
+  * Identify the socket directory to use for this cluster.  If we're doing
+  * a live check (old cluster only), we need to find out where the postmaster
+  * is listening.  Otherwise, we're going to put the socket into the current
+  * directory.
+  */
+ void
+ get_sock_dir(ClusterInfo *cluster, bool live_check)
+ {
+ #if HAVE_UNIX_SOCKETS
+     if (!live_check)
+     {
+         /* Use the current directory for the socket */
+         cluster->sockdir = pg_malloc(MAXPGPATH);
+         if (!getcwd(cluster->sockdir, MAXPGPATH))
+             pg_log(PG_FATAL, "cannot find current directory\n");
+     }
+     else
+     {
+         /*
+          *    If we are doing a live check, we will use the old cluster's Unix
+          *    domain socket directory so we can connect to the live server.
+          */
+
+         /* sockdir added to the 5th line of postmaster.pid in PG 9.1 */
+         if (GET_MAJOR_VERSION(cluster->major_version) >= 901)
+         {
+             char        filename[MAXPGPATH];
+             FILE        *fp;
+             int            i;
+
+             snprintf(filename, sizeof(filename), "%s/postmaster.pid",
+                      cluster->pgdata);
+             if ((fp = fopen(filename, "r")) == NULL)
+                 pg_log(PG_FATAL, "Could not get socket directory of the old server\n");
+
+             cluster->sockdir = pg_malloc(MAXPGPATH);
+             for (i = 0; i < 5; i++)
+                 if (fgets(cluster->sockdir, MAXPGPATH, fp) == NULL)
+                     pg_log(PG_FATAL, "Could not get socket directory of the old server\n");
+
+             fclose(fp);
+
+             /* Remove trailing newline */
+             if (strchr(cluster->sockdir, '\n') != NULL)
+                 *strchr(cluster->sockdir, '\n') = '\0';
+         }
+         else
+         {
+             /* Can't get live sockdir, so assume the default is OK. */
+             cluster->sockdir = NULL;
+         }
+     }
+ #else /* !HAVE_UNIX_SOCKETS */
+     cluster->sockdir = NULL;
+ #endif
+ }
diff --git a/contrib/pg_upgrade/pg_upgrade.c b/contrib/pg_upgrade/pg_upgrade.c
index c47c8bb..ee3a151 100644
*** a/contrib/pg_upgrade/pg_upgrade.c
--- b/contrib/pg_upgrade/pg_upgrade.c
*************** main(int argc, char **argv)
*** 88,93 ****
--- 88,96 ----
      check_cluster_versions();
      check_cluster_compatibility(live_check);

+     get_sock_dir(&old_cluster, live_check);
+     get_sock_dir(&new_cluster, false);
+
      check_old_cluster(live_check, &sequence_script_file_name);


*************** prepare_new_cluster(void)
*** 211,218 ****
       */
      prep_status("Analyzing all rows in the new cluster");
      exec_prog(UTILITY_LOG_FILE, NULL, true,
!               "\"%s/vacuumdb\" --port %d --username \"%s\" --all --analyze %s",
!               new_cluster.bindir, new_cluster.port, os_info.user,
                log_opts.verbose ? "--verbose" : "");
      check_ok();

--- 214,221 ----
       */
      prep_status("Analyzing all rows in the new cluster");
      exec_prog(UTILITY_LOG_FILE, NULL, true,
!               "\"%s/vacuumdb\" %s --all --analyze %s",
!               new_cluster.bindir, cluster_conn_opts(&new_cluster),
                log_opts.verbose ? "--verbose" : "");
      check_ok();

*************** prepare_new_cluster(void)
*** 224,231 ****
       */
      prep_status("Freezing all rows on the new cluster");
      exec_prog(UTILITY_LOG_FILE, NULL, true,
!               "\"%s/vacuumdb\" --port %d --username \"%s\" --all --freeze %s",
!               new_cluster.bindir, new_cluster.port, os_info.user,
                log_opts.verbose ? "--verbose" : "");
      check_ok();

--- 227,234 ----
       */
      prep_status("Freezing all rows on the new cluster");
      exec_prog(UTILITY_LOG_FILE, NULL, true,
!               "\"%s/vacuumdb\" %s --all --freeze %s",
!               new_cluster.bindir, cluster_conn_opts(&new_cluster),
                log_opts.verbose ? "--verbose" : "");
      check_ok();

*************** prepare_new_databases(void)
*** 261,268 ****
       * the template0 template.
       */
      exec_prog(RESTORE_LOG_FILE, NULL, true,
!               "\"%s/psql\" " EXEC_PSQL_ARGS " --port %d --username \"%s\" -f \"%s\"",
!               new_cluster.bindir, new_cluster.port, os_info.user,
                GLOBALS_DUMP_FILE);
      check_ok();

--- 264,271 ----
       * the template0 template.
       */
      exec_prog(RESTORE_LOG_FILE, NULL, true,
!               "\"%s/psql\" " EXEC_PSQL_ARGS " %s -f \"%s\"",
!               new_cluster.bindir, cluster_conn_opts(&new_cluster),
                GLOBALS_DUMP_FILE);
      check_ok();

*************** create_new_objects(void)
*** 290,297 ****

      prep_status("Restoring database schema to new cluster");
      exec_prog(RESTORE_LOG_FILE, NULL, true,
!               "\"%s/psql\" " EXEC_PSQL_ARGS " --port %d --username \"%s\" -f \"%s\"",
!               new_cluster.bindir, new_cluster.port, os_info.user,
                DB_DUMP_FILE);
      check_ok();

--- 293,300 ----

      prep_status("Restoring database schema to new cluster");
      exec_prog(RESTORE_LOG_FILE, NULL, true,
!               "\"%s/psql\" " EXEC_PSQL_ARGS " %s -f \"%s\"",
!               new_cluster.bindir, cluster_conn_opts(&new_cluster),
                DB_DUMP_FILE);
      check_ok();

diff --git a/contrib/pg_upgrade/pg_upgrade.h b/contrib/pg_upgrade/pg_upgrade.h
index fa4c6c0..7b19d91 100644
*** a/contrib/pg_upgrade/pg_upgrade.h
--- b/contrib/pg_upgrade/pg_upgrade.h
*************** typedef struct
*** 226,231 ****
--- 226,232 ----
      char       *bindir;            /* pathname for cluster's executable directory */
      char       *pgopts;            /* options to pass to the server, like pg_ctl
                                   * -o */
+     char       *sockdir;        /* directory for Unix Domain socket, if any */
      unsigned short port;        /* port number where postmaster is waiting */
      uint32        major_version;    /* PG_VERSION of cluster */
      char        major_version_str[64];    /* string PG_VERSION of cluster */
*************** void print_maps(FileNameMap *maps, int n
*** 387,392 ****
--- 388,394 ----

  void        parseCommandLine(int argc, char *argv[]);
  void        adjust_data_dir(ClusterInfo *cluster);
+ void        get_sock_dir(ClusterInfo *cluster, bool live_check);

  /* relfilenode.c */

*************** PGresult *
*** 407,412 ****
--- 409,416 ----
  executeQueryOrDie(PGconn *conn, const char *fmt,...)
  __attribute__((format(PG_PRINTF_ATTRIBUTE, 2, 3)));

+ char       *cluster_conn_opts(ClusterInfo *cluster);
+
  void        start_postmaster(ClusterInfo *cluster);
  void        stop_postmaster(bool fast);
  uint32        get_major_server_version(ClusterInfo *cluster);
diff --git a/contrib/pg_upgrade/server.c b/contrib/pg_upgrade/server.c
index 1fb0d6c..bf146b3 100644
*** a/contrib/pg_upgrade/server.c
--- b/contrib/pg_upgrade/server.c
*************** connectToServer(ClusterInfo *cluster, co
*** 46,67 ****
  /*
   * get_db_conn()
   *
!  * get database connection
   */
  static PGconn *
  get_db_conn(ClusterInfo *cluster, const char *db_name)
  {
!     char        conn_opts[MAXPGPATH];

!     snprintf(conn_opts, sizeof(conn_opts),
!              "dbname = '%s' user = '%s' port = %d", db_name, os_info.user,
!              cluster->port);

      return PQconnectdb(conn_opts);
  }


  /*
   * executeQueryOrDie()
   *
   *    Formats a query string from the given arguments and executes the
--- 46,100 ----
  /*
   * get_db_conn()
   *
!  * get database connection, using named database + standard params for cluster
   */
  static PGconn *
  get_db_conn(ClusterInfo *cluster, const char *db_name)
  {
!     char        conn_opts[2 * NAMEDATALEN + MAXPGPATH + 100];

!     if (cluster->sockdir)
!         snprintf(conn_opts, sizeof(conn_opts),
!                  "dbname = '%s' user = '%s' host = '%s' port = %d",
!                  db_name, os_info.user, cluster->sockdir, cluster->port);
!     else
!         snprintf(conn_opts, sizeof(conn_opts),
!                  "dbname = '%s' user = '%s' port = %d",
!                  db_name, os_info.user, cluster->port);

      return PQconnectdb(conn_opts);
  }


  /*
+  * std_db_conn_opts()
+  *
+  * Return standard command-line options for connecting to this cluster when
+  * using psql, pg_dump, etc.  Ideally this would match what get_db_conn()
+  * sets, but the utilities we need aren't very consistent about the treatment
+  * of database name options, so we leave that out.
+  *
+  * Note result is in static storage, so use it right away.
+  */
+ char *
+ cluster_conn_opts(ClusterInfo *cluster)
+ {
+     static char    conn_opts[MAXPGPATH + NAMEDATALEN + 100];
+
+     if (cluster->sockdir)
+         snprintf(conn_opts, sizeof(conn_opts),
+                  "--host \"%s\" --port %d --username \"%s\"",
+                  cluster->sockdir, cluster->port, os_info.user);
+     else
+         snprintf(conn_opts, sizeof(conn_opts),
+                  "--port %d --username \"%s\"",
+                  cluster->port, os_info.user);
+
+     return conn_opts;
+ }
+
+
+ /*
   * executeQueryOrDie()
   *
   *    Formats a query string from the given arguments and executes the
*************** stop_postmaster_atexit(void)
*** 140,149 ****
  void
  start_postmaster(ClusterInfo *cluster)
  {
!     char        cmd[MAXPGPATH];
      PGconn       *conn;
      bool        exit_hook_registered = false;
      bool        pg_ctl_return = false;

      if (!exit_hook_registered)
      {
--- 173,183 ----
  void
  start_postmaster(ClusterInfo *cluster)
  {
!     char        cmd[MAXPGPATH * 4 + 1000];
      PGconn       *conn;
      bool        exit_hook_registered = false;
      bool        pg_ctl_return = false;
+     char        socket_string[MAXPGPATH + 200];

      if (!exit_hook_registered)
      {
*************** start_postmaster(ClusterInfo *cluster)
*** 151,156 ****
--- 185,207 ----
          exit_hook_registered = true;
      }

+     socket_string[0] = '\0';
+
+ #ifdef HAVE_UNIX_SOCKETS
+     /* prevent TCP/IP connections, restrict socket access */
+     strcat(socket_string,
+            " -c listen_addresses='' -c unix_socket_permissions=0700");
+
+     /* Have a sockdir?  Tell the postmaster. */
+     if (cluster->sockdir)
+         snprintf(socket_string + strlen(socket_string),
+                  sizeof(socket_string) - strlen(socket_string),
+                  " -c %s='%s'",
+                  (GET_MAJOR_VERSION(cluster->major_version) < 903) ?
+                  "unix_socket_directory" : "unix_socket_directories",
+                  cluster->sockdir);
+ #endif
+
      /*
       * Using autovacuum=off disables cleanup vacuum and analyze, but freeze
       * vacuums can still happen, so we set autovacuum_freeze_max_age to its
*************** start_postmaster(ClusterInfo *cluster)
*** 159,170 ****
       * not touch them.
       */
      snprintf(cmd, sizeof(cmd),
!              "\"%s/pg_ctl\" -w -l \"%s\" -D \"%s\" -o \"-p %d %s %s\" start",
            cluster->bindir, SERVER_LOG_FILE, cluster->pgconfig, cluster->port,
               (cluster->controldata.cat_ver >=
                BINARY_UPGRADE_SERVER_FLAG_CAT_VER) ? "-b" :
               "-c autovacuum=off -c autovacuum_freeze_max_age=2000000000",
!              cluster->pgopts ? cluster->pgopts : "");

      /*
       * Don't throw an error right away, let connecting throw the error because
--- 210,221 ----
       * not touch them.
       */
      snprintf(cmd, sizeof(cmd),
!              "\"%s/pg_ctl\" -w -l \"%s\" -D \"%s\" -o \"-p %d %s %s%s\" start",
            cluster->bindir, SERVER_LOG_FILE, cluster->pgconfig, cluster->port,
               (cluster->controldata.cat_ver >=
                BINARY_UPGRADE_SERVER_FLAG_CAT_VER) ? "-b" :
               "-c autovacuum=off -c autovacuum_freeze_max_age=2000000000",
!              cluster->pgopts ? cluster->pgopts : "", socket_string);

      /*
       * Don't throw an error right away, let connecting throw the error because