Re: pg_upgrade version check improvements and small fixes - Mailing list pgsql-hackers

Dan McGee wrote:
> Not sure what the normal process is for patches, but I put together a
> few small patches for pg_upgrade after trying to use it earlier today
> and staring a non-helpful error message before I finally figured out
> what was going on.

Thanks for the detailed report and patches.  Let me address each one
individually.

> 0001 is just a simple typo fix, but didn't want to mix it in with the rest.

I applied this message capitalization fix to 9.0, 9.1, and master (9.2).

> 0002 moves a function around to be declared in the only place it is
> needed, and prevents a "sh: /oasdfpt/pgsql-8.4/bin/pg_config: No such
> file or directory" error message when you give it a bogus bindir.

You are right that I was calling pg_ctl before I had validated the bin
directory, and you were right that the function wasn't in an ideal
C file.

What I did was to move the function, and bundle all the
pg_upgrade_support test into that single function, so the function now
either errors out or returns void.

Patch attached and applied to 9.1 and master.  Good catch.

> 0003 is what I really wanted to solve, which was my failure with
> pg_upgrade. The call to pg_ctl didn't succeed because the binaries
> didn't match the data directory, thus resulting in this:
>
> $ pg_upgrade --check -d /tmp/olddata -D /tmp/newdata -b /usr/bin/ -B /usr/bin/
> Performing Consistency Checks
> -----------------------------
> Checking old data directory (/tmp/olddata)                  ok
> Checking old bin directory (/usr/bin)                       ok
> Checking new data directory (/tmp/newdata)                  ok
> Checking new bin directory (/usr/bin)                       ok
> pg_resetxlog: pg_control exists but is broken or unknown version; ignoring it
> Trying to start old server                                  .................ok
>
>  Unable to start old postmaster with the command: "/usr/bin/pg_ctl" -l
> "/dev/null" -D "/tmp/olddata" -o "-p 5432 -c autovacuum=off -c
> autovacuum_freeze_max_age=2000000000" start >> "/dev/null" 2>&1
> Perhaps pg_hba.conf was not set to "trust".
>
> The error had nothing to do with "trust" at all; it was simply that I
> tried to use 9.0 binaries with an 8.4 data directory. My patch checks
> for this and ensures that the -D bindir is the correct version, just
> as the -B datadir has to be the correct version.

I had not thought about testing if the bin and data directory were the
same major version, but you are right that it generates an odd error if
they are not.

I changed your sscanf format string because the version can be just
"9.2dev" and there is no need for the minor version.   I saw no reason
to test if the binary version matches the pg_upgrade version because we
already test the cluster version, and we test the cluster version is the
same as the binary version.

Patch attached and applied to 9.1 and master.

> I'm not on the mailing list nor do I have a lot of free time to keep
> up with normal development, but if there are quick things I can do to
> get these patches in let me know.

All done!

--
  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/check.c b/contrib/pg_upgrade/check.c
