Fix order of bin and data dir checks in pg_upgrade

If you called pg_upgrade with a --new-datadir that points to a
configuration-only dir and no --new-bindir option, it would try to
execute "postgres -C data_directory" using the new binary before
resolving the correct path to it.

To fix, check and resolve the binary dirs first, before calling
adjust_data_dir().  For clarity, rename the setup() function to
check_clusters_stopped() to better reflect what it does now.

One consequence of this is that if the binaries are not found, you now
get an error earlier than before, before the pg_upgrade_output.d
directory and the log files have been created.  That's what
necessitated the test change in this commit.  The log files are
important for things like listing unsupported objects in the source
database, but missing binaries feels like a more permanent error,
similar to invalid options or incorrect directory permissions, and it
seems OK to just print the error to stderr for those.

This has been broken forever, but I chose to not backpatch since no
one else has complained, it's a pretty minor issue, and it's easy to
work around by passing the binary dir explicitly.  Maybe later, if
someone in the field runs into this.

Reviewed-by: Chao Li <li.evan.chao@gmail.com>
Reviewed-by: Bohyun Lee <bohyun.lee@databricks.com>
Discussion: https://www.postgresql.org/message-id/6894f355-10e4-4756-89ad-ae5987f416a6@iki.fi

src/bin/pg_upgrade/exec.c

@@ -15,8 +15,6 @@
 #include "fe_utils/version.h"
 #include "pg_upgrade.h"
 
-static void check_data_dir(ClusterInfo *cluster);
-static void check_bin_dir(ClusterInfo *cluster, bool check_versions);
 static void get_bin_version(ClusterInfo *cluster);
 static void check_exec(const char *dir, const char *program, bool check_version);
 
@@ -244,15 +242,10 @@ pid_lock_file_exists(const char *datadir)
 
 
 /*
- * verify_directories()
- *
- * does all the hectic work of verifying directories and executables
- * of old and new server.
- *
- * NOTE: May update the values of all parameters
+ * Check that the current directory is writable
  */
 void
-verify_directories(void)
+check_current_dir_write_permissions(void)
 {
 #ifndef WIN32
 	if (access(".", R_OK | W_OK | X_OK) != 0)
@@ -260,14 +253,8 @@ verify_directories(void)
 	if (win32_check_directory_write_permissions() != 0)
 #endif
 		pg_fatal("You must have read and write access in the current directory.");
-
-	check_bin_dir(&old_cluster, false);
-	check_data_dir(&old_cluster);
-	check_bin_dir(&new_cluster, true);
-	check_data_dir(&new_cluster);
 }
 
-
 #ifdef WIN32
 /*
  * win32_check_directory_write_permissions()
@@ -329,7 +316,7 @@ check_single_dir(const char *pg_data, const char *subdir)
  *	us) we display an error message and exit()
  *
  */
