Обсуждение: pg_upgrade: Pass -j down to vacuumdb

Hi Hackers,

Here is a patch that passes the -j option from pg_upgrade down to 
vacuumdb if supported.

I'll add it to the January 'Fest.

Thanks for considering !

Best regards,
  Jesper

On 21/12/2018 11:12, Jesper Pedersen wrote:
> Here is a patch that passes the -j option from pg_upgrade down to 
> vacuumdb if supported.

It's not clear to me that that is what is usually wanted.

The point of the post-upgrade analyze script is specifically to do the
analyze in a gentle fashion.  Mixing in parallelism would seem to defeat
that a bit.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Hi,

On 12/29/18 10:03 AM, Peter Eisentraut wrote:
> On 21/12/2018 11:12, Jesper Pedersen wrote:
>> Here is a patch that passes the -j option from pg_upgrade down to
>> vacuumdb if supported.
> 
> It's not clear to me that that is what is usually wanted.
> 
> The point of the post-upgrade analyze script is specifically to do the
> analyze in a gentle fashion.  Mixing in parallelism would seem to defeat
> that a bit.
> 

Well, that really depends. The user passed -j to pg_upgrade in order for 
the upgrade to happen faster, so maybe they would expect, as I would, 
that the ANALYZE phase would happen in parallel too.

At least there should be a "notice" about what the script will do in 
this case.

Best regards,
  Jesper

On 02/01/2019 20:47, Jesper Pedersen wrote:
> Well, that really depends. The user passed -j to pg_upgrade in order for 
> the upgrade to happen faster, so maybe they would expect, as I would, 
> that the ANALYZE phase would happen in parallel too.

pg_upgrade -j reduces the *downtime* caused by pg_upgrade during the
upgrade process.  Also, during said downtime, nothing else is happening,
so you can use all the resources of the machine.

Once the system is back up, you don't necessarily want to use all the
resources.  The analyze script is specifically written to run while
production traffic is active.  If you just want to run the analyze as
fast as possible, you can just run vacuumdb -j directly, without using
the script.

Moreover, it's not clear that pg_upgrade and vacuumdb are bound the same
way, so it's not a given that the same -j number should be used.

Perhaps more documentation would be useful here.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:

> On 02/01/2019 20:47, Jesper Pedersen wrote:
>
>> Well, that really depends. The user passed -j to pg_upgrade in order for 
>> the upgrade to happen faster, so maybe they would expect, as I would, 
>> that the ANALYZE phase would happen in parallel too.
>
> pg_upgrade -j reduces the *downtime* caused by pg_upgrade during the
> upgrade process.  Also, during said downtime, nothing else is happening,
> so you can use all the resources of the machine.
>
> Once the system is back up, you don't necessarily want to use all the
> resources.  The analyze script is specifically written to run while
> production traffic is active.  If you just want to run the analyze as
> fast as possible, you can just run vacuumdb -j directly, without using
> the script.

Peter, I'm skeptical here.

I might permit a connection to a just pg_upgraded DB prior to any
analyze being known finished only for the most trivial case.  At my site
however, *trivial* systems are a small minority.

In fact, our automated upgrade workflow uses our home-built parallel
analyzer which predates vacuumdb -j.  Apps are not allowed into the DB
until a fast 1st pass has been done.

We run it in 2 phases...

$all preceeding upgrade steps w/system locked out
analyze-lite (reduced stats target)
open DB for application traffic
analyze-full

Of course we are increasing downtime by disallowing app traffic till
finish of analyze-lite however the assumption is that many queries would
be too slow to attempt without full analyzer coverage, albiet at a
reduced stats target.

IMO this is certainly a case of no 1-size-fits-all solution so perhaps a
--analyze-jobs option :-)

FWIW

Thanks

> Moreover, it's not clear that pg_upgrade and vacuumdb are bound the same
> way, so it's not a given that the same -j number should be used.
>
> Perhaps more documentation would be useful here.

-- 
Jerry Sievers
Postgres DBA/Development Consulting
e: postgres.consulting@comcast.net

Hi Peter,

On 1/3/19 7:03 AM, Peter Eisentraut wrote:
> Perhaps more documentation would be useful here.
> 

Here is v2 that just notes that the -j option isn't passed down.

Best regards,
  Jesper

Hi,

According to CF app, this patch needs review so I took a look at it.
Currently, your patch applies and builds cleanly, and all tests are also successful
based from the CF bot patch tester.

I'm not sure if I have understood the argument raised by Peter correctly. 
Quoting Peter, "it's not clear that pg_upgrade and vacuumdb are bound the same way, so it's not a given that the same
-jnumber should be used."
 
I think it's whether the # jobs for pg_upgrade is used the same way for parallel vacuum. 

According to the official docs, the recommended setting for pg_upgrade --j option is the maximum of the number of CPU
coresand tablespaces. [1]
 
As for vacuumdb, parallel vacuum's (-j) recommended setting is based from your max_connections [2], which is the max #
ofconcurrent connections to your db server.
 

[1] https://www.postgresql.org/docs/current/pgupgrade.html
[2] https://www.postgresql.org/docs/current/app-vacuumdb.html


Regards,
Kirk Jamison

On Fri, Jan 25, 2019 at 02:31:57AM +0000, Jamison, Kirk wrote:
> I'm not sure if I have understood the argument raised by Peter
> correctly.  Quoting Peter, "it's not clear that pg_upgrade and
> vacuumdb are bound the same way, so it's not a given that the same
> -j number should be used."  I think it's whether the # jobs for
> pg_upgrade is used the same way for parallel vacuum.

vacuumdb and pg_upgrade are designed for different purposes and have
different properties, hence using a value of -j for one does not
necessarily mean that the same value should be used for the other.
An ANALYZE is nice because it is non-intrusive for a live production
system, and if you begin to pass down a -j argument you may finish by
eating more resources that would have been necessary for the same
job.  For some users perhaps that matters, for some it does not.  So
based on that linking the value used by pg_upgrade and vacuumdb is a
bad concept in my opinion, and the patch should be rejected.  More
documentation on pg_upgrade side to explain that a bit better could be
a good idea though, as it is perfectly possible to use your own
post-upgrade script or rewrite partially the generated one.
--
Michael

On 25/01/2019 11:28, Michael Paquier wrote:
> based on that linking the value used by pg_upgrade and vacuumdb is a
> bad concept in my opinion, and the patch should be rejected.  More
> documentation on pg_upgrade side to explain that a bit better could be
> a good idea though, as it is perfectly possible to use your own
> post-upgrade script or rewrite partially the generated one.

Right.  pg_upgrade doesn't actually call vacuumdb.  It creates a script
that you may use.  The script itself contains a comment that says, if
you want to do this as fast as possible, don't use this script.  That
comment could be enhanced to suggest the use of the -j option.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Hi Kirk,

On 1/24/19 9:31 PM, Jamison, Kirk wrote:
> According to CF app, this patch needs review so I took a look at it.
> Currently, your patch applies and builds cleanly, and all tests are also successful
> based from the CF bot patch tester.
> 
> I'm not sure if I have understood the argument raised by Peter correctly.
> Quoting Peter, "it's not clear that pg_upgrade and vacuumdb are bound the same way, so it's not a given that the same
-jnumber should be used."
 
> I think it's whether the # jobs for pg_upgrade is used the same way for parallel vacuum.
> 
> According to the official docs, the recommended setting for pg_upgrade --j option is the maximum of the number of CPU
coresand tablespaces. [1]
 
> As for vacuumdb, parallel vacuum's (-j) recommended setting is based from your max_connections [2], which is the max
#of concurrent connections to your db server.
 
>

Thanks for your feedback !

As per Peter's comments I have changed the patch (v2) to not pass down 
the -j option to vacuumdb.

Only an update to the documentation and console output is made in order 
to make it more clear.

Best regards,
  Jesper

On 2019-Jan-25, Peter Eisentraut wrote:

