pg_upgrade's exec_prog() coding improvement

diff --git a/contrib/pg_upgrade/check.c b/contrib/pg_upgrade/check.c
index aa896b5..4e22e88 100644
--- a/contrib/pg_upgrade/check.c
+++ b/contrib/pg_upgrade/check.c
@@ -183,13 +183,10 @@ issue_warnings(char *sequence_script_file_name)
 		if (sequence_script_file_name)
 		{
 			prep_status("Adjusting sequences");
-			exec_prog(true, true, UTILITY_LOG_FILE, NULL,
-					  SYSTEMQUOTE "\"%s/psql\" --echo-queries "
-					  "--set ON_ERROR_STOP=on "
-					  "--no-psqlrc --port %d --username \"%s\" "
-				   "-f \"%s\" --dbname template1 >> \"%s\" 2>&1" SYSTEMQUOTE,
+			exec_prog(UTILITY_LOG_FILE, 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, UTILITY_LOG_FILE);
+					  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 07a3b54..da3901d 100644
--- a/contrib/pg_upgrade/dump.c
+++ b/contrib/pg_upgrade/dump.c
@@ -23,12 +23,10 @@ generate_old_dump(void)
 	 * --binary-upgrade records the width of dropped columns in pg_class, and
 	 * restores the frozenid's for databases and relations.
 	 */
-	exec_prog(true, true, UTILITY_LOG_FILE, NULL,
-			  SYSTEMQUOTE "\"%s/pg_dumpall\" --port %d --username \"%s\" "
-			  "--schema-only --binary-upgrade %s > \"%s\" 2>> \"%s\""
-			  SYSTEMQUOTE, new_cluster.bindir, old_cluster.port, os_info.user,
-			  log_opts.verbose ? "--verbose" : "",
-			  ALL_DUMP_FILE, UTILITY_LOG_FILE);
+	exec_prog(UTILITY_LOG_FILE, 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();
 }
 
diff --git a/contrib/pg_upgrade/exec.c b/contrib/pg_upgrade/exec.c
index 6f993df..8b68794 100644
--- a/contrib/pg_upgrade/exec.c
+++ b/contrib/pg_upgrade/exec.c
@@ -15,6 +15,8 @@
 #include <unistd.h>
 #include <sys/types.h>
 
+static bool s_exec_prog(const char *log_file, const char *opt_log_file,
+			  bool throw_error, const char *fmt, va_list args);
 static void check_data_dir(const char *pg_data);
 static void check_bin_dir(ClusterInfo *cluster);
 static void validate_exec(const char *dir, const char *cmdName);
@@ -23,83 +25,118 @@ static void validate_exec(const char *dir, const char *cmdName);
 static int	win32_check_directory_write_permissions(void);
 #endif
 
+/*
+ * exec_prog_ext
+ * 		Execute an external program and report errors
+ *
+ *	Formats a command from the given argument list, logs it to the log file,
+ *	and attempts to execute that command.  If the command executes
+ *	successfully, exec_prog_ext() returns true.
+ *
+ *  If the command fails, an error message is saved to the specified log_file.
+ *  If throw_error is true, this raises a PG_FATAL error and pg_upgrade
+ *  terminates; otherwise it is just reported as PG_REPORT and exec_prog returns
+ *  false.
+ */
+bool
+exec_prog_ext(const char *log_file, const char *opt_log_file,
+			  bool throw_error, const char *cmd,...)
+{
+	va_list		ap;
+	bool		retval;
+
+	va_start(ap, cmd);
+	retval = s_exec_prog(log_file, opt_log_file, throw_error, cmd, ap);
+	va_end(ap);
+
+	return retval;
+}
 
 /*
  * exec_prog()
+ * 		Execute an external program with stdout/stderr redirected, and report
+ * 		errors
  *
- *	Formats a command from the given argument list and executes that
- *	command.  If the command executes, exec_prog() returns 1 otherwise
- *	exec_prog() logs an error message and returns 0.  Either way, the command
- *	line to be executed is saved to the specified log file.
+ * Formats a command from the given argument list, logs it to the log file,
+ * and attempts to execute that command.  If the command executes
+ * successfully, exec_prog() returns.
  *
- *	If throw_error is TRUE, this function will throw a PG_FATAL error
- *	instead of returning should an error occur.  The command it appended
- *	to log_file;  opt_log_file is used in error messages.
+ * If the command fails, an error message is saved to the specified log_file.
+ * If throw_error is true, this raises a PG_FATAL error and pg_upgrade
+ * terminates; otherwise it is just reported as PG_REPORT.
  */
-int
-exec_prog(bool throw_error, bool is_priv, const char *log_file,
-		  const char *opt_log_file, const char *fmt,...)
+void
+exec_prog(const char *log_file, bool throw_error, const char *cmd,...)
+{
+	va_list		ap;
+
+	va_start(ap, cmd);
+	s_exec_prog(log_file, NULL, throw_error, cmd, ap);
+	va_end(ap);
+}
+
+/*
+ * s_exec_prog
+ * 		va_list version of the above
+ */
+static bool
+s_exec_prog(const char *log_file, const char *opt_log_file,
+  			bool throw_error, const char *fmt, va_list args)
 {
-	va_list		args;
 	int			result;
-	int			retval;
-	char		cmd[MAXPGPATH];
+	int			written;
+#define MAXCMDLEN (2 * MAXPGPATH)
+	char		cmd[MAXCMDLEN];
 	mode_t		old_umask = 0;
 	FILE	   *log;
 
-	if (is_priv)
-		old_umask = umask(S_IRWXG | S_IRWXO);
+	old_umask = umask(S_IRWXG | S_IRWXO);
 
-	va_start(args, fmt);
-	vsnprintf(cmd, MAXPGPATH, fmt, args);
-	va_end(args);
+	written = strlcat(cmd, SYSTEMQUOTE, strlen(SYSTEMQUOTE));
+	written += vsnprintf(cmd + written, MAXCMDLEN - written, fmt, args);
+	snprintf(cmd + written, MAXCMDLEN - written,
+			 " >> \"%s\" 2>&1" SYSTEMQUOTE, log_file);
 
 	if ((log = fopen_priv(log_file, "a+")) == NULL)
 		pg_log(PG_FATAL, "cannot write to log file %s\n", log_file);
 	pg_log(PG_VERBOSE, "%s\n", cmd);
 	fprintf(log, "command: %s\n", cmd);
+
 	/*
-	 *	In Windows, we must close then reopen the log file so the file is
-	 *	not open while the command is running, or we get a share violation.
+	 * In Windows, we must close the log file at this point so the file is not
+	 * open while the command is running, or we get a share violation.
 	 */
 	fclose(log);
 
 	result = system(cmd);
 
-	if (is_priv)
-		umask(old_umask);
+	umask(old_umask);
 
 	if (result != 0)
 	{
-		char opt_string[MAXPGPATH];
-
-		/* Create string for optional second log file */
-		if (opt_log_file)
-			snprintf(opt_string, sizeof(opt_string), " or \"%s\"", opt_log_file);
-		else
-			opt_string[0] = '\0';
-
 		report_status(PG_REPORT, "*failure*");
 		fflush(stdout);
 		pg_log(PG_VERBOSE, "There were problems executing \"%s\"\n", cmd);
-		pg_log(throw_error ? PG_FATAL : PG_REPORT,
-			   "Consult the last few lines of \"%s\"%s for\n"
-			   "the probable cause of the failure.\n",
-			   log_file, opt_string);
-		retval = 1;
+		if (opt_log_file)
+			pg_log(throw_error ? PG_FATAL : PG_REPORT,
+				   "Consult the last few lines of \"%s\" or \"%s\" for\n"
+				   "the probable cause of the failure.\n",
+				   log_file, opt_log_file);
+		else
+			pg_log(throw_error ? PG_FATAL : PG_REPORT,
+				   "Consult the last few lines of \"%s\" for\n"
+				   "the probable cause of the failure.\n",
+				   log_file);
 	}
-	else
-		retval = 0;
 
 	if ((log = fopen_priv(log_file, "a+")) == NULL)
 		pg_log(PG_FATAL, "cannot write to log file %s\n", log_file);
 	fprintf(log, "\n\n");
 	fclose(log);
 
-	return retval;
+	return result == 0;
 }
 
-
 /*
  * is_server_running()
  *
@@ -128,7 +165,6 @@ is_server_running(const char *datadir)
 	return true;
 }
 
-
 /*
  * verify_directories()
  *
diff --git a/contrib/pg_upgrade/file.c b/contrib/pg_upgrade/file.c
index 5509670..a5d92c6 100644
--- a/contrib/pg_upgrade/file.c
+++ b/contrib/pg_upgrade/file.c
@@ -103,10 +103,10 @@ copyAndUpdateFile(pageCnvCtx *pageConverter,
 /*
  * linkAndUpdateFile()
  *
- * Creates a symbolic link between the given relation files. We use
+ * Creates a hard link between the given relation files. We use
  * this function to perform a true in-place update. If the on-disk
  * format of the new cluster is bit-for-bit compatible with the on-disk
- * format of the old cluster, we can simply symlink each relation
+ * format of the old cluster, we can simply link each relation
  * instead of copying the data from the old cluster to the new cluster.
  */
 const char *
diff --git a/contrib/pg_upgrade/pg_upgrade.c b/contrib/pg_upgrade/pg_upgrade.c
index eff1a08..a618f30 100644
--- a/contrib/pg_upgrade/pg_upgrade.c
+++ b/contrib/pg_upgrade/pg_upgrade.c
@@ -140,11 +140,10 @@ main(int argc, char **argv)
 	 * because there is no need to have the schema load use new oids.
 	 */
 	prep_status("Setting next OID for new cluster");
-	exec_prog(true, true, UTILITY_LOG_FILE, NULL,
-			  SYSTEMQUOTE "\"%s/pg_resetxlog\" -o %u \"%s\" >> \"%s\" 2>&1"
-			  SYSTEMQUOTE,
+	exec_prog(UTILITY_LOG_FILE, true,
+			  "\"%s/pg_resetxlog\" -o %u \"%s\"",
 			  new_cluster.bindir, old_cluster.controldata.chkpnt_nxtoid,
-			  new_cluster.pgdata, UTILITY_LOG_FILE);
+			  new_cluster.pgdata);
 	check_ok();
 
 	create_script_for_cluster_analyze(&analyze_script_file_name);
@@ -211,11 +210,10 @@ prepare_new_cluster(void)
 	 * --analyze so autovacuum doesn't update statistics later
 	 */
 	prep_status("Analyzing all rows in the new cluster");
-	exec_prog(true, true, UTILITY_LOG_FILE, NULL,
-			  SYSTEMQUOTE "\"%s/vacuumdb\" --port %d --username \"%s\" "
-			  "--all --analyze %s >> \"%s\" 2>&1" SYSTEMQUOTE,
+	exec_prog(UTILITY_LOG_FILE, true,
+			  "\"%s/vacuumdb\" --port %d --username \"%s\" --all --analyze %s",
 			  new_cluster.bindir, new_cluster.port, os_info.user,
-			  log_opts.verbose ? "--verbose" : "", UTILITY_LOG_FILE);
+			  log_opts.verbose ? "--verbose" : "");
 	check_ok();
 
 	/*
@@ -225,11 +223,10 @@ prepare_new_cluster(void)
 	 * later.
 	 */
 	prep_status("Freezing all rows on the new cluster");
-	exec_prog(true, true, UTILITY_LOG_FILE, NULL,
-			  SYSTEMQUOTE "\"%s/vacuumdb\" --port %d --username \"%s\" "
-			  "--all --freeze %s >> \"%s\" 2>&1" SYSTEMQUOTE,
+	exec_prog(UTILITY_LOG_FILE, true,
+			  "\"%s/vacuumdb\" --port %d --username \"%s\" --all --freeze %s",
 			  new_cluster.bindir, new_cluster.port, os_info.user,
-			  log_opts.verbose ? "--verbose" : "", UTILITY_LOG_FILE);
+			  log_opts.verbose ? "--verbose" : "");
 	check_ok();
 
 	get_pg_database_relfilenode(&new_cluster);
@@ -263,14 +260,10 @@ prepare_new_databases(void)
 	 * support functions in template1 but pg_dumpall creates database using
 	 * the template0 template.
 	 */
-	exec_prog(true, true, RESTORE_LOG_FILE, NULL,
-			  SYSTEMQUOTE "\"%s/psql\" --echo-queries "
-			  "--set ON_ERROR_STOP=on "
-	/* --no-psqlrc prevents AUTOCOMMIT=off */
-			  "--no-psqlrc --port %d --username \"%s\" "
-			  "-f \"%s\" --dbname template1 >> \"%s\" 2>&1" SYSTEMQUOTE,
+	exec_prog(RESTORE_LOG_FILE, true,
+			  "\"%s/psql\" " EXEC_PSQL_ARGS " --port %d --username \"%s\" -f \"%s\"",
 			  new_cluster.bindir, new_cluster.port, os_info.user,
-			  GLOBALS_DUMP_FILE, RESTORE_LOG_FILE);
+			  GLOBALS_DUMP_FILE);
 	check_ok();
 
 	/* we load this to get a current list of databases */
@@ -296,13 +289,10 @@ create_new_objects(void)
 	check_ok();
 
 	prep_status("Restoring database schema to new cluster");
-	exec_prog(true, true, RESTORE_LOG_FILE, NULL,
-			  SYSTEMQUOTE "\"%s/psql\" --echo-queries "
-			  "--set ON_ERROR_STOP=on "
-			  "--no-psqlrc --port %d --username \"%s\" "
-			  "-f \"%s\" --dbname template1 >> \"%s\" 2>&1" SYSTEMQUOTE,
+	exec_prog(RESTORE_LOG_FILE, true,
+			  "\"%s/psql\" " EXEC_PSQL_ARGS " --port %d --username \"%s\" -f \"%s\"",
 			  new_cluster.bindir, new_cluster.port, os_info.user,
-			  DB_DUMP_FILE, RESTORE_LOG_FILE);
+			  DB_DUMP_FILE);
 	check_ok();
 
 	/* regenerate now that we have objects in the databases */
@@ -331,16 +321,14 @@ copy_subdir_files(char *subdir)
 
 	prep_status("Copying old %s to new server", subdir);
 
-	exec_prog(true, false, UTILITY_LOG_FILE, NULL,
+	exec_prog(UTILITY_LOG_FILE, true,
 #ifndef WIN32
-			  SYSTEMQUOTE "%s \"%s\" \"%s\" >> \"%s\" 2>&1" SYSTEMQUOTE,
-			  "cp -Rf",
+			  "cp -Rf \"%s\" \"%s\"",
 #else
 	/* flags: everything, no confirm, quiet, overwrite read-only */
-			  SYSTEMQUOTE "%s \"%s\" \"%s\\\" >> \"%s\" 2>&1" SYSTEMQUOTE,
-			  "xcopy /e /y /q /r",
+			  "xcopy /e /y /q /r \"%s\" \"%s\\\"",
 #endif
-			  old_path, new_path, UTILITY_LOG_FILE);
+			  old_path, new_path);
 
 	check_ok();
 }
@@ -353,22 +341,18 @@ copy_clog_xlog_xid(void)
 
 	/* set the next transaction id of the new cluster */
 	prep_status("Setting next transaction ID for new cluster");
-	exec_prog(true, true, UTILITY_LOG_FILE, NULL,
-			  SYSTEMQUOTE
-			  "\"%s/pg_resetxlog\" -f -x %u \"%s\" >> \"%s\" 2>&1"
-			  SYSTEMQUOTE, new_cluster.bindir,
-			  old_cluster.controldata.chkpnt_nxtxid,
-			  new_cluster.pgdata, UTILITY_LOG_FILE);
+	exec_prog(UTILITY_LOG_FILE, true,
+			  "\"%s/pg_resetxlog\" -f -x %u \"%s\"",
+			  new_cluster.bindir, old_cluster.controldata.chkpnt_nxtxid,
+			  new_cluster.pgdata);
 	check_ok();
 
 	/* now reset the wal archives in the new cluster */
 	prep_status("Resetting WAL archives");
-	exec_prog(true, true, UTILITY_LOG_FILE, NULL,
-			  SYSTEMQUOTE
-			  "\"%s/pg_resetxlog\" -l %s \"%s\" >> \"%s\" 2>&1"
-			  SYSTEMQUOTE, new_cluster.bindir,
+	exec_prog(UTILITY_LOG_FILE, true,
+			  "\"%s/pg_resetxlog\" -l %s \"%s\"", new_cluster.bindir,
 			  old_cluster.controldata.nextxlogfile,
-			  new_cluster.pgdata, UTILITY_LOG_FILE);
+			  new_cluster.pgdata);
 	check_ok();
 }
 
