pg_upgrade: Fix exec_prog API to be less flaky
authorAlvaro Herrera <alvherre@alvh.no-ip.org>
Mon, 27 Aug 2012 18:21:09 +0000 (14:21 -0400)
committerAlvaro Herrera <alvherre@alvh.no-ip.org>
Mon, 27 Aug 2012 18:29:14 +0000 (14:29 -0400)
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.

contrib/pg_upgrade/check.c
contrib/pg_upgrade/dump.c
contrib/pg_upgrade/exec.c
contrib/pg_upgrade/pg_upgrade.c
contrib/pg_upgrade/pg_upgrade.h
contrib/pg_upgrade/server.c

index aa896b582375162792eb608fa66b6f28adb599ef..0fec73ec7dc82f395afe32d29b88e3f6c83c8fbf 100644 (file)
@@ -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, NULL, 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();
                }
index 07a3b548a9f750f85a919380c790b6b58def6c9f..cfc4017d517a3ca093fef101df83c4b9914bbf14 100644 (file)
@@ -23,12 +23,11 @@ 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,
+       exec_prog(UTILITY_LOG_FILE, NULL, 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, UTILITY_LOG_FILE);
+                         ALL_DUMP_FILE);
        check_ok();
 }
 
index 6f993df53a3dc0ec71c272b1f8b9ccb5d68f5e25..c75d9dbcc9722ccb69be626110228205e5e9e1c0 100644 (file)
@@ -26,77 +26,81 @@ static int  win32_check_directory_write_permissions(void);
 
 /*
  * 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 true.
  *
- *     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 and exec_prog()
+ * returns false.
  */
-int
-exec_prog(bool throw_error, bool is_priv, const char *log_file,
-                 const char *opt_log_file, const char *fmt,...)
+bool
+exec_prog(const char *log_file, const char *opt_log_file,
+                 bool throw_error, const char *fmt,...)
 {
-       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;
+       va_list         ap;
 
-       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 = strlcpy(cmd, SYSTEMQUOTE, strlen(SYSTEMQUOTE));
+       va_start(ap, fmt);
+       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)
                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;
 }
 
 
index eff1a0872f20d22ec6ac53dec6a7bb4fb8d3c6cc..c47c8bba4452da03fd2174682f0679ada45aa38c 100644 (file)
@@ -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, NULL, 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, NULL, 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, NULL, 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, NULL, 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, NULL, 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, NULL, 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, NULL, 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, NULL, 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();
 }
 
index affee7a9d93c5a6f30da48b6dee8d024acaaf0c0..fa4c6c0a478484da1a99f059cab33ff17502903e 100644 (file)
@@ -316,10 +316,11 @@ 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"
+bool
+exec_prog(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);
 
index e94a897c92c89963a4430f877516060acf4668e4..1fb0d6ccceb2d6efd750d00402778cd0314936b1 100644 (file)
@@ -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,22 +159,23 @@ 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) ?
+       pg_ctl_return = exec_prog(SERVER_START_LOG_FILE,
+                                                         /* pass both file names if they 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. */
@@ -185,13 +186,14 @@ start_postmaster(ClusterInfo *cluster)
                           PQerrorMessage(conn));
                if (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);
        }
        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 +204,6 @@ start_postmaster(ClusterInfo *cluster)
 void
 stop_postmaster(bool fast)
 {
-       char            cmd[MAXPGPATH];
        ClusterInfo *cluster;
 
        if (os_info.running_cluster == &old_cluster)
@@ -212,14 +213,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, NULL, !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;
 }