Reply-To: jesper.pedersen@redhat.com
Subject: Re: pg_upgrade: Pass -j down to vacuumdb
To: Alvaro Herrera <alvherre@2ndquadrant.com>,
 "Jamison, Kirk" <k.jamison@jp.fujitsu.com>
Cc: Michael Paquier <michael@paquier.xyz>,
 Peter Eisentraut <peter.eisentraut@2ndquadrant.com>,
 "fabriziomello@gmail.com" <fabriziomello@gmail.com>,
 Tom Lane <tgl@sss.pgh.pa.us>,
 "pgsql-hackers@lists.postgresql.org" <pgsql-hackers@lists.postgresql.org>,
 Bruce Momjian <bruce@momjian.us>
References: <201902010958.cf2cbi3d2ule@alvherre.pgsql>
From: Jesper Pedersen <jesper.pedersen@redhat.com>
Organization: Red Hat
Message-ID: <bef52417-2416-cb97-afce-a351601eab43@redhat.com>
Date: Fri, 1 Feb 2019 14:14:27 -0500
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101
 Thunderbird/60.4.0
MIME-Version: 1.0
In-Reply-To: <201902010958.cf2cbi3d2ule@alvherre.pgsql>
Content-Type: multipart/mixed;
 boundary="------------1625A89444BEE0D4B5C9189B"
Content-Language: en-US
Precedence: bulk

This is a multi-part message in MIME format.
--------------1625A89444BEE0D4B5C9189B
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 7bit

Hi,

On 2/1/19 4:58 AM, Alvaro Herrera wrote:
> On 2019-Feb-01, Jamison, Kirk wrote:
> 
>> I'm not sure if misunderstood the purpose of $VACUUMDB_OPTS. I thought what
>> the other developers suggested is to utilize it so that --jobs for vacuumdb
>> would be optional and just based from user-specified option $VACUUMDB_OPTS.
>> By which it would not utilize the amount of jobs used for pg_upgrade.
>> To address your need of needing a parallel, the script would just then
>> suggest if the user wants a --jobs option. The if/else for number of jobs is
>> not needed in that case, maybe only for ifndef WIN32 else case.
> 
> No Kirk, you got it right -- (some of) those ifdefs are not needed, as
> adding the --jobs in the command line is not needed.  I do think some
> extra whitespace in the format string is needed.
> 

I'm confused by this.

The point of the patch is to highlight to the user that even though 
he/she does "pg_upgrade -j 8" the "-j 8" option isn't passed down to 
vacuumdb.

Default value is 1, so in that case the echo command doesn't highlight 
that fact, otherwise it is. The user can then cancel the script and use 
the suggested command line.

The script then executes the vaccumdb command with the $VACUUMDB_OPTS 
argument as noted in the documentation.

Sample script attached as well.

I added extra space in the --analyze-in-stages part.

Kirk, can you provide a delta patch to match what you are thinking ?

Best regards,
  Jesper


--------------1625A89444BEE0D4B5C9189B
Content-Type: text/x-patch;
 name="v7_0001-Highlight-that-the-jobs-option-isn-t-passed-down-to-.patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
 filename*0="v7_0001-Highlight-that-the-jobs-option-isn-t-passed-down-to-";
 filename*1=".patch"

From 4b5856725af5d971a26a2ba150c1067ee21bb242 Mon Sep 17 00:00:00 2001
From: jesperpedersen <jesper.pedersen@redhat.com>
Date: Fri, 21 Dec 2018 05:05:31 -0500
Subject: [PATCH] Highlight that the --jobs option isn't passed down to
 vacuumdb by default.

Author: Jesper Pedersen <jesper.pedersen@redhat.com>
---
 doc/src/sgml/ref/pgupgrade.sgml |  6 +++++-
 src/bin/pg_upgrade/check.c      | 28 ++++++++++++++++++++++------
 2 files changed, 27 insertions(+), 7 deletions(-)

diff --git a/doc/src/sgml/ref/pgupgrade.sgml b/doc/src/sgml/ref/pgupgrade.sgml
index 7e1afaf0a5..84a7ccb6f1 100644
--- a/doc/src/sgml/ref/pgupgrade.sgml
+++ b/doc/src/sgml/ref/pgupgrade.sgml
@@ -395,7 +395,11 @@ NET STOP postgresql-&majorversion;
      in parallel;  a good place to start is the maximum of the number of
      CPU cores and tablespaces.  This option can dramatically reduce the
      time to upgrade a multi-database server running on a multiprocessor