diff --git a/contrib/pg_upgrade/pg_upgrade.h b/contrib/pg_upgrade/pg_upgrade.h
index affee7a..cc8ae4e 100644
--- a/contrib/pg_upgrade/pg_upgrade.h
+++ b/contrib/pg_upgrade/pg_upgrade.h
@@ -316,10 +316,15 @@ void		split_old_dump(void);
 
 /* exec.c */
 
-int
-exec_prog(bool throw_error, bool is_priv, const char *log_file,
-		  const char *opt_log_file, const char *cmd,...)
-__attribute__((format(PG_PRINTF_ATTRIBUTE, 5, 6)));
+#define EXEC_PSQL_ARGS "--echo-queries --set ON_ERROR_STOP=on --no-psqlrc --dbname=template1"
+void
+exec_prog(const char *log_file, bool throw_error, const char *cmd,...)
+__attribute__((format(PG_PRINTF_ATTRIBUTE, 3, 4)));
+bool
+exec_prog_ext(const char *log_file, const char *opt_log_file,
+			  bool throw_error,
+			  const char *fmt,...)
+__attribute__((format(PG_PRINTF_ATTRIBUTE, 4, 5)));
 void		verify_directories(void);
 bool		is_server_running(const char *datadir);
 
diff --git a/contrib/pg_upgrade/server.c b/contrib/pg_upgrade/server.c
index e94a897..3d028cd 100644
--- a/contrib/pg_upgrade/server.c
+++ b/contrib/pg_upgrade/server.c
@@ -143,7 +143,7 @@ start_postmaster(ClusterInfo *cluster)
 	char		cmd[MAXPGPATH];
 	PGconn	   *conn;
 	bool		exit_hook_registered = false;