new file mode 100644
index 2b481da..377dea2
*** a/contrib/pg_upgrade/check.c
--- b/contrib/pg_upgrade/check.c
*************** static void check_is_super_user(ClusterI
*** 19,24 ****
--- 19,25 ----
  static void check_for_prepared_transactions(ClusterInfo *cluster);
  static void check_for_isn_and_int8_passing_mismatch(ClusterInfo *cluster);
  static void check_for_reg_data_type_usage(ClusterInfo *cluster);
+ static void check_for_support_lib(ClusterInfo *cluster);


  void
*************** check_cluster_versions(void)
*** 245,265 ****
  void
  check_cluster_compatibility(bool live_check)
  {
!     char        libfile[MAXPGPATH];
!     FILE       *lib_test;
!
!     /*
!      * Test pg_upgrade_support.so is in the proper place.     We cannot copy it
!      * ourselves because install directories are typically root-owned.
!      */
!     snprintf(libfile, sizeof(libfile), "%s/pg_upgrade_support%s", new_cluster.libpath,
!              DLSUFFIX);
!
!     if ((lib_test = fopen(libfile, "r")) == NULL)
!         pg_log(PG_FATAL,
!                "pg_upgrade_support%s must be created and installed in %s\n", DLSUFFIX, libfile);
!     else
!         fclose(lib_test);

      /* get/check pg_control data of servers */
      get_control_data(&old_cluster, live_check);
--- 246,252 ----
  void
  check_cluster_compatibility(bool live_check)
  {
!     check_for_support_lib(&new_cluster);

      /* get/check pg_control data of servers */
      get_control_data(&old_cluster, live_check);
*************** check_for_reg_data_type_usage(ClusterInf
*** 730,732 ****
--- 717,758 ----
      else
          check_ok();
  }
+
+
+ /*
+  * Test pg_upgrade_support.so is in the proper place.     We cannot copy it
+  * ourselves because install directories are typically root-owned.
+  */
+ static void
+ check_for_support_lib(ClusterInfo *cluster)
+ {
+     char        cmd[MAXPGPATH];
+     char        libdir[MAX_STRING];
+     char        libfile[MAXPGPATH];
+     FILE       *lib_test;
+     FILE       *output;
+
+     snprintf(cmd, sizeof(cmd), "\"%s/pg_config\" --pkglibdir", cluster->bindir);
+
+     if ((output = popen(cmd, "r")) == NULL)
+         pg_log(PG_FATAL, "Could not get pkglibdir data: %s\n",
+                getErrorText(errno));
+
+     fgets(libdir, sizeof(libdir), output);
+
+     pclose(output);
+
+     /* Remove trailing newline */
+     if (strchr(libdir, '\n') != NULL)
+         *strchr(libdir, '\n') = '\0';
+
+     snprintf(libfile, sizeof(libfile), "%s/pg_upgrade_support%s", libdir,
+              DLSUFFIX);
+
+     if ((lib_test = fopen(libfile, "r")) == NULL)
+         pg_log(PG_FATAL,
+                "The pg_upgrade_support module must be created and installed in the %s cluster.\n",
+                 CLUSTER_NAME(cluster));
+
+     fclose(lib_test);
+ }
diff --git a/contrib/pg_upgrade/option.c b/contrib/pg_upgrade/option.c
new file mode 100644
index 8153e30..18ce9d7
*** a/contrib/pg_upgrade/option.c
--- b/contrib/pg_upgrade/option.c
***************
*** 19,26 ****
  static void usage(void);
  static void validateDirectoryOption(char **dirpath,
                     char *envVarName, char *cmdLineOption, char *description);
- static void get_pkglibdirs(void);
- static char *get_pkglibdir(const char *bindir);


  UserOpts    user_opts;
--- 19,24 ----
*************** parseCommandLine(int argc, char *argv[])
*** 213,220 ****
                              "old cluster data resides");
      validateDirectoryOption(&new_cluster.pgdata, "NEWDATADIR", "-D",
                              "new cluster data resides");
-
-     get_pkglibdirs();
  }


--- 211,216 ----
*************** validateDirectoryOption(char **dirpath,
*** 314,357 ****
  #endif
          (*dirpath)[strlen(*dirpath) - 1] = 0;
  }
-
-
- static void
- get_pkglibdirs(void)
- {
-     /*
-      * we do not need to know the libpath in the old cluster, and might not
-      * have a working pg_config to ask for it anyway.
-      */
-     old_cluster.libpath = NULL;
-     new_cluster.libpath = get_pkglibdir(new_cluster.bindir);
- }
-
-
- static char *
- get_pkglibdir(const char *bindir)
- {
-     char        cmd[MAXPGPATH];
-     char        bufin[MAX_STRING];
-     FILE       *output;
-     int            i;
-
-     snprintf(cmd, sizeof(cmd), "\"%s/pg_config\" --pkglibdir", bindir);
-
-     if ((output = popen(cmd, "r")) == NULL)
-         pg_log(PG_FATAL, "Could not get pkglibdir data: %s\n",
-                getErrorText(errno));
-
-     fgets(bufin, sizeof(bufin), output);
-
-     if (output)
-         pclose(output);
-
-     /* Remove trailing newline */
-     i = strlen(bufin) - 1;
-
-     if (bufin[i] == '\n')
-         bufin[i] = '\0';
-
-     return pg_strdup(bufin);
- }
--- 310,312 ----
diff --git a/contrib/pg_upgrade/pg_upgrade.h b/contrib/pg_upgrade/pg_upgrade.h
new file mode 100644
index a3a0856..c27b58a
*** a/contrib/pg_upgrade/pg_upgrade.h
--- b/contrib/pg_upgrade/pg_upgrade.h
*************** typedef struct
*** 185,191 ****
      uint32        major_version;    /* PG_VERSION of cluster */
      char        major_version_str[64];    /* string PG_VERSION of cluster */
      Oid            pg_database_oid;    /* OID of pg_database relation */
-     char       *libpath;        /* pathname for cluster's pkglibdir */
      char       *tablespace_suffix;        /* directory specification */
  } ClusterInfo;

