renaming pg_resetxlog to pg_resetwal has broken pg_upgrade.

Started by Jeff Janesalmost 9 years ago5 messages
#1Jeff Janes
jeff.janes@gmail.com

Upgrading from 9.6 to dev, I now get:

$ rm bisectdata -r ; bisect/bin/pg_ctl initdb -D bisectdata;
bisect/bin/pg_upgrade -b /usr/local/pgsql9_6/bin/ -B bisect/bin/ -d 96 -D
bisectdata/

check for "/usr/local/pgsql9_6/bin/pg_resetwal" failed: No such file or
directory

This looks somewhat complicated to fix. Should check_bin_dir test the old
cluster version, and make a deterministic check based on that? Or just
check for either spelling, and stash the successful result somewhere?

Culprit is here:

commit 85c11324cabaddcfaf3347df78555b30d27c5b5a
Author: Robert Haas <rhaas@postgresql.org>
Date: Thu Feb 9 16:23:46 2017 -0500

Rename user-facing tools with "xlog" in the name to say "wal".

This means pg_receivexlog because pg_receivewal, pg_resetxlog
becomes pg_resetwal, and pg_xlogdump becomes pg_waldump.

Cheers,

Jeff

#2Michael Paquier
michael.paquier@gmail.com
In reply to: Jeff Janes (#1)
1 attachment(s)
Re: renaming pg_resetxlog to pg_resetwal has broken pg_upgrade.

On Tue, Feb 14, 2017 at 9:09 AM, Jeff Janes <jeff.janes@gmail.com> wrote:

check for "/usr/local/pgsql9_6/bin/pg_resetwal" failed: No such file or
directory

This looks somewhat complicated to fix. Should check_bin_dir test the old
cluster version, and make a deterministic check based on that? Or just
check for either spelling, and stash the successful result somewhere?

The fix does not seem that complicated to me. get_bin_version() just
needs pg_ctl to be present, so we could move that in check_bin_dir()
after looking if pg_ctl is in a valid state, and reuse the version of
bin_version to see if the binary version is post-10 or not. Then the
decision making just depends on this value. Please see the patch
attached, this is passing 9.6->10 and check-world.

I have added as well an open item on the wiki.
--
Michael

Attachments:

pgupgrade-rename-fix.patchapplication/octet-stream; name=pgupgrade-rename-fix.patchDownload
diff --git a/src/bin/pg_upgrade/check.c b/src/bin/pg_upgrade/check.c
index ff1d7a207a..d52c81e249 100644
--- a/src/bin/pg_upgrade/check.c
+++ b/src/bin/pg_upgrade/check.c
@@ -26,7 +26,6 @@ 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_jsonb_9_4_usage(ClusterInfo *cluster);
 static void check_for_pg_role_prefix(ClusterInfo *cluster);
-static void get_bin_version(ClusterInfo *cluster);
 static char *get_canonical_locale_name(int category, const char *locale);
 
 
@@ -241,10 +240,6 @@ check_cluster_versions(void)
 	if (old_cluster.major_version > new_cluster.major_version)
 		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))
@@ -1080,34 +1075,6 @@ check_for_pg_role_prefix(ClusterInfo *cluster)
 	check_ok();
 }
 
-static void
-get_bin_version(ClusterInfo *cluster)
-{
-	char		cmd[MAXPGPATH],
-				cmd_output[MAX_STRING];
-	FILE	   *output;
-	int			pre_dot = 0,
-				post_dot = 0;
-
-	snprintf(cmd, sizeof(cmd), "\"%s/pg_ctl\" --version", cluster->bindir);
-
-	if ((output = popen(cmd, "r")) == NULL ||
-		fgets(cmd_output, sizeof(cmd_output), output) == NULL)
-		pg_fatal("could not get pg_ctl version data using %s: %s\n",
-				 cmd, strerror(errno));
-
-	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) < 1)
-		pg_fatal("could not get version from %s\n", cmd);
-
-	cluster->bin_version = (pre_dot * 100 + post_dot) * 100;
-}
-
 
 /*
  * get_canonical_locale_name
diff --git a/src/bin/pg_upgrade/controldata.c b/src/bin/pg_upgrade/controldata.c
index b075e26632..519256851c 100644
--- a/src/bin/pg_upgrade/controldata.c
+++ b/src/bin/pg_upgrade/controldata.c
@@ -70,6 +70,7 @@ get_control_data(ClusterInfo *cluster, bool live_check)
 	uint32		tli = 0;
 	uint32		logid = 0;
 	uint32		segno = 0;
+	char	   *resetwal_bin;
 
 
 	/*
@@ -111,9 +112,14 @@ get_control_data(ClusterInfo *cluster, bool live_check)
 	pg_putenv("LC_ALL", NULL);
 	pg_putenv("LC_MESSAGES", "C");
 
+	/* pg_resetxlog has been renamed to pg_resetwal in version 10 */
+	if (GET_MAJOR_VERSION(cluster->bin_version) < 1000)
+		resetwal_bin = "pg_resetxlog\" -n";
+	else
+		resetwal_bin = "pg_resetwal\" -n";
 	snprintf(cmd, sizeof(cmd), "\"%s/%s \"%s\"",
 			 cluster->bindir,
-			 live_check ? "pg_controldata\"" : "pg_resetwal\" -n",
+			 live_check ? "pg_controldata\"" : resetwal_bin,
 			 cluster->pgdata);
 	fflush(stdout);
 	fflush(stderr);