-static void
+void
 check_data_dir(ClusterInfo *cluster)
 {
 	const char *pg_data = cluster->pgdata;
@@ -361,7 +348,7 @@ check_data_dir(ClusterInfo *cluster)
  *	against the version of this pg_upgrade.  This is for checking the target
  *	bindir.
  */
-static void
+void
 check_bin_dir(ClusterInfo *cluster, bool check_versions)
 {
 	struct stat statBuf;

src/bin/pg_upgrade/option.c

@@ -422,9 +422,10 @@ check_required_directory(char **dirpath, const char *envVarName, bool useCwd,
  * by querying the running server.  This has limited checking because we
  * can't check for a running server because we can't find postmaster.pid.
  *
- * On entry, cluster->pgdata has been set from command line or env variable,
- * but cluster->pgconfig isn't set.  We fill both variables with corrected
- * values.
+ * On entry, cluster->bindir has already been set, and cluster->pgdata has
+ * been initially set from the command line or env variable.  We fill
+ * cluster->pgconfig, and adjust cluster->pgdata with a corrected value if the
+ * real data directory is in a different location.
  */
 void
 adjust_data_dir(ClusterInfo *cluster)

src/bin/pg_upgrade/pg_upgrade.c

@@ -66,7 +66,8 @@ static void create_new_objects(void);
 static void copy_xact_xlog_xid(void);
 static void set_frozenxids(void);
 static void make_outputdirs(char *pgdata);
-static void setup(char *argv0);
+static void check_bin_dirs(char *argv0);
+static void check_clusters_stopped(void);
 static void create_logical_replication_slots(void);
 static void create_conflict_detection_slot(void);
 
@@ -106,6 +107,12 @@ main(int argc, char **argv)
 
 	get_restricted_token();
 
+	/*
+	 * Resolve old and new binary dirs before adjust_data_dir(), because
+	 * adjust_data_dir() might need to call the binaries.
+	 */
+	check_bin_dirs(argv[0]);
+
 	adjust_data_dir(&old_cluster);
 	adjust_data_dir(&new_cluster);
 
@@ -125,7 +132,11 @@ main(int argc, char **argv)
 	 */
 	make_outputdirs(new_cluster.pgdata);
 
-	setup(argv[0]);
+	check_current_dir_write_permissions();
+
+	check_data_dir(&old_cluster);
+	check_data_dir(&new_cluster);
+	check_clusters_stopped();
 
 	output_check_banner();
 
@@ -359,14 +370,8 @@ make_outputdirs(char *pgdata)
 
 
 static void
-setup(char *argv0)
+check_bin_dirs(char *argv0)
 {
-	/*
-	 * 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();
-
 	/*
 	 * In case the user hasn't specified the directory for the new binaries
 	 * with -B, default to using the path of the currently executed pg_upgrade
@@ -384,7 +389,18 @@ setup(char *argv0)
 		new_cluster.bindir = pg_strdup(exec_path);
 	}
 
-	verify_directories();
+	check_bin_dir(&old_cluster, false);
+	check_bin_dir(&new_cluster, true);
+}
+
+static void
+check_clusters_stopped(void)
+{
+	/*
+	 * 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();
 
 	/* no postmasters should be running, except for a live check */
 	if (pid_lock_file_exists(old_cluster.pgdata))

src/bin/pg_upgrade/pg_upgrade.h

@@ -391,7 +391,9 @@ void		generate_old_dump(void);
 
 bool		exec_prog(const char *log_filename, const char *opt_log_file,
 					  bool report_error, bool exit_on_error, const char *fmt, ...) pg_attribute_printf(5, 6);
-void		verify_directories(void);
+void		check_current_dir_write_permissions(void);
+void		check_bin_dir(ClusterInfo *cluster, bool check_versions);
+void		check_data_dir(ClusterInfo *cluster);
 bool		pid_lock_file_exists(const char *datadir);
 
 

src/bin/pg_upgrade/t/002_pg_upgrade.pl

@@ -561,35 +561,12 @@ $oldnode->safe_psql(
 # Upgrade the instance.
 $oldnode->stop;
 
-# Cause a failure at the start of pg_upgrade, this should create the logging
-# directory pg_upgrade_output.d but leave it around.  Keep --check for an
-# early exit.
-command_checks_all(
-	[
-		'pg_upgrade', '--no-sync',
-		'--old-datadir' => $oldnode->data_dir,
-		'--new-datadir' => $newnode->data_dir,
-		'--old-bindir' => $oldbindir . '/does/not/exist/',
-		'--new-bindir' => $newbindir,
-		'--socketdir' => $newnode->host,
-		'--old-port' => $oldnode->port,
-		'--new-port' => $newnode->port,
-		$mode, '--check',
-	],
-	1,
-	[qr{check for ".*?does/not/exist" failed}],
-	[],
-	'run of pg_upgrade --check for new instance with incorrect binary path');
-ok(-d $newnode->data_dir . "/pg_upgrade_output.d",
-	"pg_upgrade_output.d/ not removed after pg_upgrade failure");
-rmtree($newnode->data_dir . "/pg_upgrade_output.d");
-
 # Check that pg_upgrade aborts when encountering an invalid database
 # (However, versions that were out of support by commit c66a7d75e652 don't
 # know how to do this, so skip this test there.)
 SKIP:
 {
-	skip "database invalidation not implemented", 1
+	skip "database invalidation not implemented", 4
 	  if $oldnode->pg_version < 11;
 
 	command_checks_all(
@@ -608,6 +585,12 @@ SKIP:
 		[qr/datconnlimit/],
 		[qr/^$/],
 		'invalid database causes failure');
+
+	# Check that the failure creates the logging directory pg_upgrade_output.d
+	# and leaves it around
+	ok(-d $newnode->data_dir . "/pg_upgrade_output.d",
+		"pg_upgrade_output.d/ not removed after pg_upgrade failure");
+
 	rmtree($newnode->data_dir . "/pg_upgrade_output.d");
 }