-     machine.
+     machine. Note that this option isn't passed to the
+     <application>vacuumdb</application> application by default.
+     The system environment variable <literal>VACUUMDB_OPTS</literal> can be
+     used to pass extra options to the <application>vacuumdb</application>
+     application during the script execution.
     </para>
 
     <para>
diff --git a/src/bin/pg_upgrade/check.c b/src/bin/pg_upgrade/check.c
index fc5aa7010f..570b7c9ae7 100644
--- a/src/bin/pg_upgrade/check.c
+++ b/src/bin/pg_upgrade/check.c
@@ -495,15 +495,31 @@ 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    \"%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);
+	if (user_opts.jobs <= 1)
+	{
+		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);
+	}
+	else
+	{
+		fprintf(script, "echo %s    \"%s/vacuumdb\" %s--jobs %d --all %s%s\n", ECHO_QUOTE,
+				new_cluster.bindir, user_specification.data, user_opts.jobs,
+				/* 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",
+#ifndef WIN32
+	fprintf(script, "\"%s/vacuumdb\" %s $VACUUMDB_OPTS --all --analyze-in-stages\n",
 			new_cluster.bindir, user_specification.data);
+#else
+	fprintf(script, "\"%s\\vacuumdb.exe\" %s %%VACUUMDB_OPTS%% --all --analyze-in-stages\n",
+			new_cluster.bindir, user_specification.data);
+#endif
 	/* Did we copy the free space files? */
 	if (GET_MAJOR_VERSION(old_cluster.major_version) < 804)
 		fprintf(script, "\"%s/vacuumdb\" %s--all\n", new_cluster.bindir,
-- 
2.17.2


--------------1625A89444BEE0D4B5C9189B
Content-Type: application/x-shellscript;
 name="analyze_new_cluster.sh"
Content-Transfer-Encoding: base64
Content-Disposition: attachment;
 filename="analyze_new_cluster.sh"

IyEvYmluL3NoCgplY2hvICdUaGlzIHNjcmlwdCB3aWxsIGdlbmVyYXRlIG1pbmltYWwgb3B0
aW1pemVyIHN0YXRpc3RpY3MgcmFwaWRseScKZWNobyAnc28geW91ciBzeXN0ZW0gaXMgdXNh
YmxlLCBhbmQgdGhlbiBnYXRoZXIgc3RhdGlzdGljcyB0d2ljZSBtb3JlJwplY2hvICd3aXRo
IGluY3JlYXNpbmcgYWNjdXJhY3kuICBXaGVuIGl0IGlzIGRvbmUsIHlvdXIgc3lzdGVtIHdp
bGwnCmVjaG8gJ2hhdmUgdGhlIGRlZmF1bHQgbGV2ZWwgb2Ygb3B0aW1pemVyIHN0YXRpc3Rp
Y3MuJwplY2hvCgplY2hvICdJZiB5b3UgaGF2ZSB1c2VkIEFMVEVSIFRBQkxFIHRvIG1vZGlm
eSB0aGUgc3RhdGlzdGljcyB0YXJnZXQgZm9yJwplY2hvICdhbnkgdGFibGVzLCB5b3UgbWln
aHQgd2FudCB0byByZW1vdmUgdGhlbSBhbmQgcmVzdG9yZSB0aGVtIGFmdGVyJwplY2hvICdy
dW5uaW5nIHRoaXMgc2NyaXB0IGJlY2F1c2UgdGhleSB3aWxsIGRlbGF5IGZhc3Qgc3RhdGlz
dGljcyBnZW5lcmF0aW9uLicKZWNobwoKZWNobyAnSWYgeW91IHdvdWxkIGxpa2UgZGVmYXVs
dCBzdGF0aXN0aWNzIGFzIHF1aWNrbHkgYXMgcG9zc2libGUsIGNhbmNlbCcKZWNobyAndGhp
cyBzY3JpcHQgYW5kIHJ1bjonCmVjaG8gJyAgICAiL2hvbWUvanBlZGVyc2VuL3Bnc3FsL3Bv
c3RncmVzcWwtMTIueC9iaW4vdmFjdXVtZGIiIC0tam9icyA4IC0tYWxsIC0tYW5hbHl6ZS1v
bmx5JwplY2hvCgoiL2hvbWUvanBlZGVyc2VuL3Bnc3FsL3Bvc3RncmVzcWwtMTIueC9iaW4v
dmFjdXVtZGIiICRWQUNVVU1EQl9PUFRTIC0tYWxsIC0tYW5hbHl6ZS1pbi1zdGFnZXMKZWNo
bwoKZWNobyAnRG9uZScK
--------------1625A89444BEE0D4B5C9189B--