--- 185,190 ----
diff --git a/contrib/pg_upgrade/check.c b/contrib/pg_upgrade/check.c
new file mode 100644
index 1c3f589..1ee2aca
*** a/contrib/pg_upgrade/check.c
--- b/contrib/pg_upgrade/check.c
*************** static void check_for_prepared_transacti
*** 20,25 ****
--- 20,26 ----
  static void check_for_isn_and_int8_passing_mismatch(ClusterInfo *cluster);
  static void check_for_reg_data_type_usage(ClusterInfo *cluster);
  static void check_for_support_lib(ClusterInfo *cluster);
+ static void get_bin_version(ClusterInfo *cluster);


  void
*************** output_completion_banner(char *deletion_
*** 216,221 ****
--- 217,224 ----
  void
  check_cluster_versions(void)
  {
+     prep_status("Checking cluster versions");
+
      /* get old and new cluster versions */
      old_cluster.major_version = get_major_server_version(&old_cluster);
      new_cluster.major_version = get_major_server_version(&new_cluster);
*************** check_cluster_versions(void)
*** 235,244 ****

      /*
       * We can't allow downgrading because we use the target pg_dumpall, and
!      * pg_dumpall cannot operate on new datbase versions, only older versions.
       */
      if (old_cluster.major_version > new_cluster.major_version)
          pg_log(PG_FATAL, "This utility cannot be used to downgrade to older major PostgreSQL versions.\n");
  }


--- 238,263 ----

      /*
       * We can't allow downgrading because we use the target pg_dumpall, and
!      * pg_dumpall cannot operate on new database versions, only older versions.
       */
      if (old_cluster.major_version > new_cluster.major_version)
          pg_log(PG_FATAL, "This utility cannot be used to downgrade to older major PostgreSQL versions.\n");
+
+     /* get old and new binary versions */
+     get_bin_version(&old_cluster);
+     get_bin_version(&new_cluster);
+
+     /* Ensure binaries match the designated data directories */
+     if (GET_MAJOR_VERSION(old_cluster.major_version) !=
+         GET_MAJOR_VERSION(old_cluster.bin_version))
+         pg_log(PG_FATAL,
+                "Old cluster data and binary directories are from different major versions.\n");
+     if (GET_MAJOR_VERSION(new_cluster.major_version) !=
+         GET_MAJOR_VERSION(new_cluster.bin_version))
+         pg_log(PG_FATAL,
+                "New cluster data and binary directories are from different major versions.\n");
+
+     check_ok();
  }


*************** check_for_support_lib(ClusterInfo *clust
*** 754,756 ****
--- 773,804 ----

      fclose(lib_test);
  }
+
+
+ static void
+ get_bin_version(ClusterInfo *cluster)
+ {
+     char        cmd[MAXPGPATH], cmd_output[MAX_STRING];
+     FILE       *output;
+     int            pre_dot, post_dot;
+
+     snprintf(cmd, sizeof(cmd), "\"%s/pg_ctl\" --version", cluster->bindir);
+
+     if ((output = popen(cmd, "r")) == NULL)
+         pg_log(PG_FATAL, "Could not get pg_ctl version data: %s\n",
+                getErrorText(errno));
+
+     fgets(cmd_output, sizeof(cmd_output), output);
+
+     pclose(output);
+
+     /* Remove trailing newline */
+     if (strchr(cmd_output, '\n') != NULL)
+         *strchr(cmd_output, '\n') = '\0';
+
+     if (sscanf(cmd_output, "%*s %*s %d.%d", &pre_dot, &post_dot) != 2)
+         pg_log(PG_FATAL, "could not get version from %s\n", cmd);
+
+     cluster->bin_version = (pre_dot * 100 + post_dot) * 100;
+ }
+
diff --git a/contrib/pg_upgrade/pg_upgrade.h b/contrib/pg_upgrade/pg_upgrade.h
new file mode 100644
index c27b58a..613ddbd
*** a/contrib/pg_upgrade/pg_upgrade.h
--- b/contrib/pg_upgrade/pg_upgrade.h
*************** typedef struct
*** 184,189 ****
--- 184,190 ----
      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 */
+     uint32        bin_version;    /* version returned from pg_ctl */
      Oid            pg_database_oid;    /* OID of pg_database relation */
      char       *tablespace_suffix;        /* directory specification */
  } ClusterInfo;