pg_upgrade dead code for pre-8.4 versions

Started by Magnus Haganderover 5 years ago4 messages
#1Magnus Hagander
magnus@hagander.net

pg_upgrade documentation says it supports upgrades from 8.4 and newer.

But there is code in there that makes a check and differs pre-8.4 from
8.4-or-newer.

ISTM that code could be cleaned out, because it should be dead based on the
initial check that we are upgrading from 8.4 or later?

This appears to be:
* The code in the analyze script (see a follow-up email on this), which
actually tries to generate a vacuum script if it's a pre-8.4 without
visibility map
* The file copy code in relfilenode.c that copies vm and fsm files only
from 8.4 and up (which should now be always)

Or am I misreading something?

//Magnus

#2Daniel Gustafsson
daniel@yesql.se
In reply to: Magnus Hagander (#1)
Re: pg_upgrade dead code for pre-8.4 versions

On 6 Oct 2020, at 11:27, Magnus Hagander <magnus@hagander.net> wrote:

ISTM that code could be cleaned out, because it should be dead based on the initial check that we are upgrading from 8.4 or later?

+1. Commit 2209b3923a7afe0b removed support for 8.3 so anything left checking
for pre-8.4 should be removed as a pre-8.4 source cluster couldn't be upgraded
with pg_upgrade today.

cheers ./daniel

#3Bruce Momjian
bruce@momjian.us
In reply to: Daniel Gustafsson (#2)
1 attachment(s)
Re: pg_upgrade dead code for pre-8.4 versions

On Tue, Oct 6, 2020 at 11:40:57AM +0200, Daniel Gustafsson wrote:

On 6 Oct 2020, at 11:27, Magnus Hagander <magnus@hagander.net> wrote:

ISTM that code could be cleaned out, because it should be dead based on the initial check that we are upgrading from 8.4 or later?

+1. Commit 2209b3923a7afe0b removed support for 8.3 so anything left checking
for pre-8.4 should be removed as a pre-8.4 source cluster couldn't be upgraded
with pg_upgrade today.

OK, fixed with the attached patch, applied to all branches. There was
code that tested for >= 8.4 (useless test) or < 8.4 (dead code). I
also adjusted the version tests to be consistent.

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

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

Attachments:

pg_upgrade.difftext/x-diff; charset=us-asciiDownload
diff --git a/src/bin/pg_upgrade/check.c b/src/bin/pg_upgrade/check.c
new file mode 100644
index 89f3b0d..168058a
*** a/src/bin/pg_upgrade/check.c
--- b/src/bin/pg_upgrade/check.c
*************** void
*** 234,251 ****
  output_completion_banner(char *analyze_script_file_name,
  						 char *deletion_script_file_name)
  {
! 	/* Did we copy the free space files? */
! 	if (GET_MAJOR_VERSION(old_cluster.major_version) >= 804)
! 		pg_log(PG_REPORT,
! 			   "Optimizer statistics are not transferred by pg_upgrade so,\n"
! 			   "once you start the new server, consider running:\n"
! 			   "    %s\n\n", analyze_script_file_name);
! 	else
! 		pg_log(PG_REPORT,
! 			   "Optimizer statistics and free space information are not transferred\n"
! 			   "by pg_upgrade so, once you start the new server, consider running:\n"
! 			   "    %s\n\n", analyze_script_file_name);
! 
  
  	if (deletion_script_file_name)
  		pg_log(PG_REPORT,
--- 234,243 ----
  output_completion_banner(char *analyze_script_file_name,
  						 char *deletion_script_file_name)
  {
! 	pg_log(PG_REPORT,
! 		   "Optimizer statistics are not transferred by pg_upgrade so,\n"
! 		   "once you start the new server, consider running:\n"
! 		   "    %s\n\n", analyze_script_file_name);
  
  	if (deletion_script_file_name)
  		pg_log(PG_REPORT,
*************** create_script_for_cluster_analyze(char *
*** 510,528 ****
  			ECHO_QUOTE, ECHO_QUOTE);
  	fprintf(script, "echo %sthis script and run:%s\n",
  			ECHO_QUOTE, ECHO_QUOTE);
! 	fprintf(script, "echo %s    \"%s/vacuumdb\" %s--all %s%s\n", ECHO_QUOTE,
! 			new_cluster.bindir, user_specification.data,
! 	/* Did we copy the free space files? */
! 			(GET_MAJOR_VERSION(old_cluster.major_version) >= 804) ?
! 			"--analyze-only" : "--analyze", ECHO_QUOTE);
  	fprintf(script, "echo%s\n\n", ECHO_BLANK);
  
  	fprintf(script, "\"%s/vacuumdb\" %s--all --analyze-in-stages\n",
  			new_cluster.bindir, user_specification.data);
- 	/* Did we copy the free space files? */
- 	if (GET_MAJOR_VERSION(old_cluster.major_version) <= 803)
- 		fprintf(script, "\"%s/vacuumdb\" %s--all\n", new_cluster.bindir,
- 				user_specification.data);
  
  	fprintf(script, "echo%s\n\n", ECHO_BLANK);
  	fprintf(script, "echo %sDone%s\n",
--- 502,513 ----
  			ECHO_QUOTE, ECHO_QUOTE);
  	fprintf(script, "echo %sthis script and run:%s\n",
  			ECHO_QUOTE, ECHO_QUOTE);
! 	fprintf(script, "echo %s    \"%s/vacuumdb\" %s--all --analyze-only%s\n", ECHO_QUOTE,
! 			new_cluster.bindir, user_specification.data, ECHO_QUOTE);
  	fprintf(script, "echo%s\n\n", ECHO_BLANK);
  
  	fprintf(script, "\"%s/vacuumdb\" %s--all --analyze-in-stages\n",
  			new_cluster.bindir, user_specification.data);
  
  	fprintf(script, "echo%s\n\n", ECHO_BLANK);
  	fprintf(script, "echo %sDone%s\n",
diff --git a/src/bin/pg_upgrade/relfilenode.c b/src/bin/pg_upgrade/relfilenode.c
new file mode 100644
index af9a021..f76ddaa
*** a/src/bin/pg_upgrade/relfilenode.c
--- b/src/bin/pg_upgrade/relfilenode.c
*************** transfer_single_new_db(FileNameMap *maps
*** 163,178 ****
  			/* transfer primary file */
  			transfer_relfile(&maps[mapnum], "", vm_must_add_frozenbit);
  
! 			/* fsm/vm files added in PG 8.4 */
! 			if (GET_MAJOR_VERSION(old_cluster.major_version) >= 804)
! 			{
! 				/*
! 				 * Copy/link any fsm and vm files, if they exist
! 				 */
! 				transfer_relfile(&maps[mapnum], "_fsm", vm_must_add_frozenbit);
! 				if (vm_crashsafe_match)
! 					transfer_relfile(&maps[mapnum], "_vm", vm_must_add_frozenbit);
! 			}
  		}
  	}
  }
--- 163,174 ----
  			/* transfer primary file */
  			transfer_relfile(&maps[mapnum], "", vm_must_add_frozenbit);
  
! 			/*
! 			 * Copy/link any fsm and vm files, if they exist
! 			 */
! 			transfer_relfile(&maps[mapnum], "_fsm", vm_must_add_frozenbit);
! 			if (vm_crashsafe_match)
! 				transfer_relfile(&maps[mapnum], "_vm", vm_must_add_frozenbit);
  		}
  	}
  }
#4Magnus Hagander
magnus@hagander.net
In reply to: Bruce Momjian (#3)
Re: pg_upgrade dead code for pre-8.4 versions

On Tue, Oct 6, 2020 at 8:45 PM Bruce Momjian <bruce@momjian.us> wrote:

On Tue, Oct 6, 2020 at 11:40:57AM +0200, Daniel Gustafsson wrote:

On 6 Oct 2020, at 11:27, Magnus Hagander <magnus@hagander.net> wrote:

ISTM that code could be cleaned out, because it should be dead based

on the initial check that we are upgrading from 8.4 or later?

+1. Commit 2209b3923a7afe0b removed support for 8.3 so anything left

checking

for pre-8.4 should be removed as a pre-8.4 source cluster couldn't be

upgraded

with pg_upgrade today.

OK, fixed with the attached patch, applied to all branches. There was
code that tested for >= 8.4 (useless test) or < 8.4 (dead code). I
also adjusted the version tests to be consistent.

Thanks!

And yeah, I noticed the inconsistency in the checking as well. Getting that
cleaned up was good.

--
Magnus Hagander
Me: https://www.hagander.net/ <http://www.hagander.net/&gt;
Work: https://www.redpill-linpro.com/ <http://www.redpill-linpro.com/&gt;