pg_upgrade does not completely honor --new-port

On Tue, Sep 25, 2012 at 05:36:54PM +0300, Devrim Gunduz wrote:

Hi,

I just performed a test upgrade from 9.1 to 9.2, and used --new-port
variable. However, the analyze_new_cluster.sh does not include the new
port, thus when I run it, it fails. Any chance to add the port number to
the script?

Well, the reason people normally use the port number is to do a live
check, but obviously when the script is created it isn't doing a check.
I am worried that if I do embed the port number in there, then if they
change the port after the upgrade, they now can't use the script. I
assume users would have PGPORT set before running the script, no?

Also, is it worth to add the value specified in --new-bindir as a prefix
to vacuumdb command in the same script?

Wow, I never thought of adding a path to those scripts, but it certainly
makes sense. I have created the attached patch which does this.

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

+ It's impossible for everything to be true. +

Applied to head and 9.2.

---------------------------------------------------------------------------

On Wed, Sep 26, 2012 at 10:06:50PM -0400, Bruce Momjian wrote:

On Tue, Sep 25, 2012 at 05:36:54PM +0300, Devrim Gunduz wrote:

Hi,

I just performed a test upgrade from 9.1 to 9.2, and used --new-port
variable. However, the analyze_new_cluster.sh does not include the new
port, thus when I run it, it fails. Any chance to add the port number to
the script?

Well, the reason people normally use the port number is to do a live
check, but obviously when the script is created it isn't doing a check.
I am worried that if I do embed the port number in there, then if they
change the port after the upgrade, they now can't use the script. I
assume users would have PGPORT set before running the script, no?

Also, is it worth to add the value specified in --new-bindir as a prefix
to vacuumdb command in the same script?

Wow, I never thought of adding a path to those scripts, but it certainly
makes sense. I have created the attached patch which does this.

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

+ It's impossible for everything to be true. +

diff --git a/contrib/pg_upgrade/check.c b/contrib/pg_upgrade/check.c
index bed10f8..2785eb7 100644
--- a/contrib/pg_upgrade/check.c
+++ b/contrib/pg_upgrade/check.c
@@ -477,7 +477,7 @@ create_script_for_cluster_analyze(char **analyze_script_file_name)
ECHO_QUOTE, ECHO_QUOTE);
fprintf(script, "echo %sthis script and run:%s\n",
ECHO_QUOTE, ECHO_QUOTE);
-	fprintf(script, "echo %s    vacuumdb --all %s%s\n", ECHO_QUOTE,
+	fprintf(script, "echo %s    \"%s/vacuumdb\" --all %s%s\n", ECHO_QUOTE, new_cluster.bindir,
/* Did we copy the free space files? */
(GET_MAJOR_VERSION(old_cluster.major_version) >= 804) ?
"--analyze-only" : "--analyze", ECHO_QUOTE);
@@ -498,7 +498,7 @@ create_script_for_cluster_analyze(char **analyze_script_file_name)
ECHO_QUOTE, ECHO_QUOTE);
fprintf(script, "echo %s--------------------------------------------------%s\n",
ECHO_QUOTE, ECHO_QUOTE);
-	fprintf(script, "vacuumdb --all --analyze-only\n");
+	fprintf(script, "\"%s/vacuumdb\" --all --analyze-only\n", new_cluster.bindir);
fprintf(script, "echo%s\n", ECHO_BLANK);
fprintf(script, "echo %sThe server is now available with minimal optimizer statistics.%s\n",
ECHO_QUOTE, ECHO_QUOTE);
@@ -519,7 +519,7 @@ create_script_for_cluster_analyze(char **analyze_script_file_name)
ECHO_QUOTE, ECHO_QUOTE);
fprintf(script, "echo %s---------------------------------------------------%s\n",
ECHO_QUOTE, ECHO_QUOTE);
-	fprintf(script, "vacuumdb --all --analyze-only\n");
+	fprintf(script, "\"%s/vacuumdb\" --all --analyze-only\n", new_cluster.bindir);
fprintf(script, "echo%s\n\n", ECHO_BLANK);

