dead code in pg_upgrade

Started by Nathan Bossart10 months ago3 messages
#1Nathan Bossart
nathandbossart@gmail.com
1 attachment(s)

While trying to decipher this comment:

/*
* Do the old cluster's per-database directories share a directory
* with a new version-specific tablespace?
*/

I noticed that the condition it's referring to

if (strlen(old_cluster.tablespace_suffix) == 0)

doesn't appear to have been reachable since support for upgrading from
pre-9.2 was removed in v15 (commit e469f0a). Before then, this case seems
to have only applied to upgrades from v8.4 or older versions.

I'm planning to commit the attached patch shortly after I double-check that
I'm not missing anything.

--
nathan

Attachments:

v1-0001-pg_upgrade-Remove-some-dead-code.patchtext/plain; charset=us-asciiDownload
From 1e7961c6e256a6e37db3fcecac2400ca7d201630 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathan@postgresql.org>
Date: Mon, 17 Mar 2025 12:29:11 -0500
Subject: [PATCH v1 1/1] pg_upgrade: Remove some dead code.

---
 src/bin/pg_upgrade/check.c | 39 +++++++-------------------------------
 1 file changed, 7 insertions(+), 32 deletions(-)

diff --git a/src/bin/pg_upgrade/check.c b/src/bin/pg_upgrade/check.c
index 88db8869b6e..d32fc3d88ec 100644
--- a/src/bin/pg_upgrade/check.c
+++ b/src/bin/pg_upgrade/check.c
@@ -936,6 +936,7 @@ create_script_for_old_cluster_deletion(char **deletion_script_file_name)
 	int			tblnum;
 	char		old_cluster_pgdata[MAXPGPATH],
 				new_cluster_pgdata[MAXPGPATH];
+	char	   *old_tblspc_suffix;
 
 	*deletion_script_file_name = psprintf("%sdelete_old_cluster.%s",
 										  SCRIPT_PREFIX, SCRIPT_EXT);
@@ -1000,39 +1001,13 @@ create_script_for_old_cluster_deletion(char **deletion_script_file_name)
 			fix_path_separator(old_cluster.pgdata), PATH_QUOTE);
 
 	/* delete old cluster's alternate tablespaces */
+	old_tblspc_suffix = pg_strdup(old_cluster.tablespace_suffix);
+	fix_path_separator(old_tblspc_suffix);
 	for (tblnum = 0; tblnum < os_info.num_old_tablespaces; tblnum++)
-	{
-		/*
-		 * Do the old cluster's per-database directories share a directory
-		 * with a new version-specific tablespace?
-		 */
-		if (strlen(old_cluster.tablespace_suffix) == 0)
-		{
-			/* delete per-database directories */
-			int			dbnum;
-
-			fprintf(script, "\n");
-
-			for (dbnum = 0; dbnum < old_cluster.dbarr.ndbs; dbnum++)
-				fprintf(script, RMDIR_CMD " %c%s%c%u%c\n", PATH_QUOTE,
-						fix_path_separator(os_info.old_tablespaces[tblnum]),
-						PATH_SEPARATOR, old_cluster.dbarr.dbs[dbnum].db_oid,
-						PATH_QUOTE);
-		}
-		else
-		{
-			char	   *suffix_path = pg_strdup(old_cluster.tablespace_suffix);
-
-			/*
-			 * Simply delete the tablespace directory, which might be ".old"
-			 * or a version-specific subdirectory.
-			 */
-			fprintf(script, RMDIR_CMD " %c%s%s%c\n", PATH_QUOTE,
-					fix_path_separator(os_info.old_tablespaces[tblnum]),
-					fix_path_separator(suffix_path), PATH_QUOTE);
-			pfree(suffix_path);
-		}
-	}
+		fprintf(script, RMDIR_CMD " %c%s%s%c\n", PATH_QUOTE,
+				fix_path_separator(os_info.old_tablespaces[tblnum]),
+				old_tblspc_suffix, PATH_QUOTE);
+	pfree(old_tblspc_suffix);
 
 	fclose(script);
 
-- 
2.39.5 (Apple Git-154)

#2Tom Lane
tgl@sss.pgh.pa.us
In reply to: Nathan Bossart (#1)
Re: dead code in pg_upgrade

Nathan Bossart <nathandbossart@gmail.com> writes:

I noticed that the condition it's referring to
if (strlen(old_cluster.tablespace_suffix) == 0)
doesn't appear to have been reachable since support for upgrading from
pre-9.2 was removed in v15 (commit e469f0a). Before then, this case seems
to have only applied to upgrades from v8.4 or older versions.

I'm planning to commit the attached patch shortly after I double-check that
I'm not missing anything.

+1. e469f0aaf just removed code that was immediately connected to a
version check; I didn't look too hard for consequent simplifications.
After looking, I concur that tablespace_suffix can never be empty
anymore. I don't see any other spots checking this condition.

regards, tom lane

#3Nathan Bossart
nathandbossart@gmail.com
In reply to: Tom Lane (#2)
Re: dead code in pg_upgrade

On Mon, Mar 17, 2025 at 01:56:49PM -0400, Tom Lane wrote:

Nathan Bossart <nathandbossart@gmail.com> writes:

I noticed that the condition it's referring to
if (strlen(old_cluster.tablespace_suffix) == 0)
doesn't appear to have been reachable since support for upgrading from
pre-9.2 was removed in v15 (commit e469f0a). Before then, this case seems
to have only applied to upgrades from v8.4 or older versions.

I'm planning to commit the attached patch shortly after I double-check that
I'm not missing anything.

+1. e469f0aaf just removed code that was immediately connected to a
version check; I didn't look too hard for consequent simplifications.
After looking, I concur that tablespace_suffix can never be empty
anymore. I don't see any other spots checking this condition.

Committed, thanks for looking.

--
nathan