On Sun, Jun 05, 2022 at 09:24:25AM +0900, Michael Paquier wrote: > Well, another error that could happen in the early code paths is > EACCES on a custom socket directory specified, and we'd still face the > same problem on a follow-up restart. Using a sub-directory structure > as Daniel and Tom mention would address all that (if ignoring EEXIST > for the BASE_OUTPUTDIR), removing any existing content from the base > path when not using --retain. This comes with the disadvantage of > bloating the disk on repeated errors, but this last bit would not > really be a huge problem, I guess, as it could be more useful to keep > the error information around.
I have been toying with the idea of a sub-directory named with a timestamp (Unix time, like log_line_prefix's %n but this could be any format) under pg_upgrade_output.d/ and finished with the attached. The logs are removed from the root path when --check is used without --retain, like for a non-check command. I have added a set of tests to provide some coverage for the whole: - Failure of --check where the binary path does not exist, and pg_upgrade_output.d/ is not removed. - Follow-up run of pg_upgrade --check, where pg_upgrade_output.d/ is removed. - Check that pg_upgrade_output.d/ is also removed after the main upgrade command completes. The logic in charge of cleaning up the logs has been moved to a single routine, aka cleanup_logs(). Thoughts? -- Michael
diff --git a/src/bin/pg_upgrade/check.c b/src/bin/pg_upgrade/check.c
index 6114303b52..b736f89816 100644
--- a/src/bin/pg_upgrade/check.c
+++ b/src/bin/pg_upgrade/check.c
@@ -210,6 +210,8 @@ report_clusters_compatible(void)
pg_log(PG_REPORT, "\n*Clusters are compatible*\n");
/* stops new cluster */
stop_postmaster(false);
+
+ cleanup_logs();
exit(0);
}
diff --git a/src/bin/pg_upgrade/pg_upgrade.c b/src/bin/pg_upgrade/pg_upgrade.c
index ecb3e1f647..ac854d268f 100644
--- a/src/bin/pg_upgrade/pg_upgrade.c
+++ b/src/bin/pg_upgrade/pg_upgrade.c
@@ -58,7 +58,6 @@ 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);
ClusterInfo old_cluster,
new_cluster;
@@ -204,7 +203,7 @@ main(int argc, char **argv)
pg_free(deletion_script_file_name);
- cleanup();
+ cleanup_logs();
return 0;
}
@@ -221,14 +220,37 @@ make_outputdirs(char *pgdata)
char **filename;
time_t run_time = time(NULL);
char filename_path[MAXPGPATH];
+ char timebuf[128];
+ struct timeval time;
+ log_opts.rootdir = (char *) pg_malloc(MAXPGPATH);
+ snprintf(log_opts.rootdir, MAXPGPATH, "%s/%s", pgdata, BASE_OUTPUTDIR);
+
+ /* BASE_OUTPUTDIR/$unix_timestamp/ */
+ gettimeofday(&time, NULL);
+ snprintf(timebuf, sizeof(timebuf), "%ld.%03d",
+ (long) time.tv_sec,
+ (int) (time.tv_usec / 1000));
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, DUMP_OUTPUTDIR);
- log_opts.logdir = (char *) pg_malloc(MAXPGPATH);
- snprintf(log_opts.logdir, MAXPGPATH, "%s/%s", pgdata, LOG_OUTPUTDIR);
+ snprintf(log_opts.basedir, MAXPGPATH, "%s/%s", log_opts.rootdir,
+ timebuf);
+ /* BASE_OUTPUTDIR/$unix_timestamp/dump/ */
+ log_opts.dumpdir = (char *) pg_malloc(MAXPGPATH);
+ snprintf(log_opts.dumpdir, MAXPGPATH, "%s/%s/%s", log_opts.rootdir,
+ timebuf, DUMP_OUTPUTDIR);
+
+ /* BASE_OUTPUTDIR/$unix_timestamp/log/ */
+ log_opts.logdir = (char *) pg_malloc(MAXPGPATH);
+ snprintf(log_opts.logdir, MAXPGPATH, "%s/%s/%s", log_opts.rootdir,
+ timebuf, LOG_OUTPUTDIR);
+
+ /*
+ * Ignore the error case where the root path exists, as it is kept
+ * the same across runs.
+ */
+ if (mkdir(log_opts.rootdir, pg_dir_create_mode) && errno != EEXIST)
+ pg_fatal("could not create directory \"%s\": %m\n", log_opts.rootdir);
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))
@@ -745,14 +767,3 @@ set_frozenxids(bool minmxid_only)
check_ok();
}
-
-
-static void
-cleanup(void)
-{
- fclose(log_opts.internal);
-
- /* Remove dump and log files? */
- if (!log_opts.retain)
- (void) rmtree(log_opts.basedir, true);
-}
diff --git a/src/bin/pg_upgrade/pg_upgrade.h b/src/bin/pg_upgrade/pg_upgrade.h
index 86d3dc46fa..157ef0e2ff 100644
--- a/src/bin/pg_upgrade/pg_upgrade.h
+++ b/src/bin/pg_upgrade/pg_upgrade.h
@@ -30,12 +30,14 @@
#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.
+ * Base directories that include all the files generated internally, from the
+ * root path of the new cluster. The paths are dynamically built as of
+ * BASE_OUTPUTDIR/$unix_timestamp/{LOG_OUTPUTDIR,DUMP_OUTPUTDIR} to ensure
+ * their uniqueness in each run.
*/
#define BASE_OUTPUTDIR "pg_upgrade_output.d"
-#define LOG_OUTPUTDIR BASE_OUTPUTDIR "/log"
-#define DUMP_OUTPUTDIR BASE_OUTPUTDIR "/dump"
+#define LOG_OUTPUTDIR "log"
+#define DUMP_OUTPUTDIR "dump"
#define DB_DUMP_LOG_FILE_MASK "pg_upgrade_dump_%u.log"
#define SERVER_LOG_FILE "pg_upgrade_server.log"
@@ -276,7 +278,8 @@ typedef struct
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 *rootdir; /* Root directory, aka pg_upgrade_output.d */
+ char *basedir; /* Base output directory, with timestamp */
char *dumpdir; /* Dumps */
char *logdir; /* Log files */
bool isatty; /* is stdout a tty */
@@ -432,6 +435,7 @@ void report_status(eLogType type, const char *fmt,...) pg_attribute_printf(2, 3
void pg_log(eLogType type, const char *fmt,...) pg_attribute_printf(2, 3);
void pg_fatal(const char *fmt,...) pg_attribute_printf(1, 2) pg_attribute_noreturn();
void end_progress_output(void);
+void cleanup_logs(void);
void prep_status(const char *fmt,...) pg_attribute_printf(1, 2);
void prep_status_progress(const char *fmt,...) pg_attribute_printf(1, 2);
unsigned int str2uint(const char *str);
diff --git a/src/bin/pg_upgrade/t/002_pg_upgrade.pl b/src/bin/pg_upgrade/t/002_pg_upgrade.pl
index 55c7354ba2..db939ee9a1 100644
--- a/src/bin/pg_upgrade/t/002_pg_upgrade.pl
+++ b/src/bin/pg_upgrade/t/002_pg_upgrade.pl
@@ -213,6 +213,38 @@ chdir ${PostgreSQL::Test::Utils::tmp_check};
# 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_fails(
+ [
+ 'pg_upgrade', '--no-sync',
+ '-d', $oldnode->data_dir,
+ '-D', $newnode->data_dir,
+ '-b', $oldbindir . '/does/not/exist/',
+ '-B', $newbindir,
+ '-p', $oldnode->port,
+ '-P', $newnode->port,
+ '--check'
+ ],
+ '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");
+
+# --check command works here, cleans up pg_upgrade_output.d.
+command_ok(
+ [
+ 'pg_upgrade', '--no-sync', '-d', $oldnode->data_dir,
+ '-D', $newnode->data_dir, '-b', $oldbindir,
+ '-B', $newbindir, '-p', $oldnode->port,
+ '-P', $newnode->port, '--check'
+ ],
+ 'run of pg_upgrade --check for new instance');
+ok(!-d $newnode->data_dir . "/pg_upgrade_output.d",
+ "pg_upgrade_output.d/ removed after pg_upgrade --check success");
+
+# Actual run, pg_upgrade_output.d is removed at the end.
command_ok(
[
'pg_upgrade', '--no-sync', '-d', $oldnode->data_dir,
@@ -221,6 +253,9 @@ command_ok(
'-P', $newnode->port
],
'run of pg_upgrade for new instance');
+ok( !-d $newnode->data_dir . "/pg_upgrade_output.d",
+ "pg_upgrade_output.d/ removed after pg_upgrade success");
+
$newnode->start;
# Check if there are any logs coming from pg_upgrade, that would only be
diff --git a/src/bin/pg_upgrade/util.c b/src/bin/pg_upgrade/util.c
index 1a328b4270..45ae719639 100644
--- a/src/bin/pg_upgrade/util.c
+++ b/src/bin/pg_upgrade/util.c
@@ -55,6 +55,18 @@ end_progress_output(void)
pg_log(PG_REPORT, "%-*s", MESSAGE_WIDTH, "");
}
+/*
+ * Remove any logs generated internally. To be used once when exiting.
+ */
+void
+cleanup_logs(void)
+{
+ fclose(log_opts.internal);
+
+ /* Remove dump and log files? */
+ if (!log_opts.retain)
+ (void) rmtree(log_opts.rootdir, true);
+}
/*
* prep_status
diff --git a/doc/src/sgml/ref/pgupgrade.sgml b/doc/src/sgml/ref/pgupgrade.sgml
index 8cda8d16d1..0c46707069 100644
--- a/doc/src/sgml/ref/pgupgrade.sgml
+++ b/doc/src/sgml/ref/pgupgrade.sgml
@@ -768,7 +768,8 @@ psql --username=postgres --file=script.sql postgres
<para>
<application>pg_upgrade</application> creates various working files, such
as schema dumps, stored within <literal>pg_upgrade_output.d</literal> in
- the directory of the new cluster.
+ the directory of the new cluster. Each run creates a new subdirectory named
+ with a Unix timestamp where all the generated files are stored.
</para>
<para>
signature.asc
Description: PGP signature