#ifndef WIN32
@@ -532,7 +532,7 @@ create_script_for_cluster_analyze(char **analyze_script_file_name)
ECHO_QUOTE, ECHO_QUOTE);
fprintf(script, "echo %s-------------------------------------------------------------%s\n",
ECHO_QUOTE, ECHO_QUOTE);
-	fprintf(script, "vacuumdb --all %s\n",
+	fprintf(script, "\"%s/vacuumdb\" --all %s\n", new_cluster.bindir,
/* Did we copy the free space files? */
(GET_MAJOR_VERSION(old_cluster.major_version) >= 804) ?
"--analyze-only" : "--analyze");

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

+ It's impossible for everything to be true. +

Hi,

On Wed, 2012-09-26 at 22:06 -0400, Bruce Momjian wrote:

I just performed a test upgrade from 9.1 to 9.2, and used
--new-port variable. However, the analyze_new_cluster.sh does not
include the new port, thus when I run it, it fails. Any chance to
add the port number to the script?

Well, the reason people normally use the port number is to do a live
check, but obviously when the script is created it isn't doing a
check. I am worried that if I do embed the port number in there, then
if they change the port after the upgrade, they now can't use the
script. I assume users would have PGPORT set before running the
script, no?

They can't use the script in each way -- at least we can make it usable
for one case, I think.

Regards,

--
Devrim GÜNDÜZ
Principal Systems Engineer @ EnterpriseDB: http://www.enterprisedb.com
PostgreSQL Danışmanı/Consultant, Red Hat Certified Engineer
Community: devrim~PostgreSQL.org, devrim.gunduz~linux.org.tr
http://www.gunduz.org Twitter: http://twitter.com/devrimgunduz

Excerpts from Devrim GÜNDÜZ's message of mié oct 03 17:00:16 -0300 2012:

Hi,

On Wed, 2012-09-26 at 22:06 -0400, Bruce Momjian wrote:

I just performed a test upgrade from 9.1 to 9.2, and used
--new-port variable. However, the analyze_new_cluster.sh does not
include the new port, thus when I run it, it fails. Any chance to
add the port number to the script?

Well, the reason people normally use the port number is to do a live
check, but obviously when the script is created it isn't doing a
check. I am worried that if I do embed the port number in there, then
if they change the port after the upgrade, they now can't use the
script. I assume users would have PGPORT set before running the
script, no?

They can't use the script in each way -- at least we can make it usable
for one case, I think.

Well, you could have the script set the port number only if the variable
is not set from the calling shell ... you know,
PGPORT=${PGPORT:=the_other_number} . That way, if the user wants to
specify a different port, they have to set PGPORT before calling the
script.

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Wed, Oct 3, 2012 at 11:00:16PM +0300, Devrim Gunduz wrote:

Hi,

On Wed, 2012-09-26 at 22:06 -0400, Bruce Momjian wrote:

I just performed a test upgrade from 9.1 to 9.2, and used
--new-port variable. However, the analyze_new_cluster.sh does not
include the new port, thus when I run it, it fails. Any chance to
add the port number to the script?

Well, the reason people normally use the port number is to do a live
check, but obviously when the script is created it isn't doing a
check. I am worried that if I do embed the port number in there, then
if they change the port after the upgrade, they now can't use the
script. I assume users would have PGPORT set before running the
script, no?

They can't use the script in each way -- at least we can make it usable
for one case, I think.

Well, my assumption is that they are unlikely to move the old _binary_
directory, but they are more likely to change the port number. My point
is that if they change the port number to the default from a
non-default, or they set the PGPORT environment variable, the script
will work. If we hard-code the port, it would not work.

In fact, pg_upgrade defaults to use port 50432 if they don't supply one.
We would embed the port number only if they supplied a custom port
number, but again, they might change that before going live with the new
server.

I guess I am confused why you would use pg_upgrade, and start the new
server on a non-default port that isn't the same as PGPORT.

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

+ It's impossible for everything to be true. +

On Wed, Oct 3, 2012 at 05:16:55PM -0300, Alvaro Herrera wrote:

Excerpts from Devrim G�ND�Z's message of mi� oct 03 17:00:16 -0300 2012:

Hi,

On Wed, 2012-09-26 at 22:06 -0400, Bruce Momjian wrote:

I just performed a test upgrade from 9.1 to 9.2, and used
--new-port variable. However, the analyze_new_cluster.sh does not
include the new port, thus when I run it, it fails. Any chance to
add the port number to the script?

Well, the reason people normally use the port number is to do a live
check, but obviously when the script is created it isn't doing a
check. I am worried that if I do embed the port number in there, then
if they change the port after the upgrade, they now can't use the
script. I assume users would have PGPORT set before running the
script, no?

They can't use the script in each way -- at least we can make it usable
for one case, I think.

Well, you could have the script set the port number only if the variable
is not set from the calling shell ... you know,
PGPORT=${PGPORT:=the_other_number} . That way, if the user wants to
specify a different port, they have to set PGPORT before calling the
script.

Good idea, but that is only going to work on Unix, and in fact only
using certain shells. I don't think we want to go there, do we? I
could expand that out to a normal shell _if_ statement, but again, only
works on Unix.

What we _could_ do is to add a comment line at the top that defines a
string that can be supplied, and default it to the port number; that
would work on Unix and Windows, e.g.

# uncomment and adjust if you want a special port number
# PGPORT_STR="-p 5435"
# export PGPORT

For Windows it would be "REM". Is everyone happy with that?

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

+ It's impossible for everything to be true. +