From: Justin Pryzby <pryzby@telsasoft.com>
To: Bruce Momjian <bruce@momjian.us>
Cc: pgsql-hackers@lists.postgresql.org
Subject: pg_upgrade should truncate/remove its logs before running
Date: Sat, 11 Dec 2021 20:50:17 -0600
Message-ID: <20211212025017.GN17618@telsasoft.com> (raw)
I have seen this numerous times but had not dug into it, until now.
If pg_upgrade fails and is re-run, it appends to its logfiles, which is
confusing since, if it fails again, it then looks like the original error
recurred and wasn't fixed. The "append" behavior dates back to 717f6d608.
I think it should either truncate the logfiles, or error early if any of the
files exist. Or it could put all its output files into a newly-created
subdirectory. Or this message could be output to the per-db logfiles, and not
just the static ones:
| "pg_upgrade run on %s".
For the per-db logfiels with OIDs in their name, changing open() from "append"
mode to truncate mode doesn't work, since they're written to in parallel.
They have to be removed/truncated in advance.
This is one possible fix. You can test its effect by deliberately breaking one
of the calls to exec_progs(), like this.
- "\"%s/pg_restore\" %s %s --exit-on-error --verbose "
+ "\"%s/pg_restore\" %s %s --exit-on-error --verboose "
Attachments:0001-pg_upgrade-fail-if-logfiles-exist.patch (text/x-diff, 2.0K) download
From 1b1a86df1febce5f55257c625b81e1499f6bfce3 Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryzbyj@telsasoft.com>
Date: Sat, 11 Dec 2021 19:19:50 -0600
Subject: [PATCH] pg_upgrade: fail if logfiles exist..
Rather than appending to them, which is confusing since old errors are still
present.
---
src/bin/pg_upgrade/check.c | 22 ++++++++++++++++++++++
1 file changed, 22 insertions(+)
diff --git a/src/bin/pg_upgrade/check.c b/src/bin/pg_upgrade/check.cindex 1106912863..92060e5abe 100644--- a/src/bin/pg_upgrade/check.c+++ b/src/bin/pg_upgrade/check.c@@ -18,6 +18,7 @@ static void check_new_cluster_is_empty(void);
static void check_databases_are_compatible(void);
static void check_locale_and_encoding(DbInfo *olddb, DbInfo *newdb);
static bool equivalent_locale(int category, const char *loca, const char *locb);
+static void remove_logfiles(void);
static void check_is_install_user(ClusterInfo *cluster);
static void check_proper_datallowconn(ClusterInfo *cluster);
static void check_for_prepared_transactions(ClusterInfo *cluster);
@@ -168,7 +169,10 @@ check_and_dump_old_cluster(bool live_check)
* the old server is running.
*/
if (!user_opts.check)
+ {+ remove_logfiles();
generate_old_dump();
+ }
if (!live_check)
stop_postmaster(false);
@@ -635,6 +639,24 @@ create_script_for_old_cluster_deletion(char **deletion_script_file_name)
check_ok();
}
+/*+ * Remove any per-db logfiles left around by a previous upgrade attempt.+ * The statically-named logfiles aren't touched, since there are clear+ * separators added by parseCommandLine().+ */+static void+remove_logfiles(void)+{+ int dbnum;+ for (dbnum = 0; dbnum < old_cluster.dbarr.ndbs; dbnum++)+ {+ char log_file_name[MAXPGPATH];+ DbInfo *old_db = &old_cluster.dbarr.dbs[dbnum];+ snprintf(log_file_name, sizeof(log_file_name), DB_DUMP_LOG_FILE_MASK, old_db->db_oid);+ if (unlink(log_file_name) < 0 && errno != ENOENT)+ pg_log(PG_FATAL, "failed to remove pre-existing logfile: %s: %m", log_file_name);+ }+}
/*
* check_is_install_user()
--
2.17.0
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: pryzby@telsasoft.com, bruce@momjian.us, pgsql-hackers@lists.postgresql.org
Subject: Re: pg_upgrade should truncate/remove its logs before running
In-Reply-To: <20211212025017.GN17618@telsasoft.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox