Date: Tue, 20 Oct 2020 18:06:38 -0500
From: Justin Pryzby <pryzby@telsasoft.com>
To: Bruce Momjian <bruce@momjian.us>
Cc: pgsql-hackers@postgresql.org
Subject: Re: pg_upgrade: fail early if a tablespace dir already exists for
 new cluster version
Message-ID: <20201020230638.GI9241@telsasoft.com>
References: <20200925005531.GJ23631@telsasoft.com>
 <20201009185325.GA24870@momjian.us>
 <20201009192310.GS17626@telsasoft.com>
 <20201009234251.GA8419@momjian.us>
 <20201015233530.GG3797@momjian.us>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="Cp3Cp8fzgozWLBWL"
Content-Disposition: inline
In-Reply-To: <20201015233530.GG3797@momjian.us>
User-Agent: Mutt/1.9.4 (2018-02-28)
Precedence: bulk


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

On Thu, Oct 15, 2020 at 07:35:30PM -0400, Bruce Momjian wrote:
> On Fri, Oct  9, 2020 at 07:42:51PM -0400, Bruce Momjian wrote:
> > On Fri, Oct  9, 2020 at 02:23:10PM -0500, Justin Pryzby wrote:
> > > In my local branch, I had revised this comment to say:
> > > 
> > > + * Note, v8.4 has no tablespace_suffix, which is fine so long as the version we
> > > + * being upgraded *to* has a suffix, since it's not allowed to pg_upgrade from
> > > + * a version to the same version if tablespaces are in use.
> > 
> > OK, updated patch attached.  Also, from your original patch, I didn't
> > need to call canonicalize_path() since we are not comparing paths, and I
> > didn't need to include common/relpath.h.  I also renamed a variable.
> 
> Patch applied to all supported versions.  Thanks for the report, and the

I wonder if pg_upgrade should try to rmdir() the tablespace dirs before
restoring global objects, allowing it to succeed, rather than just "failing
early".  That can avoid the need to re-create the new cluster, which I imagine
in some scenarios might be bad enough to require aborting the upgrade.
I propose only for master branch.

This doesn't avoid the need to re-create the new cluster if anything has been
restored into it (eg. if pg_tablespace is populated), it just cleans out any
empty dirs left behind by a previous pg_upgrade which failed after "restoring
global objects" but before copying/linking data into that tablespace.

|rm -fr pgsql12.dat pgsql14.dat tblspc;
|/usr/lib/postgresql/12/bin/initdb -D pgsql12.dat
|./tmp_install/usr/local/pgsql/bin/initdb -D pgsql14.dat
|mkdir tblspc tblspc/PG_14_202010201
|echo "CREATE TABLESPACE one LOCATION '`pwd`/tblspc'" |/usr/lib/postgresql/12/bin/postgres --single postgres -D pgsql12.dat -p 5678 -k /tmp
|./tmp_install/usr/local/pgsql/bin/pg_upgrade -D pgsql14.dat/ -d pgsql12.dat -b /usr/lib/postgresql/12/bin

-- 
Justin

--Cp3Cp8fzgozWLBWL
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
 filename="v3-0001-Avoid-failure-if-tablespace-dirs-exist-and-empty.patch"

From 2eb5d39bc38474f3fe956d0f3344c47257133afd Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryzbyj@telsasoft.com>
Date: Tue, 20 Oct 2020 17:30:54 -0500
Subject: [PATCH v3] Avoid failure if tablespace dirs exist and empty..

See also: 3c0471b5fdd847febb30ec805a3e44f3eac5b66f
---
 src/bin/pg_upgrade/check.c      | 23 ++++++++++++++++-------
 src/bin/pg_upgrade/pg_upgrade.c | 13 +++++++++++++
 2 files changed, 29 insertions(+), 7 deletions(-)

diff --git a/src/bin/pg_upgrade/check.c b/src/bin/pg_upgrade/check.c
index 05e6bf7f2c..57f9fa9ad6 100644
--- a/src/bin/pg_upgrade/check.c
+++ b/src/bin/pg_upgrade/check.c
@@ -531,9 +531,9 @@ create_script_for_cluster_analyze(char **analyze_script_file_name)
 
 
 /*
- * A previous run of pg_upgrade might have failed and the new cluster
- * directory recreated, but they might have forgotten to remove
- * the new cluster's tablespace directories.  Therefore, check that
+ * A previous run of pg_upgrade might have failed.
+ * The admin might ahve recreated the new cluster directory, but forgotten to
+ * remove the new cluster's tablespace directories.  Therefore, check that
  * new cluster tablespace directories do not already exist.  If
  * they do, it would cause an error while restoring global objects.
  * This allows the failure to be detected at check time, rather than
@@ -554,15 +554,24 @@ check_for_new_tablespace_dir(ClusterInfo *new_cluster)
 
 	for (tblnum = 0; tblnum < os_info.num_old_tablespaces; tblnum++)
 	{
-		struct stat statbuf;
-
 		snprintf(new_tablespace_dir, MAXPGPATH, "%s%s",
 				os_info.old_tablespaces[tblnum],
 				new_cluster->tablespace_suffix);
 
-		if (stat(new_tablespace_dir, &statbuf) == 0 || errno != ENOENT)
-			pg_fatal("new cluster tablespace directory already exists: \"%s\"\n",
+		/*
+		 * New tablepace dirs are expected to not exist; if they
+		 * do exist but are empty, we avoid failing here, and remove them
+		 * before creating tablespaces while "restoring global objects".
+		 */
+		switch (pg_check_dir(new_tablespace_dir))
+		{
+			case 0: /* Nonextant */
+			case 1: /* Empty */
+				break;
+			default:
+				pg_fatal("new cluster tablespace directory already exists and nonempty\"%s\"\n",
 					 new_tablespace_dir);
+		}
 	}
 
 	check_ok();
diff --git a/src/bin/pg_upgrade/pg_upgrade.c b/src/bin/pg_upgrade/pg_upgrade.c
index 1bc86e4205..6b7f8a690a 100644
--- a/src/bin/pg_upgrade/pg_upgrade.c
+++ b/src/bin/pg_upgrade/pg_upgrade.c
@@ -300,6 +300,19 @@ prepare_new_globals(void)
 	 */
 	set_frozenxids(false);
 
+	/*
+	 * Remove any *empty* tablespace dirs left behind by a previous, failed
+	 * upgrade.
+	 */
+	for (int tblnum = 0; tblnum < os_info.num_old_tablespaces; tblnum++)
+	{
+		char	new_tablespace_dir[MAXPGPATH];
+		snprintf(new_tablespace_dir, MAXPGPATH, "%s%s",
+				os_info.old_tablespaces[tblnum],
+				new_cluster.tablespace_suffix);
+		(void) rmdir(new_tablespace_dir);
+	}
+
 	/*
 	 * Now restore global objects (roles and tablespaces).
 	 */
-- 
2.17.0


--Cp3Cp8fzgozWLBWL--