-	int			pg_ctl_return = 0;
+	bool		pg_ctl_return = false;
 
 	if (!exit_hook_registered)
 	{
@@ -159,23 +159,24 @@ start_postmaster(ClusterInfo *cluster)
 	 * not touch them.
 	 */
 	snprintf(cmd, sizeof(cmd),
-			 SYSTEMQUOTE "\"%s/pg_ctl\" -w -l \"%s\" -D \"%s\" "
-			 "-o \"-p %d %s %s\" start >> \"%s\" 2>&1" SYSTEMQUOTE,
+			 "\"%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 : "", SERVER_START_LOG_FILE);
+			 cluster->pgopts ? cluster->pgopts : "");
 
 	/*
 	 * 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, true, SERVER_START_LOG_FILE,
-	/* pass both file names if the differ */
-					  (strcmp(SERVER_LOG_FILE, SERVER_START_LOG_FILE) != 0) ?
-							  SERVER_LOG_FILE : NULL,
-							  "%s", cmd);
+	pg_ctl_return = exec_prog_ext(SERVER_START_LOG_FILE,
+								  /* pass both file names if the differ */
+								  (strcmp(SERVER_LOG_FILE,
+										  SERVER_START_LOG_FILE) != 0) ?
+								  SERVER_LOG_FILE : NULL,
+								  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 ||
@@ -191,7 +192,7 @@ start_postmaster(ClusterInfo *cluster)
 	PQfinish(conn);
 
 	/* If the connection didn't fail, fail now */
-	if (pg_ctl_return != 0)
+	if (!pg_ctl_return)
 		pg_log(PG_FATAL, "pg_ctl failed to start the %s server, or connection failed\n",
 			   CLUSTER_NAME(cluster));
 
@@ -202,7 +203,6 @@ start_postmaster(ClusterInfo *cluster)
 void
 stop_postmaster(bool fast)
 {
-	char		cmd[MAXPGPATH];
 	ClusterInfo *cluster;
 
 	if (os_info.running_cluster == &old_cluster)
@@ -212,14 +212,11 @@ stop_postmaster(bool fast)
 	else
 		return;					/* no cluster running */
 
-	snprintf(cmd, sizeof(cmd),
-			 SYSTEMQUOTE "\"%s/pg_ctl\" -w -D \"%s\" -o \"%s\" "
-			 "%s stop >> \"%s\" 2>&1" SYSTEMQUOTE,
-			 cluster->bindir, cluster->pgconfig,
-			 cluster->pgopts ? cluster->pgopts : "",
-			 fast ? "-m fast" : "", SERVER_STOP_LOG_FILE);
-
-	exec_prog(fast ? false : true, true, SERVER_STOP_LOG_FILE, NULL, "%s", cmd);
+	exec_prog(SERVER_STOP_LOG_FILE, !fast,
+			  "\"%s/pg_ctl\" -w -D \"%s\" -o \"%s\" %s stop",
+			  cluster->bindir, cluster->pgconfig,
+			  cluster->pgopts ? cluster->pgopts : "",
+			  fast ? "-m fast" : "");
 
 	os_info.running_cluster = NULL;
 }