diff --git a/src/bin/pg_upgrade/exec.c b/src/bin/pg_upgrade/exec.c
index c74521f24a..b6a3ef791e 100644
--- a/src/bin/pg_upgrade/exec.c
+++ b/src/bin/pg_upgrade/exec.c
@@ -16,6 +16,7 @@
 
 static void check_data_dir(ClusterInfo *cluster);
 static void check_bin_dir(ClusterInfo *cluster);
+static void get_bin_version(ClusterInfo *cluster);
 static void validate_exec(const char *dir, const char *cmdName);
 
 #ifdef WIN32
@@ -24,6 +25,40 @@ static int	win32_check_directory_write_permissions(void);
 
 
 /*
+ * get_bin_version
+ *
+ *	Fetch versions of binaries for cluster.
+ */
+static void
+get_bin_version(ClusterInfo *cluster)
+{
+	char		cmd[MAXPGPATH],
+				cmd_output[MAX_STRING];
+	FILE	   *output;
+	int			pre_dot = 0,
+				post_dot = 0;
+
+	snprintf(cmd, sizeof(cmd), "\"%s/pg_ctl\" --version", cluster->bindir);
+
+	if ((output = popen(cmd, "r")) == NULL ||
+		fgets(cmd_output, sizeof(cmd_output), output) == NULL)
+		pg_fatal("could not get pg_ctl version data using %s: %s\n",
+				 cmd, strerror(errno));
+
+	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) < 1)
+		pg_fatal("could not get version from %s\n", cmd);
+
+	cluster->bin_version = (pre_dot * 100 + post_dot) * 100;
+}
+
+
+/*
  * exec_prog()
  *		Execute an external program with stdout/stderr redirected, and report
  *		errors
@@ -335,7 +370,20 @@ check_bin_dir(ClusterInfo *cluster)
 
 	validate_exec(cluster->bindir, "postgres");
 	validate_exec(cluster->bindir, "pg_ctl");
-	validate_exec(cluster->bindir, "pg_resetwal");
+
+	/*
+	 * Fetch the binary versions after checking for the existence of pg_ctl,
+	 * this gives a correct error if the binary used itself for the version
+	 * fetching is broken.
+	 */
+	get_bin_version(&old_cluster);
+	get_bin_version(&new_cluster);
+
+	/* pg_resetxlog has been renamed to pg_resetwal in version 10 */
+	if (GET_MAJOR_VERSION(cluster->bin_version) < 1000)
+		validate_exec(cluster->bindir, "pg_resetxlog");
+	else
+		validate_exec(cluster->bindir, "pg_resetwal");
 	if (cluster == &new_cluster)
 	{
 		/* these are only needed in the new cluster */
#3Jeff Janes
jeff.janes@gmail.com
In reply to: Michael Paquier (#2)
Re: renaming pg_resetxlog to pg_resetwal has broken pg_upgrade.

On Mon, Feb 13, 2017 at 6:19 PM, Michael Paquier <michael.paquier@gmail.com>
wrote:

On Tue, Feb 14, 2017 at 9:09 AM, Jeff Janes <jeff.janes@gmail.com> wrote:

check for "/usr/local/pgsql9_6/bin/pg_resetwal" failed: No such file or
directory

This looks somewhat complicated to fix. Should check_bin_dir test the

old

cluster version, and make a deterministic check based on that? Or just
check for either spelling, and stash the successful result somewhere?

The fix does not seem that complicated to me. get_bin_version() just
needs pg_ctl to be present, so we could move that in check_bin_dir()
after looking if pg_ctl is in a valid state, and reuse the version of
bin_version to see if the binary version is post-10 or not. Then the
decision making just depends on this value. Please see the patch
attached, this is passing 9.6->10 and check-world.

That fixes it for me.

I thought people would object to checking the version number in two
different places to make the same fundamental decision, and would want that
refactored somehow. But if you are OK with it, then I am.

Cheers,

Jeff

#4Michael Paquier
michael.paquier@gmail.com
In reply to: Jeff Janes (#3)
Re: renaming pg_resetxlog to pg_resetwal has broken pg_upgrade.

On Wed, Feb 15, 2017 at 3:55 AM, Jeff Janes <jeff.janes@gmail.com> wrote:

I thought people would object to checking the version number in two
different places to make the same fundamental decision, and would want that
refactored somehow. But if you are OK with it, then I am.

The binary versions are checked only once, which does not with change
HEAD. With this patch it happens just earlier, which makes the most
sense now that we have a condition depending on the version of what is
installed.
--
Michael

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#5Robert Haas
robertmhaas@gmail.com
In reply to: Michael Paquier (#4)
Re: renaming pg_resetxlog to pg_resetwal has broken pg_upgrade.

On Tue, Feb 14, 2017 at 5:13 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:

On Wed, Feb 15, 2017 at 3:55 AM, Jeff Janes <jeff.janes@gmail.com> wrote:

I thought people would object to checking the version number in two
different places to make the same fundamental decision, and would want that
refactored somehow. But if you are OK with it, then I am.

The binary versions are checked only once, which does not with change
HEAD. With this patch it happens just earlier, which makes the most
sense now that we have a condition depending on the version of what is
installed.

Thanks, Michael! Committed.

I actually thought about this problem when I committed the original
patch but decided it ought to be OK because I didn't see why we'd be
running pg_resetxlog on the old cluster. I didn't think about the
fact that we might be running it with the -n option. Oops.

Thanks Jeff for the report.

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers