Thread: pg_upgrade analyze script
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 https://postgr.es/m/CABUevEx-D0PNVe00tkeQRGennZQwDtBJn=493MJt-x6sppbUxA@mail.gmail.com.
--
Attachment
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.
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 > https://postgr.es/m/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. https://www.postgresql.org/message-id/flat/1389237323.30068.8.camel%40vanquo.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] 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/
Attachment
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
Attachment
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/
Attachment
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
Attachment
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
Attachment
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