> On 25/01/2019 11:28, Michael Paquier wrote:
> > based on that linking the value used by pg_upgrade and vacuumdb is a
> > bad concept in my opinion, and the patch should be rejected.  More
> > documentation on pg_upgrade side to explain that a bit better could be
> > a good idea though, as it is perfectly possible to use your own
> > post-upgrade script or rewrite partially the generated one.
> 
> Right.  pg_upgrade doesn't actually call vacuumdb.  It creates a script
> that you may use.  The script itself contains a comment that says, if
> you want to do this as fast as possible, don't use this script.  That
> comment could be enhanced to suggest the use of the -j option.

So let's have it write with a $VACUUMDB_OPTS variable, which is by
default defined as empty but with a comment suggesting that maybe the
user wants to add the -j option.  This way, if they have to edit it,
they only have to edit the VACUUMDB_OPTS line instead of each of the two
vacuumdb lines.

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

Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> So let's have it write with a $VACUUMDB_OPTS variable, which is by
> default defined as empty but with a comment suggesting that maybe the
> user wants to add the -j option.  This way, if they have to edit it,
> they only have to edit the VACUUMDB_OPTS line instead of each of the two
> vacuumdb lines.

Even better, allow the script to absorb a value from the environment,
so that it needn't be edited at all.

            regards, tom lane

On Fri, Jan 25, 2019 at 12:16:49PM -0500, Tom Lane wrote:
> Alvaro Herrera <alvherre@2ndquadrant.com> writes:
>> So let's have it write with a $VACUUMDB_OPTS variable, which is by
>> default defined as empty but with a comment suggesting that maybe the
>> user wants to add the -j option.  This way, if they have to edit it,
>> they only have to edit the VACUUMDB_OPTS line instead of each of the two
>> vacuumdb lines.
>
> Even better, allow the script to absorb a value from the environment,
> so that it needn't be edited at all.

+1.
--
Michael

Hi Jesper,

On Friday, January 25, 2019, Jesper Pedersen <jesper.pedersen@redhat.com> 
> Thanks for your feedback !
>
> As per Peter's comments I have changed the patch (v2) to not pass down the -j option to vacuumdb.
>
> Only an update to the documentation and console output is made in order to make it more clear.

Yeah, I understood from the references that the parallel option use
for vacuumdb and pg_upgrade are different. I was also getting
clarification whether pg_upgrade gets to pass it down to vacuumdb.
Based from other Michael's and Peter's replies, I now understand it.

There were also helpful comments from the developers above,
pointing it to the right direction.
In addition to Peter's comment, quoting "...if you want to do this
as fast as possible, don't use this script.  That comment could be
enhanced to suggest the use of the -j option.", so I think you 
should also update the following script in create_script_for_cluster_analyze():
     fprintf(script, "echo %sIf you would like default statistics as quickly as possible, cancel%s\n",
             ECHO_QUOTE, ECHO_QUOTE);

There were also advice from Tom and Alvaro that you can incorporate
further in your next patch.

>Alvaro Herrera <alvherre@2ndquadrant.com> writes:
>> So let's have it write with a $VACUUMDB_OPTS variable, which is by 
>> default defined as empty but with a comment suggesting that maybe the 
>> user wants to add the -j option.  This way, if they have to edit it, 
>> they only have to edit the VACUUMDB_OPTS line instead of each of the 
>> two vacuumdb lines.
>
Tom Lane wrote:
>Even better, allow the script to absorb a value from the environment, so that it needn't be edited at all.

Regards,
Kirk Jamison

Hi,

On 1/27/19 7:50 PM, Jamison, Kirk wrote:
> There were also helpful comments from the developers above,
> pointing it to the right direction.
> In addition to Peter's comment, quoting "...if you want to do this
> as fast as possible, don't use this script.  That comment could be
> enhanced to suggest the use of the -j option.", so I think you
> should also update the following script in create_script_for_cluster_analyze():
>       fprintf(script, "echo %sIf you would like default statistics as quickly as possible, cancel%s\n",
>               ECHO_QUOTE, ECHO_QUOTE);
>

