Date: Fri, 9 Oct 2020 14:53:25 -0400
From: Bruce Momjian <bruce@momjian.us>
To: Justin Pryzby <pryzby@telsasoft.com>
Cc: pgsql-hackers@postgresql.org
Subject: Re: pg_upgrade: fail early if a tablespace dir already exists for
 new cluster version
Message-ID: <20201009185325.GA24870@momjian.us>
References: <20200925005531.GJ23631@telsasoft.com>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="G4iJoqBmSsgzjUCe"
Content-Disposition: inline
In-Reply-To: <20200925005531.GJ23631@telsasoft.com>
User-Agent: Mutt/1.10.1 (2018-07-13)
Precedence: bulk


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

On Thu, Sep 24, 2020 at 07:55:31PM -0500, Justin Pryzby wrote:
> This should be caught during --check, or earlier in the upgrade, rather than
> only after dumping the schema.
> 
> Typically, the tablespace dir would be left behind by a previous failed upgrade
> attempt, causing a cascade of failured upgrades.  I guess I may not be the only
> one with a 3 year old wrapper which has a hack to check for this.

This is an interesting failure case I never considered --- running
pg_upgrade, having it fail, deleting and recreating the _new_ cluster
directory, but not removing the new cluster's tablepace directories.  I
was able to recreate the failure, and verify that your patch properly
throws an error during 'check' for this case.

Modified patch attached.  I plan to apply this to all supported Postgres
versions.

-- 
  Bruce Momjian  <bruce@momjian.us>        https://momjian.us
  EnterpriseDB                             https://enterprisedb.com

  The usefulness of a cup is in its emptiness, Bruce Lee


--G4iJoqBmSsgzjUCe
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment; filename="tablespace.diff"

diff --git a/src/bin/pg_upgrade/check.c b/src/bin/pg_upgrade/check.c
new file mode 100644
index 168058a..0c845b7
*** a/src/bin/pg_upgrade/check.c
--- b/src/bin/pg_upgrade/check.c
*************** static void check_for_tables_with_oids(C
*** 27,32 ****
--- 27,33 ----
  static void check_for_reg_data_type_usage(ClusterInfo *cluster);
  static void check_for_jsonb_9_4_usage(ClusterInfo *cluster);
  static void check_for_pg_role_prefix(ClusterInfo *cluster);
+ static void check_for_new_tablespace_dir(ClusterInfo *new_cluster);
  static char *get_canonical_locale_name(int category, const char *locale);
  
  
*************** check_new_cluster(void)
*** 187,192 ****
--- 188,195 ----
  	check_is_install_user(&new_cluster);
  
  	check_for_prepared_transactions(&new_cluster);
+ 
+ 	check_for_new_tablespace_dir(&new_cluster);
  }
  
  
*************** create_script_for_cluster_analyze(char *
*** 528,533 ****
--- 531,568 ----
  
  
  /*
+  * Check that tablespace directories do not already exist for new cluster.
+  * If they do, it would cause an error while restoring global objects.
+  * This allows the failure to be detected at check time, rather than
+  * during schema restore.
+  *
+  * Note, v8.4 has no tablespace_suffix, which is fine as long as the new
+  * cluster version has a suffix.
+  */
+ static void
+ check_for_new_tablespace_dir(ClusterInfo *new_cluster)
+ {
+ 	char	new_tablespace_dir[MAXPGPATH];
+ 
+  	prep_status("Checking for new cluster tablespace directories");
+ 
+ 	for (int 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_tablespace_dir);
+ 	}
+ 
+ 	check_ok();
+ }
+ 
+ /*
   * create_script_for_old_cluster_deletion()
   *
   *	This is particularly useful for tablespace deletion.

--G4iJoqBmSsgzjUCe--