pg_upgrade: Move all the files generated internally to a subdirectory
authorMichael Paquier <michael@paquier.xyz>
Sun, 6 Feb 2022 03:27:29 +0000 (12:27 +0900)
committerMichael Paquier <michael@paquier.xyz>
Sun, 6 Feb 2022 03:27:29 +0000 (12:27 +0900)
Historically, the location of any files generated by pg_upgrade, as of
the per-database logs and internal dumps, has been the current working
directory, leaving all those files behind when using --retain or on a
failure.

Putting all those contents in a targeted subdirectory makes the whole
easier to debug, and simplifies the code in charge of cleaning up the
logs.  Note that another reason is that this facilitates the move of
pg_upgrade to TAP with a fixed location for all the logs to grab if the
test fails repeatedly.

Initially, we thought about being able to specify the output directory
with a new option, but we have settled on using a subdirectory located
at the root of the new cluster's data folder, "pg_upgrade_output.d",
instead, as at the end the new data directory is the location of all the
data generated by pg_upgrade.  There is a take with group permissions
here though: if the new data folder has been initialized with this
option, we need to create all the files and paths with the correct
permissions or a base backup taken after a pg_upgrade --retain would
fail, meaning that GetDataDirectoryCreatePerm() has to be called before
creating the log paths, before a couple of sanity checks on the clusters
and before getting the socket directory for the cluster's host settings.
The idea of the new location is based on a suggestion from Peter
Eisentraut.

Also thanks to Andrew Dunstan, Peter Eisentraut, Daniel Gustafsson, Tom
Lane and Bruce Momjian for the discussion (in alphabetical order).

Author: Justin Pryzby
Discussion: https://postgr.es/m/20211212025017.GN17618@telsasoft.com

doc/src/sgml/ref/pgupgrade.sgml
src/bin/pg_upgrade/.gitignore
src/bin/pg_upgrade/Makefile
src/bin/pg_upgrade/check.c
src/bin/pg_upgrade/dump.c
src/bin/pg_upgrade/exec.c
src/bin/pg_upgrade/function.c
src/bin/pg_upgrade/option.c
src/bin/pg_upgrade/pg_upgrade.c
src/bin/pg_upgrade/pg_upgrade.h
src/bin/pg_upgrade/server.c

index a1745a2642cef1ca0169b3176ba98eaaff667b2d..729c886ac00cc7789cf74debc4c83c64564d50f2 100644 (file)
@@ -768,8 +768,8 @@ psql --username=postgres --file=script.sql postgres
 
   <para>
    <application>pg_upgrade</application> creates various working files, such
-   as schema dumps, in the current working directory.  For security, be sure
-   that that directory is not readable or writable by any other users.
+   as schema dumps, stored within <literal>pg_upgrade_output.d</literal> in
+   the directory of the new cluster.
   </para>
 
   <para>
index 2d3bfeaa502137899a761835e692f60674277bd9..939e50db6c5a81aeb2481e470ad1eeeed7695156 100644 (file)
@@ -1,9 +1,7 @@
 /pg_upgrade
 # Generated by test suite
-/pg_upgrade_internal.log
 /delete_old_cluster.sh
 /delete_old_cluster.bat
 /reindex_hash.sql
-/loadable_libraries.txt
 /log/
 /tmp_check/
index 44d06be5a61bab3c5b4c35f39efac686d01f4b01..49b94f0ac7992dabf570d053d5711d5d5b4c0fc5 100644 (file)
@@ -45,9 +45,7 @@ uninstall:
 clean distclean maintainer-clean:
    rm -f pg_upgrade$(X) $(OBJS)
    rm -rf delete_old_cluster.sh log/ tmp_check/ \
-          loadable_libraries.txt reindex_hash.sql \
-          pg_upgrade_dump_globals.sql \
-          pg_upgrade_dump_*.custom pg_upgrade_*.log
+          reindex_hash.sql
 
 # When $(MAKE) is present, make automatically infers that this is a
 # recursive make. which is not actually what we want here, as that
index 3d218c2ad24792be57acbe513ac58aa32a6db715..019bcb6c7b74c61d83da224c3a207e6fdbe63744 100644 (file)
@@ -783,7 +783,8 @@ check_for_isn_and_int8_passing_mismatch(ClusterInfo *cluster)
        return;
    }
 
-   snprintf(output_path, sizeof(output_path),
+   snprintf(output_path, sizeof(output_path), "%s/%s",
+            log_opts.basedir,
             "contrib_isn_and_int8_pass_by_value.txt");
 
    for (dbnum = 0; dbnum < cluster->dbarr.ndbs; dbnum++)
@@ -860,7 +861,8 @@ check_for_user_defined_postfix_ops(ClusterInfo *cluster)
 
    prep_status("Checking for user-defined postfix operators");
 
-   snprintf(output_path, sizeof(output_path),
+   snprintf(output_path, sizeof(output_path), "%s/%s",
+            log_opts.basedir,
             "postfix_ops.txt");
 
    /* Find any user defined postfix operators */
@@ -959,7 +961,8 @@ check_for_tables_with_oids(ClusterInfo *cluster)
 
    prep_status("Checking for tables WITH OIDS");
 
-   snprintf(output_path, sizeof(output_path),
+   snprintf(output_path, sizeof(output_path), "%s/%s",
+            log_opts.basedir,
             "tables_with_oids.txt");
 
    /* Find any tables declared WITH OIDS */
@@ -1214,7 +1217,8 @@ check_for_user_defined_encoding_conversions(ClusterInfo *cluster)
 
    prep_status("Checking for user-defined encoding conversions");
 
-   snprintf(output_path, sizeof(output_path),
+   snprintf(output_path, sizeof(output_path), "%s/%s",
+            log_opts.basedir,
             "encoding_conversions.txt");
 
    /* Find any user defined encoding conversions */
index 953873fa01de98fae2f41f03fe4377ac3b53ffae..b69b4f95695ec107995c6050dc3daa68f501a8ec 100644 (file)
@@ -22,9 +22,10 @@ generate_old_dump(void)
    /* run new pg_dumpall binary for globals */
    exec_prog(UTILITY_LOG_FILE, NULL, true, true,
              "\"%s/pg_dumpall\" %s --globals-only --quote-all-identifiers "
-             "--binary-upgrade %s -f %s",
+             "--binary-upgrade %s -f \"%s/%s\"",
              new_cluster.bindir, cluster_conn_opts(&old_cluster),
              log_opts.verbose ? "--verbose" : "",
+             log_opts.dumpdir,
              GLOBALS_DUMP_FILE);
    check_ok();
 
@@ -52,9 +53,10 @@ generate_old_dump(void)
 
        parallel_exec_prog(log_file_name, NULL,
                           "\"%s/pg_dump\" %s --schema-only --quote-all-identifiers "
-                          "--binary-upgrade --format=custom %s --file=\"%s\" %s",
+                          "--binary-upgrade --format=custom %s --file=\"%s/%s\" %s",
                           new_cluster.bindir, cluster_conn_opts(&old_cluster),
                           log_opts.verbose ? "--verbose" : "",
+                          log_opts.dumpdir,
                           sql_file_name, escaped_connstr.data);
 
        termPQExpBuffer(&escaped_connstr);
index c0cbb1bec79b6de79cdd98a6287058bdb19146c7..fadeea12cac4d767868afd5610bccdf5f1db00e9 100644 (file)
@@ -78,11 +78,12 @@ get_bin_version(ClusterInfo *cluster)
  * The code requires it be called first from the primary thread on Windows.
  */
 bool
-exec_prog(const char *log_file, const char *opt_log_file,
+exec_prog(const char *log_filename, const char *opt_log_file,
          bool report_error, bool exit_on_error, const char *fmt,...)
 {
    int         result = 0;
    int         written;
+   char        log_file[MAXPGPATH];
 
 #define MAXCMDLEN (2 * MAXPGPATH)
    char        cmd[MAXCMDLEN];
@@ -97,6 +98,8 @@ exec_prog(const char *log_file, const char *opt_log_file,
        mainThreadId = GetCurrentThreadId();
 #endif
 
+   snprintf(log_file, MAXPGPATH, "%s/%s", log_opts.logdir, log_filename);
+
    written = 0;
    va_start(ap, fmt);
    written += vsnprintf(cmd + written, MAXCMDLEN - written, fmt, ap);
index e73a309070835219224cb8b25a14ec61bc914121..ea785df771a99fb37cedd0e69be70b82831d92f8 100644 (file)
@@ -128,7 +128,8 @@ check_loadable_libraries(void)
 
    prep_status("Checking for presence of required libraries");
 
-   snprintf(output_path, sizeof(output_path), "loadable_libraries.txt");
+   snprintf(output_path, sizeof(output_path), "%s/%s",
+            log_opts.basedir, "loadable_libraries.txt");
 
    /*
     * Now we want to sort the library names into order.  This avoids multiple
index c894fdd685eb20e179c3c861edccca8e0b0a5258..d2c82cc2bbb97570b54e627b006c7fddfaffa753 100644 (file)
@@ -9,7 +9,6 @@
 
 #include "postgres_fe.h"
 
-#include <time.h>
 #ifdef WIN32
 #include <io.h>
 #endif
@@ -63,9 +62,6 @@ parseCommandLine(int argc, char *argv[])
    int         option;         /* Command line option */
    int         optindex = 0;   /* used by getopt_long */
    int         os_user_effective_id;
-   FILE       *fp;
-   char      **filename;
-   time_t      run_time = time(NULL);
 
    user_opts.do_sync = true;
    user_opts.transfer_mode = TRANSFER_MODE_COPY;
@@ -208,27 +204,9 @@ parseCommandLine(int argc, char *argv[])
    if (optind < argc)
        pg_fatal("too many command-line arguments (first is \"%s\")\n", argv[optind]);
 
-   if ((log_opts.internal = fopen_priv(INTERNAL_LOG_FILE, "a")) == NULL)
-       pg_fatal("could not open log file \"%s\": %m\n", INTERNAL_LOG_FILE);
-
    if (log_opts.verbose)
        pg_log(PG_REPORT, "Running in verbose mode\n");
 
-   /* label start of upgrade in logfiles */
-   for (filename = output_files; *filename != NULL; filename++)
-   {
-       if ((fp = fopen_priv(*filename, "a")) == NULL)
-           pg_fatal("could not write to log file \"%s\": %m\n", *filename);
-
-       /* Start with newline because we might be appending to a file. */
-       fprintf(fp, "\n"
-               "-----------------------------------------------------------------\n"
-               "  pg_upgrade run on %s"
-               "-----------------------------------------------------------------\n\n",
-               ctime(&run_time));
-       fclose(fp);
-   }
-
    /* Turn off read-only mode;  add prefix to PGOPTIONS? */
    if (getenv("PGOPTIONS"))
    {
index 07defacd673223ca46970a0d4be8202f5439f8d6..48a54170a72b348c771aeca6aa176f890cce6a63 100644 (file)
@@ -38,6 +38,8 @@
 
 #include "postgres_fe.h"
 
+#include <time.h>
+
 #ifdef HAVE_LANGINFO_H
 #include <langinfo.h>
 #endif
@@ -54,6 +56,7 @@ static void prepare_new_globals(void);
 static void create_new_objects(void);
 static void copy_xact_xlog_xid(void);
 static void set_frozenxids(bool minmxid_only);
+static void make_outputdirs(char *pgdata);
 static void setup(char *argv0, bool *live_check);
 static void cleanup(void);
 
@@ -92,6 +95,22 @@ main(int argc, char **argv)
    adjust_data_dir(&old_cluster);
    adjust_data_dir(&new_cluster);
 
+   /*
+    * Set mask based on PGDATA permissions, needed for the creation of the
+    * output directories with correct permissions.
+    */
+   if (!GetDataDirectoryCreatePerm(new_cluster.pgdata))
+       pg_fatal("could not read permissions of directory \"%s\": %s\n",
+                new_cluster.pgdata, strerror(errno));
+
+   umask(pg_mode_mask);
+
+   /*
+    * This needs to happen after adjusting the data directory of the new
+    * cluster in adjust_data_dir().
+    */
+   make_outputdirs(new_cluster.pgdata);
+
    setup(argv[0], &live_check);
 
    output_check_banner(live_check);
@@ -103,13 +122,6 @@ main(int argc, char **argv)
 
    check_cluster_compatibility(live_check);
 
-   /* Set mask based on PGDATA permissions */
-   if (!GetDataDirectoryCreatePerm(new_cluster.pgdata))
-       pg_fatal("could not read permissions of directory \"%s\": %s\n",
-                new_cluster.pgdata, strerror(errno));
-
-   umask(pg_mode_mask);
-
    check_and_dump_old_cluster(live_check);
 
 
@@ -197,6 +209,56 @@ main(int argc, char **argv)
    return 0;
 }
 
+/*
+ * Create and assign proper permissions to the set of output directories
+ * used to store any data generated internally, filling in log_opts in
+ * the process.
+ */
+static void
+make_outputdirs(char *pgdata)
+{
+   FILE       *fp;
+   char      **filename;
+   time_t      run_time = time(NULL);
+   char        filename_path[MAXPGPATH];
+
+   log_opts.basedir = (char *) pg_malloc(MAXPGPATH);
+   snprintf(log_opts.basedir, MAXPGPATH, "%s/%s", pgdata, BASE_OUTPUTDIR);
+   log_opts.dumpdir = (char *) pg_malloc(MAXPGPATH);
+   snprintf(log_opts.dumpdir, MAXPGPATH, "%s/%s", pgdata, LOG_OUTPUTDIR);
+   log_opts.logdir = (char *) pg_malloc(MAXPGPATH);
+   snprintf(log_opts.logdir, MAXPGPATH, "%s/%s", pgdata, DUMP_OUTPUTDIR);
+
+   if (mkdir(log_opts.basedir, pg_dir_create_mode))
+       pg_fatal("could not create directory \"%s\": %m\n", log_opts.basedir);
+   if (mkdir(log_opts.dumpdir, pg_dir_create_mode))
+       pg_fatal("could not create directory \"%s\": %m\n", log_opts.dumpdir);
+   if (mkdir(log_opts.logdir, pg_dir_create_mode))
+       pg_fatal("could not create directory \"%s\": %m\n", log_opts.logdir);
+
+   snprintf(filename_path, sizeof(filename_path), "%s/%s", log_opts.logdir,
+            INTERNAL_LOG_FILE);
+   if ((log_opts.internal = fopen_priv(filename_path, "a")) == NULL)
+       pg_fatal("could not open log file \"%s\": %m\n", filename_path);
+
+   /* label start of upgrade in logfiles */
+   for (filename = output_files; *filename != NULL; filename++)
+   {
+       snprintf(filename_path, sizeof(filename_path), "%s/%s",
+                log_opts.logdir, *filename);
+       if ((fp = fopen_priv(filename_path, "a")) == NULL)
+           pg_fatal("could not write to log file \"%s\": %m\n", filename_path);
+
+       /* Start with newline because we might be appending to a file. */
+       fprintf(fp, "\n"
+               "-----------------------------------------------------------------\n"
+               "  pg_upgrade run on %s"
+               "-----------------------------------------------------------------\n\n",
+               ctime(&run_time));
+       fclose(fp);
+   }
+}
+
 
 static void
 setup(char *argv0, bool *live_check)
@@ -306,8 +368,9 @@ prepare_new_globals(void)
    prep_status("Restoring global objects in the new cluster");
 
    exec_prog(UTILITY_LOG_FILE, NULL, true, true,
-             "\"%s/psql\" " EXEC_PSQL_ARGS " %s -f \"%s\"",
+             "\"%s/psql\" " EXEC_PSQL_ARGS " %s -f \"%s/%s\"",
              new_cluster.bindir, cluster_conn_opts(&new_cluster),
+             log_opts.dumpdir,
              GLOBALS_DUMP_FILE);
    check_ok();
 }
@@ -352,10 +415,11 @@ create_new_objects(void)
                  true,
                  true,
                  "\"%s/pg_restore\" %s %s --exit-on-error --verbose "
-                 "--dbname postgres \"%s\"",
+                 "--dbname postgres \"%s/%s\"",
                  new_cluster.bindir,
                  cluster_conn_opts(&new_cluster),
                  create_opts,
+                 log_opts.dumpdir,
                  sql_file_name);
 
        break;                  /* done once we've processed template1 */
@@ -389,10 +453,11 @@ create_new_objects(void)
        parallel_exec_prog(log_file_name,
                           NULL,
                           "\"%s/pg_restore\" %s %s --exit-on-error --verbose "
-                          "--dbname template1 \"%s\"",
+                          "--dbname template1 \"%s/%s\"",
                           new_cluster.bindir,
                           cluster_conn_opts(&new_cluster),
                           create_opts,
+                          log_opts.dumpdir,
                           sql_file_name);
    }
 
@@ -689,28 +754,5 @@ cleanup(void)
 
    /* Remove dump and log files? */
    if (!log_opts.retain)
-   {
-       int         dbnum;
-       char      **filename;
-
-       for (filename = output_files; *filename != NULL; filename++)
-           unlink(*filename);
-
-       /* remove dump files */
-       unlink(GLOBALS_DUMP_FILE);
-
-       if (old_cluster.dbarr.dbs)
-           for (dbnum = 0; dbnum < old_cluster.dbarr.ndbs; dbnum++)
-           {
-               char        sql_file_name[MAXPGPATH],
-                           log_file_name[MAXPGPATH];
-               DbInfo     *old_db = &old_cluster.dbarr.dbs[dbnum];
-
-               snprintf(sql_file_name, sizeof(sql_file_name), DB_DUMP_FILE_MASK, old_db->db_oid);
-               unlink(sql_file_name);
-
-               snprintf(log_file_name, sizeof(log_file_name), DB_DUMP_LOG_FILE_MASK, old_db->db_oid);
-               unlink(log_file_name);
-           }
-   }
+       rmtree(log_opts.basedir, true);
 }
index 1db8e3f0fbe023a0ed29ee3540a274246f710dae..0aca0a77aaee6ef36a1f435ba07e4e2ec609e8a8 100644 (file)
 #define GLOBALS_DUMP_FILE  "pg_upgrade_dump_globals.sql"
 #define DB_DUMP_FILE_MASK  "pg_upgrade_dump_%u.custom"
 
+/*
+ * Base directories that include all the files generated internally,
+ * from the root path of the new cluster.
+ */
+#define BASE_OUTPUTDIR     "pg_upgrade_output.d"
+#define LOG_OUTPUTDIR      BASE_OUTPUTDIR "/log"
+#define DUMP_OUTPUTDIR     BASE_OUTPUTDIR "/dump"
+
 #define DB_DUMP_LOG_FILE_MASK  "pg_upgrade_dump_%u.log"
 #define SERVER_LOG_FILE        "pg_upgrade_server.log"
 #define UTILITY_LOG_FILE   "pg_upgrade_utility.log"
@@ -262,6 +270,10 @@ typedef struct
    FILE       *internal;       /* internal log FILE */
    bool        verbose;        /* true -> be verbose in messages */
    bool        retain;         /* retain log files on success */
+   /* Set of internal directories for output files */
+   char       *basedir;        /* Base output directory */
+   char       *dumpdir;        /* Dumps */
+   char       *logdir;         /* Log files */
 } LogOpts;
 
 
index f5d4656dec82aa3a1c04802c4d35f6afd55987d5..7878d233de2fc5d0c59295d40f792f1782a33bcc 100644 (file)
@@ -238,8 +238,10 @@ start_postmaster(ClusterInfo *cluster, bool report_and_exit_on_error)
     * vacuumdb --freeze actually freezes the tuples.
     */
    snprintf(cmd, sizeof(cmd),
-            "\"%s/pg_ctl\" -w -l \"%s\" -D \"%s\" -o \"-p %d -b%s %s%s\" start",
-            cluster->bindir, SERVER_LOG_FILE, cluster->pgconfig, cluster->port,
+            "\"%s/pg_ctl\" -w -l \"%s/%s\" -D \"%s\" -o \"-p %d -b%s %s%s\" start",
+            cluster->bindir,
+            log_opts.logdir,
+            SERVER_LOG_FILE, cluster->pgconfig, cluster->port,
             (cluster == &new_cluster) ?
             " -c synchronous_commit=off -c fsync=off -c full_page_writes=off -c vacuum_defer_cleanup_age=0" : "",
             cluster->pgopts ? cluster->pgopts : "", socket_string);