Yes, it will echo the -j option if passed, and the server supports it.

>> Alvaro Herrera <alvherre@2ndquadrant.com> writes:
>>> So let's have it write with a $VACUUMDB_OPTS variable, which is by
>>> default defined as empty but with a comment suggesting that maybe the
>>> user wants to add the -j option.  This way, if they have to edit it,
>>> they only have to edit the VACUUMDB_OPTS line instead of each of the
>>> two vacuumdb lines.
>>
> Tom Lane wrote:
>> Even better, allow the script to absorb a value from the environment, so that it needn't be edited at all.
> 

Done.

Attached is v3.

Best regards,
  Jesper

On 1/28/19 11:30 AM, Fabrízio de Royes Mello wrote:
> IMHO you should use long option name '--jobs' like others.
>

Thanks for your feedback !

Done, and v4 attached.

Best regards,
  Jesper

On 28/01/2019 17:47, Jesper Pedersen wrote:
> On 1/28/19 11:30 AM, Fabrízio de Royes Mello wrote:
>> IMHO you should use long option name '--jobs' like others.
>>
> 
> Thanks for your feedback !
> 
> Done, and v4 attached.

I would drop the changes in pgupgrade.sgml.  We don't need to explain
what doesn't happen, when nobody before now ever thought that it would
happen.

Also, we don't need the versioning stuff.  The new cluster in pg_upgrade
is always of the current version, and we know what that one supports.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 1/28/19 3:50 PM, Peter Eisentraut wrote:
>> Done, and v4 attached.
> 
> I would drop the changes in pgupgrade.sgml.  We don't need to explain
> what doesn't happen, when nobody before now ever thought that it would
> happen.
> 
> Also, we don't need the versioning stuff.  The new cluster in pg_upgrade
> is always of the current version, and we know what that one supports.
> 

Done.

Best regards,
  Jesper

Hi Jesper,

Thanks for updating your patches quickly.

>On 1/28/19 3:50 PM, Peter Eisentraut wrote:
>>> Done, and v4 attached.
>> 
>> I would drop the changes in pgupgrade.sgml.  We don't need to explain 
>> what doesn't happen, when nobody before now ever thought that it would 
>> happen.
>> 
>> Also, we don't need the versioning stuff.  The new cluster in 
>> pg_upgrade is always of the current version, and we know what that one supports.
>> 

>Done.

I just checked the patch.
As per advice, you removed the versioning and specified --jobs. 
The patch still applies, builds and passed the tests successfully.

I'd argue though that it's helpful to find a short documentation to clarify
that it's not pass down by to vacuumdb default. I think the initial doc 
change from the previous patch would be good.
+     machine. Note, that this option isn't passed to the
+     <application>vacuumdb</application> application by default.
Just remove the comma symbol from "Note, that..."


Regards,
Kirk Jamison

On Tue, Jan 29, 2019 at 12:35:30AM +0000, Jamison, Kirk wrote:
> I just checked the patch.
> As per advice, you removed the versioning and specified --jobs.
> The patch still applies, builds and passed the tests successfully.

I would document the optional VACUUM_OPTS on the page of pg_upgrade.
If Peter thinks it is fine to not do so, that's fine for me as well.

It seems to me that the latest patch sent is incorrect for multiple
reasons:
1) You still enforce -j to use the number of jobs that the caller of
pg_upgrade provides, and we agreed that both things are separate
concepts upthread, no?  What has been suggested by Alvaro is to add a
comment so as one can use VACUUM_OPTS with -j optionally, instead of
suggesting a full-fledged vacuumdb command which depends on what
pg_upgrade uses.  So there is no actual need for the if/else
complication business.
2) Perhaps we need to worry about the second vacuumdb --all command,
which may want custom options, which are not necessarily the same as
the options of the first command?  I don't think we need to care as it
applies only to an upgraded cluster using something older 8.4, just
wondering..
--
Michael

Hi,

On 1/29/19 12:08 AM, Michael Paquier wrote:
> On Tue, Jan 29, 2019 at 12:35:30AM +0000, Jamison, Kirk wrote:
>> I just checked the patch.
>> As per advice, you removed the versioning and specified --jobs.
>> The patch still applies, builds and passed the tests successfully.
> 
> I would document the optional VACUUM_OPTS on the page of pg_upgrade.
> If Peter thinks it is fine to not do so, that's fine for me as well.
> 

I added most of the documentation back, as requested by Kirk.

> It seems to me that the latest patch sent is incorrect for multiple
> reasons:
> 1) You still enforce -j to use the number of jobs that the caller of
> pg_upgrade provides, and we agreed that both things are separate
> concepts upthread, no?  What has been suggested by Alvaro is to add a
> comment so as one can use VACUUM_OPTS with -j optionally, instead of
> suggesting a full-fledged vacuumdb command which depends on what
> pg_upgrade uses.  So there is no actual need for the if/else
> complication business.

I think it is ok for the echo command to highlight to the user that 
running --analyze-only using the same amount of jobs will give a faster 
result.

> 2) Perhaps we need to worry about the second vacuumdb --all command,
> which may want custom options, which are not necessarily the same as
> the options of the first command?  I don't think we need to care as it
> applies only to an upgraded cluster using something older 8.4, just
> wondering..

I think that --all --analyze-in-stages is what most people want. And 
with the $VACUUMDB_OPTS variable people have an option to put in more, 
such as -j X.

Best regards,
  Jesper

On January 29, 2019 8:19 PM +0000, Jesper Pedersen wrote:
>On 1/29/19 12:08 AM, Michael Paquier wrote:
>> I would document the optional VACUUM_OPTS on the page of pg_upgrade.
>> If Peter thinks it is fine to not do so, that's fine for me as well.
>> 
..
>I added most of the documentation back, as requested by Kirk.

Actually, I find it useful to be documented. However, major contributors have higher opinions in terms of experience,
soI think it's alright with me not to include the doc part if they finally say so.
 

>> It seems to me that the latest patch sent is incorrect for multiple
>> reasons:
>> 1) You still enforce -j to use the number of jobs that the caller of 
>> pg_upgrade provides, and we agreed that both things are separate 
>> concepts upthread, no?  What has been suggested by Alvaro is to add a 
>> comment so as one can use VACUUM_OPTS with -j optionally, instead of 
>> suggesting a full-fledged vacuumdb command which depends on what 
>> pg_upgrade uses.  So there is no actual need for the if/else 
>> complication business.

> I think it is ok for the echo command to highlight to the user that running --analyze-only using the same amount of
jobswill give a faster result.
 

I missed that part. 
IIUC, using the $VACUUMDB_OPTS variable would simplify and correct the usage of jobs for vacuumdb. (pg_upgrade jobs is
notequal to vacuumdb jobs) Plus, it might simplify and reduce the number of additional lines.
 
Tom Lane also suggested above to make the script absorb the value from env.
Would that address your concern of getting a faster result, yes?


Regards,
Kirk Jamison

Hi,

On 1/30/19 7:59 PM, Jamison, Kirk wrote:
>> I added most of the documentation back, as requested by Kirk.
> 
> Actually, I find it useful to be documented. However, major contributors have higher opinions in terms of experience,
soI think it's alright with me not to include the doc part if they finally say so.
 
> 

I think we need to leave it to the Committer to decide, as both Peter  
and Michael are committers; provided the patch reaches RfC.

>>> It seems to me that the latest patch sent is incorrect for multiple
>>> reasons:
>>> 1) You still enforce -j to use the number of jobs that the caller of
>>> pg_upgrade provides, and we agreed that both things are separate
>>> concepts upthread, no?  What has been suggested by Alvaro is to add a
>>> comment so as one can use VACUUM_OPTS with -j optionally, instead of
>>> suggesting a full-fledged vacuumdb command which depends on what
>>> pg_upgrade uses.  So there is no actual need for the if/else
>>> complication business.
> 
>> I think it is ok for the echo command to highlight to the user that running --analyze-only using the same amount of
jobswill give a faster result.
 
> 
> I missed that part.
> IIUC, using the $VACUUMDB_OPTS variable would simplify and correct the usage of jobs for vacuumdb. (pg_upgrade jobs
isnot equal to vacuumdb jobs) Plus, it might simplify and reduce the number of additional lines.
 
> Tom Lane also suggested above to make the script absorb the value from env.
> Would that address your concern of getting a faster result, yes?
> 

The actual line in the script executed is using $VACUUMDB_OPTS at the  
moment, so could you expand on the above ? The 'if' could be removed if  
we assume that pg_upgrade would only be used with server > 9.5, but to  
be safer I would leave it in, as it is only console output.

Best regards,
  Jesper

On January 31, 2019, 9:29PM +0000, Jesper Pedersen wrote:

>>> I added most of the documentation back, as requested by Kirk.
>> 
>> Actually, I find it useful to be documented. However, major contributors have higher opinions in terms of
experience,so I think it's alright with me not to include the doc part if they finally say so. 
 
>
> I think we need to leave it to the Committer to decide, as both Peter and Michael are committers; provided the patch
reachesRfC.
 

Agreed.

>>> 1) You still enforce -j to use the number of jobs that the caller of 
>>> pg_upgrade provides, and we agreed that both things are separate 
>>> concepts upthread, no?  What has been suggested by Alvaro is to add 
>>> a comment so as one can use VACUUM_OPTS with -j optionally, instead 
>>> of suggesting a full-fledged vacuumdb command which depends on what 
>>> pg_upgrade uses.  So there is no actual need for the if/else 
>>> complication business.
> 
>> I think it is ok for the echo command to highlight to the user that 
>> running --analyze-only using the same amount of jobs will give a faster result.

Since you used user_opts.jobs (which is coming from pg_upgrade, is it not?),
could you clarify more the statement above? Or did you mean somehow that
it won't be a problem for vacuumdb to use the same?
Though correctness-wise is arguable, if the committers can let it pass
from your answer, then I think it's alright.

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.

Regards,
Kirk Jamison

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.

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

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

Hi,

On 2019-02-01 14:14:27 -0500, Jesper Pedersen wrote:

> 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.

This is currently marked as waiting for author, which doesn't seem
accurate. I've set it to needs review, and moved it to the next
commitfest - please revert if that's not accurate.

- Andres

Hi, 

On February 1, 2019 8:14 PM +0000, Jesper Pedersen wrote:

> 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.


> 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'tpassed 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
cancelthe 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.

Sorry I think I might have understood now where you are coming from,
where your script clarifies that it's not necessarily passed down.
If committers can let it pass, maybe it's necessary to highlight in the script,
Something like:
"If you would like default statistics as quickly as possible (e.g. run in parallel)..."

The difference is still perhaps your use of --jobs option
as somehow "explicitly" embedded in the code, compared to the suggestion of
making it optional by using the $VACUUMDB_OPTS variable, so that
jobs need not to be explicitly written, unless the user wants to.
(which I also think it's a better improvement because it's optional)

Some quick code check. sorry, don't have much time to write a patch for it today.
Perhaps you could write it similar to what you did in the --analyze-in-stages part.
Maybe something like (not sure if correct):
+    fprintf(script, "echo %s    \"%s/vacuumdb\" %s $VACUUMDB_OPTS --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);

I'll continue to review though from time to time.

Regards,
Kirk Jamison

Hi Jesper,

Sorry I almost completely forgot to get back to you on this.
Actually your patch works when I tested it before,
and I understand the intention.
. 
Although a point was raised by other developers by making
--jobs optional in the suggested line by using the env variable instead.

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

I was thinking of something like the attached, 
but the user needs to edit the variable value though. 
I am not sure how to implement the suggestion to allow the 
script to absorb a value from the environment,
so that it doesn't need to be edited at all.

Regards,
Kirk Jamison

On Mon, Mar 11, 2019 at 10:05 PM Jamison, Kirk <k.jamison@jp.fujitsu.com> wrote:
> I was thinking of something like the attached,

+     machine. Note that this option isn't passed to the
+     <application>vacuumdb</application> application by default.

The words 'by default' should be removed here, because there is also
no non-default way to get that behavior, either.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Hi,

Thanks Kirk !

On 3/12/19 2:20 PM, Robert Haas wrote:
> The words 'by default' should be removed here, because there is also
> no non-default way to get that behavior, either.
> 

Here is v9 based on Kirk's and your input.

Best regards,
  Jesper

Hi Jesper,

> Thanks Kirk !
>
> > On 3/12/19 2:20 PM, Robert Haas wrote:
> > The words 'by default' should be removed here, because there is also 
> > no non-default way to get that behavior, either.
> > 
> 
> Here is v9 based on Kirk's and your input.

Thanks! Although there were trailing whitespaces.
After fixing that, it works as intended (built & passed tests successfully).
That aside, the v9 patch looks good and comment was addressed.


Regards,
Kirk Jamison

Jesper Pedersen <jesper.pedersen@redhat.com> writes:
> [ v9_0001-Highlight-that-the-jobs-option-isn-t-passed-down-to-.patch ]

Now that I've looked at this, it seems like it's fairly confused about
what the proposed VACUUMDB_OPTS variable would actually do for you.
If you're going to run vacuumdb directly, it hardly seems like you'd
bother with setting such a variable; you'd just enter the options you
want directly on the command line.

Conversely, if what you plan to do is set VACUUMDB_OPTS and then
run this script, the fact that --analyze-in-stages is hard-wired
into the script's invocation doesn't seem right either: you probably
don't want that if your goal is to get done as fast as possible.

In short, I'm not convinced that most of this patch is an improvement
on the status quo.  I think we'd be best off to just take the idea
of explicitly mentioning adding --jobs to a manual run, ie roughly

    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 %sYou may wish to add --jobs=N for parallel analyzing.%s\n",
+            ECHO_QUOTE, ECHO_QUOTE);
    fprintf(script, "echo%s\n\n", ECHO_BLANK);

    fprintf(script, "\"%s/vacuumdb\" %s--all --analyze-in-stages\n",


            regards, tom lane

On Mon, Mar 25, 2019 at 05:57:50PM -0400, Tom Lane wrote:
> In short, I'm not convinced that most of this patch is an improvement
> on the status quo.  I think we'd be best off to just take the idea
> of explicitly mentioning adding --jobs to a manual run, ie roughly

Yeah, no objections from here to keep that stuff the simpler the
better.  So I am on board with your suggestion.
--
Michael

On Tuesday, March 26, 2019 3:20 AM, Michael Paquier <michael@paquier.xyz> wrote:

> On Mon, Mar 25, 2019 at 05:57:50PM -0400, Tom Lane wrote:
>
> > In short, I'm not convinced that most of this patch is an improvement
> > on the status quo. I think we'd be best off to just take the idea
> > of explicitly mentioning adding --jobs to a manual run, ie roughly
>
> Yeah, no objections from here to keep that stuff the simpler the
> better. So I am on board with your suggestion.

+1, I'd rather have that as "documentation" after an upgrade.

cheers ./daniel

On 2019-03-25 22:57, Tom Lane wrote:
> +    fprintf(script, "echo %sYou may wish to add --jobs=N for parallel analyzing.%s\n",
> +            ECHO_QUOTE, ECHO_QUOTE);

But then you get that information after you have already started the script.

I don't find any information about this analyze business on the
pg_upgrade reference page.  Maybe a discussion there could explain the
different paths better than making the output script extra complicated.

Essentially: If you want a slow and gentle analyze, use the supplied
script.  If you want a fast analyze, use vacuumdb, perhaps with an
appropriate --jobs option.  Note that pg_upgrade --jobs and vacuumdb
--jobs are resource-bound in different ways, so the same value might not
be appropriate for both.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
> On 2019-03-25 22:57, Tom Lane wrote:
>> +    fprintf(script, "echo %sYou may wish to add --jobs=N for parallel analyzing.%s\n",
>> +            ECHO_QUOTE, ECHO_QUOTE);

> But then you get that information after you have already started the script.

Yes, but that's already true of all the info that this script prints out.

> I don't find any information about this analyze business on the
> pg_upgrade reference page.  Maybe a discussion there could explain the
> different paths better than making the output script extra complicated.

> Essentially: If you want a slow and gentle analyze, use the supplied
> script.  If you want a fast analyze, use vacuumdb, perhaps with an
> appropriate --jobs option.  Note that pg_upgrade --jobs and vacuumdb
> --jobs are resource-bound in different ways, so the same value might not
> be appropriate for both.

I have no objection to handling it that way (i.e., just as a doc fix).
Or we could do both.

            regards, tom lane

On 2019-03-28 02:43, Jeff Janes wrote:
> At first blush I thought it was obvious that you would not want to run
> analyze-in-stages in parallel.  But after thinking about it some more
> and reflecting on experience doing some troublesome upgrades, I would
> reverse that and say it is now obvious you do want at least the first
> stage of analyze-in-stages, and probably the first two, to run in
> parallel.  That is not currently an option it supports, so we can't
> really recommend it in the script or the docs.

So do you think we should copy down the -j option from pg_upgrade, or
make some other arrangement?

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Wed, Apr 03, 2019 at 04:42:14PM -0400, Jeff Janes wrote:
> So maybe the first stage could
> be run by pg_upgrade itself, while the new server is still running on a
> linux socket in a private directory.

I think that would take too long.  It would be less of an issue if there was
feedback/progress from pg_upgrade during the analyze.

For our upgrades (which typically take ~15min but several customers take up to
~60min), I only analyze base tables (essentially, those which are neither
parents nor children), then start services, then ANALYZE with default stats
target.  I would want to avoid delaying services restart for more than another
(say) 5 minutes, and I would want to avoid even that unless there was a
progress report indicating that it's projected to take only a few more minutes.

I just did a test on one of our large-but-not-huge customers.  With
stats_target=1, analyzing a 145GB partitioned table looks like it'll take
perhaps an hour; they have ~1TB data, so delaying services during ANALYZE would
nullify the utility of pg_upgrade.  I can restore the essential tables from
backup in 15-30 minutes.

It might be fine if pg_upgrade took an option which enabled analyze, perhaps
instead of outputting analyze_new_cluster.sh.  But actually, a problem with
*that* is that currently pg_upgrade avoids starting the new cluster.  That
seems to be deliberate, since, with --link, that's an irreversible operation:
it's unsafe to start the old cluster afterwards.

Tangent: I have a queued mail from ~15 months ago wherein I proposed adding to
pg_upgrade an option to remove the old data dir (or probably only the files
associated with known relations).  I realized at the time that would be pretty
scary without having first verified that the new cluster at least starts.  I'm
not sure how good an idea that is, but --startnewcluster would be needed there,
too.

Justin

On 2019-04-03 23:24, Justin Pryzby wrote:
> I just did a test on one of our large-but-not-huge customers.  With
> stats_target=1, analyzing a 145GB partitioned table looks like it'll take
> perhaps an hour; they have ~1TB data, so delaying services during ANALYZE would
> nullify the utility of pg_upgrade.  I can restore the essential tables from
> backup in 15-30 minutes.

I think the real fix is to have pg_upgrade copy over the statistics.
They are right there after all, just take them.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
> I think the real fix is to have pg_upgrade copy over the statistics.
> They are right there after all, just take them.

Well, it's not "just take them", we need to develop some infrastructure
that will permit coping with cross-version differences in the stats
storage.  This isn't insoluble, but it will take some work.  We had
a prior discussion that trailed off without much result:

https://www.postgresql.org/message-id/flat/724322880.K8vzik8zPz%40abook

            regards, tom lane