Date: Sat, 11 Dec 2021 20:50:17 -0600
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
Message-ID: <20211212025017.GN17618@telsasoft.com>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="s33SFO/vI16wUVFN"
Content-Disposition: inline
User-Agent: Mutt/1.9.4 (2018-02-28)
Archived-At: 
 <https://www.postgresql.org/message-id/20211212025017.GN17618%40telsasoft.com>
Precedence: bulk


--s33SFO/vI16wUVFN
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

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 "

--s33SFO/vI16wUVFN
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
 filename="0001-pg_upgrade-fail-if-logfiles-exist.patch"

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.c
index 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


--s33SFO/vI16wUVFN--