Skip to content

P
Postgres FD Implementation
Commit 088c065c authored by Alvaro Herrera's avatar Alvaro Herrera
Browse files
Options

pg_upgrade: Fix exec_prog API to be less flaky 

The previous signature made it very easy to pass something other than
the printf-format specifier in the corresponding position, without any
warning from the compiler.

While at it, move some of the escaping, redirecting and quoting
responsibilities from the callers into exec_prog() itself.  This makes
the callsites cleaner.
parent 34c02044
Show whitespace changes
Inline Side-by-side
Showing with 96 additions and 113 deletions
contrib/pg_upgrade/check.c
View file @ 088c065c
...@@ -183,13 +183,10 @@ issue_warnings(char *sequence_script_file_name) ...@@ -183,13 +183,10 @@ issue_warnings(char *sequence_script_file_name)
if (sequence_script_file_name) if (sequence_script_file_name)
{ {
prep_status("Adjusting sequences"); prep_status("Adjusting sequences");
exec_prog(true, true, UTILITY_LOG_FILE, NULL, exec_prog(UTILITY_LOG_FILE, NULL, true,
SYSTEMQUOTE "\"%s/psql\" --echo-queries " "\"%s/psql\" " EXEC_PSQL_ARGS " --port %d --username \"%s\" -f \"%s\"",
"--set ON_ERROR_STOP=on "
"--no-psqlrc --port %d --username \"%s\" "
"-f \"%s\" --dbname template1 >> \"%s\" 2>&1" SYSTEMQUOTE,
new_cluster.bindir, new_cluster.port, os_info.user, 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); unlink(sequence_script_file_name);
check_ok(); check_ok();
} }
......
contrib/pg_upgrade/dump.c
View file @ 088c065c
...@@ -23,12 +23,11 @@ generate_old_dump(void) ...@@ -23,12 +23,11 @@ generate_old_dump(void)
* --binary-upgrade records the width of dropped columns in pg_class, and * --binary-upgrade records the width of dropped columns in pg_class, and
* restores the frozenid's for databases and relations. * restores the frozenid's for databases and relations.
*/ */
exec_prog(true, true, UTILITY_LOG_FILE, NULL, exec_prog(UTILITY_LOG_FILE, NULL, true,
SYSTEMQUOTE "\"%s/pg_dumpall\" --port %d --username \"%s\" " "\"%s/pg_dumpall\" --port %d --username \"%s\" --schema-only --binary-upgrade %s -f %s",
"--schema-only --binary-upgrade %s > \"%s\" 2>> \"%s\"" new_cluster.bindir, old_cluster.port, os_info.user,
SYSTEMQUOTE, new_cluster.bindir, old_cluster.port, os_info.user,
log_opts.verbose ? "--verbose" : "", log_opts.verbose ? "--verbose" : "",
ALL_DUMP_FILE, UTILITY_LOG_FILE); ALL_DUMP_FILE);
check_ok(); check_ok();
} }
......
contrib/pg_upgrade/exec.c
View file @ 088c065c
...@@ -26,77 +26,81 @@ static int win32_check_directory_write_permissions(void); ...@@ -26,77 +26,81 @@ static int win32_check_directory_write_permissions(void);
/* /*
* exec_prog() * exec_prog()
* Execute an external program with stdout/stderr redirected, and report
* errors
* *
* Formats a command from the given argument list and executes that * Formats a command from the given argument list, logs it to the log file,
* command. If the command executes, exec_prog() returns 1 otherwise * and attempts to execute that command. If the command executes
* exec_prog() logs an error message and returns 0. Either way, the command * successfully, exec_prog() returns true.
* line to be executed is saved to the specified log file.
* *
* If throw_error is TRUE, this function will throw a PG_FATAL error * If the command fails, an error message is saved to the specified log_file.
* instead of returning should an error occur. The command it appended * If throw_error is true, this raises a PG_FATAL error and pg_upgrade
* to log_file; opt_log_file is used in error messages. * terminates; otherwise it is just reported as PG_REPORT and exec_prog()
* returns false.
*/ */
int bool
exec_prog(bool throw_error, bool is_priv, const char *log_file, exec_prog(const char *log_file, const char *opt_log_file,
const char *opt_log_file, const char *fmt,...) bool throw_error, const char *fmt,...)
{ {
va_list args;
int result; int result;
int retval; int written;
char cmd[MAXPGPATH]; #define MAXCMDLEN (2 * MAXPGPATH)
char cmd[MAXCMDLEN];
mode_t old_umask = 0; mode_t old_umask = 0;
FILE *log; FILE *log;
va_list ap;
if (is_priv)
old_umask = umask(S_IRWXG | S_IRWXO); old_umask = umask(S_IRWXG | S_IRWXO);
va_start(args, fmt); written = strlcpy(cmd, SYSTEMQUOTE, strlen(SYSTEMQUOTE));
vsnprintf(cmd, MAXPGPATH, fmt, args); va_start(ap, fmt);
va_end(args); written += vsnprintf(cmd + written, MAXCMDLEN - written, fmt, ap);
va_end(ap);
if (written >= MAXCMDLEN)
pg_log(PG_FATAL, "command too long\n");
written += snprintf(cmd + written, MAXCMDLEN - written,
" >> \"%s\" 2>&1" SYSTEMQUOTE, log_file);
if (written >= MAXCMDLEN)
pg_log(PG_FATAL, "command too long\n");
if ((log = fopen_priv(log_file, "a+")) == NULL) if ((log = fopen_priv(log_file, "a+")) == NULL)
pg_log(PG_FATAL, "cannot write to log file %s\n", log_file); pg_log(PG_FATAL, "cannot write to log file %s\n", log_file);
pg_log(PG_VERBOSE, "%s\n", cmd); pg_log(PG_VERBOSE, "%s\n", cmd);
fprintf(log, "command: %s\n", cmd); fprintf(log, "command: %s\n", cmd);
/* /*
* In Windows, we must close then reopen the log file so the file is * In Windows, we must close the log file at this point so the file is not
* not open while the command is running, or we get a share violation. * open while the command is running, or we get a share violation.
*/ */
fclose(log); fclose(log);
result = system(cmd); result = system(cmd);
if (is_priv)
umask(old_umask); umask(old_umask);
if (result != 0) 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*"); report_status(PG_REPORT, "*failure*");
fflush(stdout); fflush(stdout);
pg_log(PG_VERBOSE, "There were problems executing \"%s\"\n", cmd); pg_log(PG_VERBOSE, "There were problems executing \"%s\"\n", cmd);
if (opt_log_file)
pg_log(throw_error ? PG_FATAL : PG_REPORT, pg_log(throw_error ? PG_FATAL : PG_REPORT,
"Consult the last few lines of \"%s\"%s for\n" "Consult the last few lines of \"%s\" or \"%s\" for\n"
"the probable cause of the failure.\n", "the probable cause of the failure.\n",
log_file, opt_string); log_file, opt_log_file);
retval = 1;
}
else else
retval = 0; 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);
}
if ((log = fopen_priv(log_file, "a+")) == NULL) if ((log = fopen_priv(log_file, "a+")) == NULL)
pg_log(PG_FATAL, "cannot write to log file %s\n", log_file); pg_log(PG_FATAL, "cannot write to log file %s\n", log_file);
fprintf(log, "\n\n"); fprintf(log, "\n\n");
fclose(log); fclose(log);
return retval; return result == 0;
} }
......
contrib/pg_upgrade/pg_upgrade.c
View file @ 088c065c
...@@ -140,11 +140,10 @@ main(int argc, char **argv) ...@@ -140,11 +140,10 @@ main(int argc, char **argv)
* because there is no need to have the schema load use new oids. * because there is no need to have the schema load use new oids.
*/ */
prep_status("Setting next OID for new cluster"); prep_status("Setting next OID for new cluster");
exec_prog(true, true, UTILITY_LOG_FILE, NULL, exec_prog(UTILITY_LOG_FILE, NULL, true,
SYSTEMQUOTE "\"%s/pg_resetxlog\" -o %u \"%s\" >> \"%s\" 2>&1" "\"%s/pg_resetxlog\" -o %u \"%s\"",
SYSTEMQUOTE,
new_cluster.bindir, old_cluster.controldata.chkpnt_nxtoid, new_cluster.bindir, old_cluster.controldata.chkpnt_nxtoid,
new_cluster.pgdata, UTILITY_LOG_FILE); new_cluster.pgdata);
check_ok(); check_ok();
create_script_for_cluster_analyze(&analyze_script_file_name); create_script_for_cluster_analyze(&analyze_script_file_name);
...@@ -211,11 +210,10 @@ prepare_new_cluster(void) ...@@ -211,11 +210,10 @@ prepare_new_cluster(void)
* --analyze so autovacuum doesn't update statistics later * --analyze so autovacuum doesn't update statistics later
*/ */
prep_status("Analyzing all rows in the new cluster"); prep_status("Analyzing all rows in the new cluster");
exec_prog(true, true, UTILITY_LOG_FILE, NULL, exec_prog(UTILITY_LOG_FILE, NULL, true,
SYSTEMQUOTE "\"%s/vacuumdb\" --port %d --username \"%s\" " "\"%s/vacuumdb\" --port %d --username \"%s\" --all --analyze %s",
"--all --analyze %s >> \"%s\" 2>&1" SYSTEMQUOTE,
new_cluster.bindir, new_cluster.port, os_info.user, new_cluster.bindir, new_cluster.port, os_info.user,
log_opts.verbose ? "--verbose" : "", UTILITY_LOG_FILE); log_opts.verbose ? "--verbose" : "");
check_ok(); check_ok();
/* /*
...@@ -225,11 +223,10 @@ prepare_new_cluster(void) ...@@ -225,11 +223,10 @@ prepare_new_cluster(void)
* later. * later.
*/ */
prep_status("Freezing all rows on the new cluster"); prep_status("Freezing all rows on the new cluster");
exec_prog(true, true, UTILITY_LOG_FILE, NULL, exec_prog(UTILITY_LOG_FILE, NULL, true,
SYSTEMQUOTE "\"%s/vacuumdb\" --port %d --username \"%s\" " "\"%s/vacuumdb\" --port %d --username \"%s\" --all --freeze %s",
"--all --freeze %s >> \"%s\" 2>&1" SYSTEMQUOTE,
new_cluster.bindir, new_cluster.port, os_info.user, new_cluster.bindir, new_cluster.port, os_info.user,
log_opts.verbose ? "--verbose" : "", UTILITY_LOG_FILE); log_opts.verbose ? "--verbose" : "");
check_ok(); check_ok();
get_pg_database_relfilenode(&new_cluster); get_pg_database_relfilenode(&new_cluster);
...@@ -263,14 +260,10 @@ prepare_new_databases(void) ...@@ -263,14 +260,10 @@ prepare_new_databases(void)
* support functions in template1 but pg_dumpall creates database using * support functions in template1 but pg_dumpall creates database using
* the template0 template. * the template0 template.
*/ */
exec_prog(true, true, RESTORE_LOG_FILE, NULL, exec_prog(RESTORE_LOG_FILE, NULL, true,
SYSTEMQUOTE "\"%s/psql\" --echo-queries " "\"%s/psql\" " EXEC_PSQL_ARGS " --port %d --username \"%s\" -f \"%s\"",
"--set ON_ERROR_STOP=on "
/* --no-psqlrc prevents AUTOCOMMIT=off */
"--no-psqlrc --port %d --username \"%s\" "
"-f \"%s\" --dbname template1 >> \"%s\" 2>&1" SYSTEMQUOTE,
new_cluster.bindir, new_cluster.port, os_info.user, new_cluster.bindir, new_cluster.port, os_info.user,
GLOBALS_DUMP_FILE, RESTORE_LOG_FILE); GLOBALS_DUMP_FILE);
check_ok(); check_ok();
/* we load this to get a current list of databases */ /* we load this to get a current list of databases */
...@@ -296,13 +289,10 @@ create_new_objects(void) ...@@ -296,13 +289,10 @@ create_new_objects(void)
check_ok(); check_ok();
prep_status("Restoring database schema to new cluster"); prep_status("Restoring database schema to new cluster");
exec_prog(true, true, RESTORE_LOG_FILE, NULL, exec_prog(RESTORE_LOG_FILE, NULL, true,
SYSTEMQUOTE "\"%s/psql\" --echo-queries " "\"%s/psql\" " EXEC_PSQL_ARGS " --port %d --username \"%s\" -f \"%s\"",
"--set ON_ERROR_STOP=on "
"--no-psqlrc --port %d --username \"%s\" "
"-f \"%s\" --dbname template1 >> \"%s\" 2>&1" SYSTEMQUOTE,
new_cluster.bindir, new_cluster.port, os_info.user, new_cluster.bindir, new_cluster.port, os_info.user,
DB_DUMP_FILE, RESTORE_LOG_FILE); DB_DUMP_FILE);
check_ok(); check_ok();
/* regenerate now that we have objects in the databases */ /* regenerate now that we have objects in the databases */
...@@ -331,16 +321,14 @@ copy_subdir_files(char *subdir) ...@@ -331,16 +321,14 @@ copy_subdir_files(char *subdir)
prep_status("Copying old %s to new server", subdir); prep_status("Copying old %s to new server", subdir);
exec_prog(true, false, UTILITY_LOG_FILE, NULL, exec_prog(UTILITY_LOG_FILE, NULL, true,
#ifndef WIN32 #ifndef WIN32
SYSTEMQUOTE "%s \"%s\" \"%s\" >> \"%s\" 2>&1" SYSTEMQUOTE, "cp -Rf \"%s\" \"%s\"",
"cp -Rf",
#else #else
/* flags: everything, no confirm, quiet, overwrite read-only */ /* flags: everything, no confirm, quiet, overwrite read-only */
SYSTEMQUOTE "%s \"%s\" \"%s\\\" >> \"%s\" 2>&1" SYSTEMQUOTE, "xcopy /e /y /q /r \"%s\" \"%s\\\"",
"xcopy /e /y /q /r",
#endif #endif
old_path, new_path, UTILITY_LOG_FILE); old_path, new_path);
check_ok(); check_ok();
} }
...@@ -353,22 +341,18 @@ copy_clog_xlog_xid(void) ...@@ -353,22 +341,18 @@ copy_clog_xlog_xid(void)
/* set the next transaction id of the new cluster */ /* set the next transaction id of the new cluster */
prep_status("Setting next transaction ID for new cluster"); prep_status("Setting next transaction ID for new cluster");
exec_prog(true, true, UTILITY_LOG_FILE, NULL, exec_prog(UTILITY_LOG_FILE, NULL, true,
SYSTEMQUOTE "\"%s/pg_resetxlog\" -f -x %u \"%s\"",
"\"%s/pg_resetxlog\" -f -x %u \"%s\" >> \"%s\" 2>&1" new_cluster.bindir, old_cluster.controldata.chkpnt_nxtxid,
SYSTEMQUOTE, new_cluster.bindir, new_cluster.pgdata);
old_cluster.controldata.chkpnt_nxtxid,
new_cluster.pgdata, UTILITY_LOG_FILE);
check_ok(); check_ok();
/* now reset the wal archives in the new cluster */ /* now reset the wal archives in the new cluster */
prep_status("Resetting WAL archives"); prep_status("Resetting WAL archives");
exec_prog(true, true, UTILITY_LOG_FILE, NULL, exec_prog(UTILITY_LOG_FILE, NULL, true,
SYSTEMQUOTE "\"%s/pg_resetxlog\" -l %s \"%s\"", new_cluster.bindir,
"\"%s/pg_resetxlog\" -l %s \"%s\" >> \"%s\" 2>&1"
SYSTEMQUOTE, new_cluster.bindir,
old_cluster.controldata.nextxlogfile, old_cluster.controldata.nextxlogfile,
new_cluster.pgdata, UTILITY_LOG_FILE); new_cluster.pgdata);
check_ok(); check_ok();
} }
......
contrib/pg_upgrade/pg_upgrade.h
View file @ 088c065c
...@@ -316,10 +316,11 @@ void split_old_dump(void); ...@@ -316,10 +316,11 @@ void split_old_dump(void);
/* exec.c */ /* exec.c */
int #define EXEC_PSQL_ARGS "--echo-queries --set ON_ERROR_STOP=on --no-psqlrc --dbname=template1"
exec_prog(bool throw_error, bool is_priv, const char *log_file, bool
const char *opt_log_file, const char *cmd,...) exec_prog(const char *log_file, const char *opt_log_file,
__attribute__((format(PG_PRINTF_ATTRIBUTE, 5, 6))); bool throw_error, const char *fmt,...)
__attribute__((format(PG_PRINTF_ATTRIBUTE, 4, 5)));
void verify_directories(void); void verify_directories(void);
bool is_server_running(const char *datadir); bool is_server_running(const char *datadir);
......
contrib/pg_upgrade/server.c
View file @ 088c065c
...@@ -143,7 +143,7 @@ start_postmaster(ClusterInfo *cluster) ...@@ -143,7 +143,7 @@ start_postmaster(ClusterInfo *cluster)
char cmd[MAXPGPATH]; char cmd[MAXPGPATH];
PGconn *conn; PGconn *conn;
bool exit_hook_registered = false; bool exit_hook_registered = false;
int pg_ctl_return = 0; bool pg_ctl_return = false;
if (!exit_hook_registered) if (!exit_hook_registered)
{ {
...@@ -159,22 +159,23 @@ start_postmaster(ClusterInfo *cluster) ...@@ -159,22 +159,23 @@ start_postmaster(ClusterInfo *cluster)
* not touch them. * not touch them.
*/ */
snprintf(cmd, sizeof(cmd), snprintf(cmd, sizeof(cmd),
SYSTEMQUOTE "\"%s/pg_ctl\" -w -l \"%s\" -D \"%s\" " "\"%s/pg_ctl\" -w -l \"%s\" -D \"%s\" -o \"-p %d %s %s\" start",
"-o \"-p %d %s %s\" start >> \"%s\" 2>&1" SYSTEMQUOTE,
cluster->bindir, SERVER_LOG_FILE, cluster->pgconfig, cluster->port, cluster->bindir, SERVER_LOG_FILE, cluster->pgconfig, cluster->port,
(cluster->controldata.cat_ver >= (cluster->controldata.cat_ver >=
BINARY_UPGRADE_SERVER_FLAG_CAT_VER) ? "-b" : BINARY_UPGRADE_SERVER_FLAG_CAT_VER) ? "-b" :
"-c autovacuum=off -c autovacuum_freeze_max_age=2000000000", "-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 * Don't throw an error right away, let connecting throw the error because
* it might supply a reason for the failure. * it might supply a reason for the failure.
*/ */
pg_ctl_return = exec_prog(false, true, SERVER_START_LOG_FILE, pg_ctl_return = exec_prog(SERVER_START_LOG_FILE,
/* pass both file names if the differ */ /* pass both file names if they differ */
(strcmp(SERVER_LOG_FILE, SERVER_START_LOG_FILE) != 0) ? (strcmp(SERVER_LOG_FILE,
SERVER_START_LOG_FILE) != 0) ?
SERVER_LOG_FILE : NULL, SERVER_LOG_FILE : NULL,
false,
"%s", cmd); "%s", cmd);
/* Check to see if we can connect to the server; if not, report it. */ /* Check to see if we can connect to the server; if not, report it. */
...@@ -185,13 +186,14 @@ start_postmaster(ClusterInfo *cluster) ...@@ -185,13 +186,14 @@ start_postmaster(ClusterInfo *cluster)
PQerrorMessage(conn)); PQerrorMessage(conn));
if (conn) if (conn)
PQfinish(conn); PQfinish(conn);
pg_log(PG_FATAL, "could not connect to %s postmaster started with the command: %s\n", pg_log(PG_FATAL, "could not connect to %s postmaster started with the command:\n"
"%s\n",
CLUSTER_NAME(cluster), cmd); CLUSTER_NAME(cluster), cmd);
} }
PQfinish(conn); PQfinish(conn);
/* If the connection didn't fail, fail now */ /* 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", pg_log(PG_FATAL, "pg_ctl failed to start the %s server, or connection failed\n",
CLUSTER_NAME(cluster)); CLUSTER_NAME(cluster));
...@@ -202,7 +204,6 @@ start_postmaster(ClusterInfo *cluster) ...@@ -202,7 +204,6 @@ start_postmaster(ClusterInfo *cluster)
void void
stop_postmaster(bool fast) stop_postmaster(bool fast)
{ {
char cmd[MAXPGPATH];
ClusterInfo *cluster; ClusterInfo *cluster;
if (os_info.running_cluster == &old_cluster) if (os_info.running_cluster == &old_cluster)
...@@ -212,14 +213,11 @@ stop_postmaster(bool fast) ...@@ -212,14 +213,11 @@ stop_postmaster(bool fast)
else else
return; /* no cluster running */ return; /* no cluster running */
snprintf(cmd, sizeof(cmd), exec_prog(SERVER_STOP_LOG_FILE, NULL, !fast,
SYSTEMQUOTE "\"%s/pg_ctl\" -w -D \"%s\" -o \"%s\" " "\"%s/pg_ctl\" -w -D \"%s\" -o \"%s\" %s stop",
"%s stop >> \"%s\" 2>&1" SYSTEMQUOTE,
cluster->bindir, cluster->pgconfig, cluster->bindir, cluster->pgconfig,
cluster->pgopts ? cluster->pgopts : "", cluster->pgopts ? cluster->pgopts : "",
fast ? "-m fast" : "", SERVER_STOP_LOG_FILE); fast ? "-m fast" : "");
exec_prog(fast ? false : true, true, SERVER_STOP_LOG_FILE, NULL, "%s", cmd);
os_info.running_cluster = NULL; os_info.running_cluster = NULL;
} }
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment