pg_upgrade analyze script

On Tue, Oct 6, 2020 at 11:43:01AM +0200, Magnus Hagander wrote:

For a long time now, pg_upgrade drops a script (analyze_new_cluster.sh) that
just calls vacuumdb to run the analyze in stages. This script made a lot of
sense back when it actually implemented the stages, but these days since it's
just calling a single command, I think it's just unnecessary complication.

I suggest we drop it and just replace it with instructions to call vacuumdb
directly.

Uh, that script has instructions on what is running. Do we want to show
that at the end of running pg_upgrade or do people understand "stages"
by now?

Attached patch does this. It also removes the support in the instructions that
talk about pre-8.4 databases, which I believe is dead code per�https://
postgr.es/m/CABUevEx-D0PNVe00tkeQRGennZQwDtBJn=493MJt-x6sppbUxA@mail.gmail.com.

I just removed that.

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

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

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

On Tue, Oct 6, 2020 at 11:43:01AM +0200, Magnus Hagander wrote:

For a long time now, pg_upgrade drops a script (analyze_new_cluster.sh)

that

just calls vacuumdb to run the analyze in stages. This script made a lot

of

sense back when it actually implemented the stages, but these days since

it's

just calling a single command, I think it's just unnecessary

complication.

I suggest we drop it and just replace it with instructions to call

vacuumdb

directly.

Uh, that script has instructions on what is running. Do we want to show
that at the end of running pg_upgrade or do people understand "stages"
by now?

I think that's information that belongs in the documentation, for those
that case. I'm willing to bet that the large majority of the people who run
the script never read that, and certainly don't cancel it. Those that need
it to their upgrade planning long before they get to that stage.

It's already decently documented on the vacuumdb page. Maybe just add a
link to that from the pg_upgrade reference page (
https://www.postgresql.org/docs/current/pgupgrade.html under point 14)?

Attached patch does this. It also removes the support in the instructions
that

talk about pre-8.4 databases, which I believe is dead code per https://

postgr.es/m/CABUevEx-D0PNVe00tkeQRGennZQwDtBJn=493MJt-x6sppbUxA@mail.gmail.com
.

I just removed that.

Great, I'll simplify the patch if we agree that it's a good way forward,
and also add a doc reference per above.

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

On Tue, Oct 6, 2020 at 10:40:30PM +0200, Magnus Hagander wrote:

I think that's information that belongs in the documentation, for those that
case. I'm willing to bet that the large majority of the people who run the
script never read that, and certainly don't cancel it. Those that need it to
their upgrade planning long before they get to that stage.

It's already decently documented on the vacuumdb page.� Maybe just add a link
to that from the pg_upgrade reference page (https://www.postgresql.org/docs/
current/pgupgrade.html under point 14)?

Agreed.

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

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

On 2020-10-06 11:43, Magnus Hagander wrote:

For a long time now, pg_upgrade drops a script (analyze_new_cluster.sh)
that just calls vacuumdb to run the analyze in stages. This script made
a lot of sense back when it actually implemented the stages, but these
days since it's just calling a single command, I think it's just
unnecessary complication.

I suggest we drop it and just replace it with instructions to call
vacuumdb directly.

Attached patch does this. It also removes the support in the
instructions that talk about pre-8.4 databases, which I believe is dead
code per
/messages/by-id/CABUevEx-D0PNVe00tkeQRGennZQwDtBJn=493MJt-x6sppbUxA@mail.gmail.com.

I agree that the script should be removed. It makes a lot of things
simpler.

Here is the thread that proposed implementing vacuumdb-in-stages. There
were discussions then about removing the script also, but didn't come to
a conclusion.

/messages/by-id/1389237323.30068.8.camel@vanquo.pezone.net

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

Hi,

thank you for you contribution.

I did notice that the cfbot [1]http://cfbot.cputube.org/magnus-hagander.html is failing for this patch.
Please try to address the issues for the upcoming Commitfest.

Cheers,
//Georgios

[1]: http://cfbot.cputube.org/magnus-hagander.html

On Fri, Oct 30, 2020 at 5:10 PM Georgios Kokolatos
<gkokolatos@protonmail.com> wrote:

Hi,

thank you for you contribution.

I did notice that the cfbot [1] is failing for this patch.
Please try to address the issues for the upcoming Commitfest.

Thanks for the notice -- PFA a rebased version!

--
Magnus Hagander
Me: https://www.hagander.net/
Work: https://www.redpill-linpro.com/

On Mon, Nov 02, 2020 at 02:18:32PM +0100, Magnus Hagander wrote:

On Fri, Oct 30, 2020 at 5:10 PM Georgios Kokolatos
<gkokolatos@protonmail.com> wrote:

I did notice that the cfbot [1] is failing for this patch.
Please try to address the issues for the upcoming Commitfest.

Thanks for the notice -- PFA a rebased version!

No objections to remove this script from me.

I have spotted one small-ish thing. This patch is missing to update
the following code in vcregress.pl:
print "\nSetting up stats on new cluster\n\n";
system(".\\analyze_new_cluster.bat") == 0 or exit 1;
--
Michael

On Mon, Nov 9, 2020 at 8:53 AM Michael Paquier <michael@paquier.xyz> wrote:

On Mon, Nov 02, 2020 at 02:18:32PM +0100, Magnus Hagander wrote:

On Fri, Oct 30, 2020 at 5:10 PM Georgios Kokolatos
<gkokolatos@protonmail.com> wrote:

I did notice that the cfbot [1] is failing for this patch.
Please try to address the issues for the upcoming Commitfest.

Thanks for the notice -- PFA a rebased version!

No objections to remove this script from me.

I have spotted one small-ish thing. This patch is missing to update
the following code in vcregress.pl:
print "\nSetting up stats on new cluster\n\n";
system(".\\analyze_new_cluster.bat") == 0 or exit 1;

Ah, nice catch -- thanks! I guess this is unfortunately not a test
that's part of what cfbot tests.

Untested on Windows, but following the patterns of the rows before it.
I will go ahead and push this version in a bit.

--
Magnus Hagander
Me: https://www.hagander.net/
Work: https://www.redpill-linpro.com/

On Mon, Nov 9, 2020 at 11:22 AM Magnus Hagander <magnus@hagander.net> wrote:

On Mon, Nov 9, 2020 at 8:53 AM Michael Paquier <michael@paquier.xyz> wrote:

On Mon, Nov 02, 2020 at 02:18:32PM +0100, Magnus Hagander wrote:

On Fri, Oct 30, 2020 at 5:10 PM Georgios Kokolatos
<gkokolatos@protonmail.com> wrote:

I did notice that the cfbot [1] is failing for this patch.
Please try to address the issues for the upcoming Commitfest.

Thanks for the notice -- PFA a rebased version!

No objections to remove this script from me.

I have spotted one small-ish thing. This patch is missing to update
the following code in vcregress.pl:
print "\nSetting up stats on new cluster\n\n";
system(".\\analyze_new_cluster.bat") == 0 or exit 1;

Ah, nice catch -- thanks! I guess this is unfortunately not a test
that's part of what cfbot tests.

Untested on Windows, but following the patterns of the rows before it.
I will go ahead and push this version in a bit.

And now pushed, let's see if the buildfarm agrees with the Windows change...

--
Magnus Hagander
Me: https://www.hagander.net/
Work: https://www.redpill-linpro.com/

On 2020-11-09 11:22, Magnus Hagander wrote:

I have spotted one small-ish thing. This patch is missing to update
the following code in vcregress.pl:
print "\nSetting up stats on new cluster\n\n";
system(".\\analyze_new_cluster.bat") == 0 or exit 1;

Ah, nice catch -- thanks! I guess this is unfortunately not a test
that's part of what cfbot tests.

Untested on Windows, but following the patterns of the rows before it.
I will go ahead and push this version in a bit.

You should just remove those calls. There is no need to replace them
with vacuumdb calls. The reason those calls were there is that they
were testing the generated script itself. If the script is gone, there
is no more need. There are already separate tests for testing vacuumdb.

On Mon, Nov 09, 2020 at 03:47:22PM +0100, Peter Eisentraut wrote:

You should just remove those calls. There is no need to replace them with
vacuumdb calls. The reason those calls were there is that they were testing
the generated script itself. If the script is gone, there is no more need.
There are already separate tests for testing vacuumdb.

True, 102_vacuumdb_stages.pl already does all that.
--
Michael

On Tue, Nov 10, 2020 at 10:21:04AM +0900, Michael Paquier wrote:

On Mon, Nov 09, 2020 at 03:47:22PM +0100, Peter Eisentraut wrote:

You should just remove those calls. There is no need to replace them with
vacuumdb calls. The reason those calls were there is that they were testing
the generated script itself. If the script is gone, there is no more need.
There are already separate tests for testing vacuumdb.

True, 102_vacuumdb_stages.pl already does all that.

Let's fix that. From what I can see it only involves the attached,
and as a bobus it also reduces the number of extra characters showing
on my terminal after running pg_upgrade tests.
--
Michael

On Mon, Nov 9, 2020 at 3:47 PM Peter Eisentraut
<peter.eisentraut@enterprisedb.com> wrote:

On 2020-11-09 11:22, Magnus Hagander wrote:

I have spotted one small-ish thing. This patch is missing to update
the following code in vcregress.pl:
print "\nSetting up stats on new cluster\n\n";
system(".\\analyze_new_cluster.bat") == 0 or exit 1;

Ah, nice catch -- thanks! I guess this is unfortunately not a test
that's part of what cfbot tests.

Untested on Windows, but following the patterns of the rows before it.
I will go ahead and push this version in a bit.

You should just remove those calls. There is no need to replace them
with vacuumdb calls. The reason those calls were there is that they
were testing the generated script itself. If the script is gone, there
is no more need. There are already separate tests for testing vacuumdb.

Good point. Done.

--
Magnus Hagander
Me: https://www.hagander.net/
Work: https://www.redpill-linpro.com/

On Wed, Nov 11, 2020 at 2:21 AM Michael Paquier <michael@paquier.xyz> wrote:

On Tue, Nov 10, 2020 at 10:21:04AM +0900, Michael Paquier wrote:

On Mon, Nov 09, 2020 at 03:47:22PM +0100, Peter Eisentraut wrote:

You should just remove those calls. There is no need to replace them with
vacuumdb calls. The reason those calls were there is that they were testing
the generated script itself. If the script is gone, there is no more need.
There are already separate tests for testing vacuumdb.

True, 102_vacuumdb_stages.pl already does all that.

Let's fix that. From what I can see it only involves the attached,
and as a bobus it also reduces the number of extra characters showing
on my terminal after running pg_upgrade tests.

Gah. For some reason, I did not spot this email until after I had
applied the other patch. And not only that, I also missed one line in
my patch that you included in yours :/

My apologies.

--
Magnus Hagander
Me: https://www.hagander.net/
Work: https://www.redpill-linpro.com/

On Wed, Nov 11, 2020 at 04:51:27PM +0100, Magnus Hagander wrote:

Gah. For some reason, I did not spot this email until after I had
applied the other patch. And not only that, I also missed one line in
my patch that you included in yours :/

My apologies.

No worries, thanks.
--
Michael