Обсуждение: vacuumdb and new VACUUM options
Hi, vacuumdb command supports the corresponding options to any VACUUM parameters except INDEX_CLEANUP and TRUNCATE that were added recently. Should vacuumdb also support those new parameters, i.e., add --index-cleanup and --truncate options to the command? Regards, -- Fujii Masao
On Wed, May 8, 2019 at 2:41 AM Fujii Masao <masao.fujii@gmail.com> wrote: > > Hi, > > vacuumdb command supports the corresponding options to > any VACUUM parameters except INDEX_CLEANUP and TRUNCATE > that were added recently. Should vacuumdb also support those > new parameters, i.e., add --index-cleanup and --truncate options > to the command? I think it's a good idea to add new options of these parameters for vacuumdb. While making INDEX_CLEANUP option patch I also attached the patch for INDEX_CLEANUP parameter before[1], although it adds --disable-index-cleanup option instead. [1] 0002 patch on https://www.postgresql.org/message-id/CAD21AoBtM%3DHGLkMKBgch37mf0-epa3_o%3DY1PU0_m9r5YmtS-NQ%40mail.gmail.com Regards, -- Masahiko Sawada NIPPON TELEGRAPH AND TELEPHONE CORPORATION NTT Open Source Software Center
On Wed, May 08, 2019 at 09:26:35AM +0900, Masahiko Sawada wrote: > I think it's a good idea to add new options of these parameters for > vacuumdb. While making INDEX_CLEANUP option patch I also attached the > patch for INDEX_CLEANUP parameter before[1], although it adds > --disable-index-cleanup option instead. I have added an open item for that. I think that we should added these options. -- Michael
Вложения
On Wed, May 8, 2019 at 9:06 AM Michael Paquier <michael@paquier.xyz> wrote: > > On Wed, May 08, 2019 at 09:26:35AM +0900, Masahiko Sawada wrote: > > I think it's a good idea to add new options of these parameters for > > vacuumdb. While making INDEX_CLEANUP option patch I also attached the > > patch for INDEX_CLEANUP parameter before[1], although it adds > > --disable-index-cleanup option instead. > > I have added an open item for that. I think that we should added > these options. +1, and thanks for adding the open item!
On Wed, May 8, 2019 at 9:32 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> On Wed, May 8, 2019 at 2:41 AM Fujii Masao <masao.fujii@gmail.com> wrote:
> >
> > Hi,
> >
> > vacuumdb command supports the corresponding options to
> > any VACUUM parameters except INDEX_CLEANUP and TRUNCATE
> > that were added recently. Should vacuumdb also support those
> > new parameters, i.e., add --index-cleanup and --truncate options
> > to the command?
>
> I think it's a good idea to add new options of these parameters for
> vacuumdb. While making INDEX_CLEANUP option patch I also attached the
> patch for INDEX_CLEANUP parameter before[1], although it adds
> --disable-index-cleanup option instead.
Regarding INDEX_CLEANUP, now VACUUM has three modes;
(1) VACUUM (INDEX_CLEANUP on) does index cleanup
whatever vacuum_index_cleanup reloption is.
(2) VACUUM (INDEX_CLEANUP off) does not do index cleanup
whatever vacuum_index_cleanup reloption is.
(3) plain VACUUM decides whether to do index cleanup
according to vacuum_index_cleanup reloption.
If no option for index cleanup is specified, vacuumdb command
should work in the mode (3). IMO this is intuitive.
The question is; we should support vacuumdb option for (1), i.e.,,
something like --index-cleanup option is added?
Or for (2), i.e., something like --disable-index-cleanup option is added
as your patch does? Or for both?
Regards,
--
Fujii Masao
Em qua, 8 de mai de 2019 às 14:19, Fujii Masao <masao.fujii@gmail.com> escreveu: > > The question is; we should support vacuumdb option for (1), i.e.,, > something like --index-cleanup option is added? > Or for (2), i.e., something like --disable-index-cleanup option is added > as your patch does? Or for both? > --index-cleanup=BOOL -- Euler Taveira Timbira - http://www.timbira.com.br/ PostgreSQL: Consultoria, Desenvolvimento, Suporte 24x7 e Treinamento
On Wed, May 08, 2019 at 06:21:09PM -0300, Euler Taveira wrote: > Em qua, 8 de mai de 2019 às 14:19, Fujii Masao <masao.fujii@gmail.com> escreveu: >> The question is; we should support vacuumdb option for (1), i.e.,, >> something like --index-cleanup option is added? >> Or for (2), i.e., something like --disable-index-cleanup option is added >> as your patch does? Or for both? > > --index-cleanup=BOOL I agree with Euler's suggestion to have a 1-1 mapping between the option of vacuumdb and the VACUUM parameter, because that's more intuitive: - --index-cleanup=3Dfalse =3D> VACUUM (INDEX_CLEANUP=3Dfalse) - --index-cleanup=3Dtrue =3D> VACUUM (INDEX_CLEANUP=3Dtrue) - no --index-cleanup means to rely on the reloption. -- Michael
Вложения
On Thu, May 9, 2019 at 10:01 AM Michael Paquier <michael@paquier.xyz> wrote: > > On Wed, May 08, 2019 at 06:21:09PM -0300, Euler Taveira wrote: > > Em qua, 8 de mai de 2019 às 14:19, Fujii Masao <masao.fujii@gmail.com> escreveu: > >> The question is; we should support vacuumdb option for (1), i.e.,, > >> something like --index-cleanup option is added? > >> Or for (2), i.e., something like --disable-index-cleanup option is added > >> as your patch does? Or for both? > > > > --index-cleanup=BOOL > > I agree with Euler's suggestion to have a 1-1 mapping between the > option of vacuumdb and the VACUUM parameter +1. Attached the draft version patches for both options. Regards, -- Masahiko Sawada NIPPON TELEGRAPH AND TELEPHONE CORPORATION NTT Open Source Software Center
Вложения
At Thu, 9 May 2019 20:14:51 +0900, Masahiko Sawada <sawada.mshk@gmail.com> wrote in
<CAD21AoBmA9H3ZRuQFF+9io9PKhP+ePS=D+ThZ6ohRMdBm2x8Pw@mail.gmail.com>
> On Thu, May 9, 2019 at 10:01 AM Michael Paquier <michael@paquier.xyz> wrote:
> >
> > On Wed, May 08, 2019 at 06:21:09PM -0300, Euler Taveira wrote:
> > > Em qua, 8 de mai de 2019 às 14:19, Fujii Masao <masao.fujii@gmail.com> escreveu:
> > >> The question is; we should support vacuumdb option for (1), i.e.,,
> > >> something like --index-cleanup option is added?
> > >> Or for (2), i.e., something like --disable-index-cleanup option is added
> > >> as your patch does? Or for both?
> > >
> > > --index-cleanup=BOOL
> >
> > I agree with Euler's suggestion to have a 1-1 mapping between the
> > option of vacuumdb and the VACUUM parameter
>
> +1. Attached the draft version patches for both options.
+ printf(_(" --index-cleanup=BOOLEAN do or do not index vacuuming and index cleanup\n"));
+ printf(_(" --truncate=BOOLEAN do or do not truncate off empty pages at the end of the table\n"));
I *feel* that force/inhibit is suitable than true/false for the
options.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
On Thu, May 9, 2019 at 1:39 PM Kyotaro HORIGUCHI
<horiguchi.kyotaro@lab.ntt.co.jp> wrote:
>
> At Thu, 9 May 2019 20:14:51 +0900, Masahiko Sawada <sawada.mshk@gmail.com> wrote in
<CAD21AoBmA9H3ZRuQFF+9io9PKhP+ePS=D+ThZ6ohRMdBm2x8Pw@mail.gmail.com>
> > On Thu, May 9, 2019 at 10:01 AM Michael Paquier <michael@paquier.xyz> wrote:
> > >
> > > On Wed, May 08, 2019 at 06:21:09PM -0300, Euler Taveira wrote:
> > > > Em qua, 8 de mai de 2019 às 14:19, Fujii Masao <masao.fujii@gmail.com> escreveu:
> > > >> The question is; we should support vacuumdb option for (1), i.e.,,
> > > >> something like --index-cleanup option is added?
> > > >> Or for (2), i.e., something like --disable-index-cleanup option is added
> > > >> as your patch does? Or for both?
> > > >
> > > > --index-cleanup=BOOL
> > >
> > > I agree with Euler's suggestion to have a 1-1 mapping between the
> > > option of vacuumdb and the VACUUM parameter
> >
> > +1. Attached the draft version patches for both options.
>
> + printf(_(" --index-cleanup=BOOLEAN do or do not index vacuuming and index cleanup\n"));
> + printf(_(" --truncate=BOOLEAN do or do not truncate off empty pages at the end of the
table\n"));
>
> I *feel* that force/inhibit is suitable than true/false for the
> options.
Indeed.
+ If not specify this option
+ the behavior depends on <literal>vacuum_index_cleanup</literal> option
+ for the table to be vacuumed.
+ If not specify this option
+ the behavior depends on <literal>vacuum_truncate</literal> option
+ for the table to be vacuumed.
Those sentences should be rephrased to something like "If this option
is not specified, the bahvior...".
On Fri, May 10, 2019 at 9:03 PM Julien Rouhaud <rjuju123@gmail.com> wrote:
>
> On Thu, May 9, 2019 at 1:39 PM Kyotaro HORIGUCHI
> <horiguchi.kyotaro@lab.ntt.co.jp> wrote:
> >
> > At Thu, 9 May 2019 20:14:51 +0900, Masahiko Sawada <sawada.mshk@gmail.com> wrote in
<CAD21AoBmA9H3ZRuQFF+9io9PKhP+ePS=D+ThZ6ohRMdBm2x8Pw@mail.gmail.com>
> > > On Thu, May 9, 2019 at 10:01 AM Michael Paquier <michael@paquier.xyz> wrote:
> > > >
> > > > On Wed, May 08, 2019 at 06:21:09PM -0300, Euler Taveira wrote:
> > > > > Em qua, 8 de mai de 2019 às 14:19, Fujii Masao <masao.fujii@gmail.com> escreveu:
> > > > >> The question is; we should support vacuumdb option for (1), i.e.,,
> > > > >> something like --index-cleanup option is added?
> > > > >> Or for (2), i.e., something like --disable-index-cleanup option is added
> > > > >> as your patch does? Or for both?
> > > > >
> > > > > --index-cleanup=BOOL
> > > >
> > > > I agree with Euler's suggestion to have a 1-1 mapping between the
> > > > option of vacuumdb and the VACUUM parameter
> > >
> > > +1. Attached the draft version patches for both options.
> >
> > + printf(_(" --index-cleanup=BOOLEAN do or do not index vacuuming and index cleanup\n"));
> > + printf(_(" --truncate=BOOLEAN do or do not truncate off empty pages at the end of the
table\n"));
> >
> > I *feel* that force/inhibit is suitable than true/false for the
> > options.
>
> Indeed.
The new VACUUM command option for these option take true and false as
the same meaning. What is the motivation is to change a 1-1 mapping
name?
>
> + If not specify this option
> + the behavior depends on <literal>vacuum_index_cleanup</literal> option
> + for the table to be vacuumed.
>
> + If not specify this option
> + the behavior depends on <literal>vacuum_truncate</literal> option
> + for the table to be vacuumed.
>
> Those sentences should be rephrased to something like "If this option
> is not specified, the bahvior...".
Thank you! I've incorporated your comment in my branch. I'll post the
updated version patch after the above discussion got a consensus.
Regards,
--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center
On Mon, May 13, 2019 at 07:28:25PM +0900, Masahiko Sawada wrote: > Thank you! I've incorporated your comment in my branch. I'll post the > updated version patch after the above discussion got a consensus. Fujii-san, any input about the way to move forward here? Beta1 is planned for next week, hence it would be nice to progress on this front this week. -- Michael
Вложения
On Tue, May 14, 2019 at 10:01 AM Michael Paquier <michael@paquier.xyz> wrote: > > On Mon, May 13, 2019 at 07:28:25PM +0900, Masahiko Sawada wrote: > > Thank you! I've incorporated your comment in my branch. I'll post the > > updated version patch after the above discussion got a consensus. > > Fujii-san, any input about the way to move forward here? Beta1 is > planned for next week, hence it would be nice to progress on this > front this week. I think that we can push "--index-cleanup=BOOLEAN" version into beta1, and then change the interface of the options if we received many complaints about "--index-cleanup=BOOLEAN" from users. So this week, I'd like to review Sawada's patch and commit it if that's ok. Regards, -- Fujii Masao
On Thu, May 9, 2019 at 8:20 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote: > > On Thu, May 9, 2019 at 10:01 AM Michael Paquier <michael@paquier.xyz> wrote: > > > > On Wed, May 08, 2019 at 06:21:09PM -0300, Euler Taveira wrote: > > > Em qua, 8 de mai de 2019 às 14:19, Fujii Masao <masao.fujii@gmail.com> escreveu: > > >> The question is; we should support vacuumdb option for (1), i.e.,, > > >> something like --index-cleanup option is added? > > >> Or for (2), i.e., something like --disable-index-cleanup option is added > > >> as your patch does? Or for both? > > > > > > --index-cleanup=BOOL > > > > I agree with Euler's suggestion to have a 1-1 mapping between the > > option of vacuumdb and the VACUUM parameter > > +1. Attached the draft version patches for both options. Thanks for the patch! + if (strncasecmp(opt_str, "true", 4) != 0 && + strncasecmp(opt_str, "false", 5) != 0) Shouldn't we allow also "on" and "off", "1", "0" as a valid boolean value, like VACUUM does? + char *index_cleanup; The patch would be simpler if enum trivalue is used for index_cleanup variable as the type. Regards, -- Fujii Masao
On Wed, May 15, 2019 at 03:19:29AM +0900, Fujii Masao wrote: > + if (strncasecmp(opt_str, "true", 4) != 0 && > + strncasecmp(opt_str, "false", 5) != 0) > > Shouldn't we allow also "on" and "off", "1", "0" as a valid boolean value, > like VACUUM does? I am wondering, in order to keep this patch simple, if you shouldn't accept any value and just let the parsing logic on the backend side do all the work. That's what we do for other things like the connection parameter replication for example, and there is no need to mimic a boolean parsing equivalent on the frontend with something like check_bool_str() as presented in the patch. The main downside is that the error message gets linked to VACUUM and not vacuumdb. Another thing which you may be worth looking at would be to make parse_bool() frontend aware, where pg_strncasecmp() is actually available. -- Michael
Вложения
On Wed, May 15, 2019 at 7:51 AM Michael Paquier <michael@paquier.xyz> wrote: > > On Wed, May 15, 2019 at 03:19:29AM +0900, Fujii Masao wrote: > > + if (strncasecmp(opt_str, "true", 4) != 0 && > > + strncasecmp(opt_str, "false", 5) != 0) > > > > Shouldn't we allow also "on" and "off", "1", "0" as a valid boolean value, > > like VACUUM does? > > I am wondering, in order to keep this patch simple, if you shouldn't > accept any value and just let the parsing logic on the backend side > do all the work. That's what we do for other things like the > connection parameter replication for example, and there is no need to > mimic a boolean parsing equivalent on the frontend with something like > check_bool_str() as presented in the patch. The main downside is that > the error message gets linked to VACUUM and not vacuumdb. I might be missing something but if the frontend code doesn't check arguments and we let the backend parsing logic do all the work then it allows user to execute an arbitrary SQL command via vacuumdb. > > Another thing which you may be worth looking at would be to make > parse_bool() frontend aware, where pg_strncasecmp() is actually > available. Or how about add a function that parse a boolean string value, as a common routine among frontend programs, maybe in common.c or fe_utils? We results in having the duplicate code between frontend and backend but it may be less side effects than making parse_bool available on frontend code. Regards, -- Masahiko Sawada NIPPON TELEGRAPH AND TELEPHONE CORPORATION NTT Open Source Software Center
Hi, On 2019-05-15 11:36:52 +0900, Masahiko Sawada wrote: > I might be missing something but if the frontend code doesn't check > arguments and we let the backend parsing logic do all the work then it > allows user to execute an arbitrary SQL command via vacuumdb. But, so what? The user could just have used psql to do so? Greetings, Andres Freund
On Wed, May 15, 2019 at 11:45 AM Andres Freund <andres@anarazel.de> wrote: > > Hi, > > On 2019-05-15 11:36:52 +0900, Masahiko Sawada wrote: > > I might be missing something but if the frontend code doesn't check > > arguments and we let the backend parsing logic do all the work then it > > allows user to execute an arbitrary SQL command via vacuumdb. > > But, so what? The user could just have used psql to do so? Indeed. It shouldn't be a problem and we even now can do that by specifying for example --table="t(c1);select 1" but doesn't work. Regards, -- Masahiko Sawada NIPPON TELEGRAPH AND TELEPHONE CORPORATION NTT Open Source Software Center
On Wed, May 15, 2019 at 1:01 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote: > > On Wed, May 15, 2019 at 11:45 AM Andres Freund <andres@anarazel.de> wrote: > > > > Hi, > > > > On 2019-05-15 11:36:52 +0900, Masahiko Sawada wrote: > > > I might be missing something but if the frontend code doesn't check > > > arguments and we let the backend parsing logic do all the work then it > > > allows user to execute an arbitrary SQL command via vacuumdb. > > > > But, so what? The user could just have used psql to do so? > > Indeed. It shouldn't be a problem and we even now can do that by > specifying for example --table="t(c1);select 1" but doesn't work. > I've attached new version patch that takes the way to let the backend parser do all work. Regards, -- Masahiko Sawada NIPPON TELEGRAPH AND TELEPHONE CORPORATION NTT Open Source Software Center
Вложения
On Wed, May 15, 2019 at 03:44:22PM +0900, Masahiko Sawada wrote: > I've attached new version patch that takes the way to let the backend > parser do all work. I was wondering how the error handling gets by not having the parsing on the frontend, and it could be worse: $ vacuumdb --index-cleanup=popo vacuumdb: vacuuming database "postgres" vacuumdb: error: vacuuming of table "pg_catalog.pg_proc" in database "postgres" failed: ERROR: index_cleanup requires a Boolean value $ vacuumdb --truncate=popo vacuumdb: vacuuming database "postgres" vacuumdb: error: vacuuming of table "pg_catalog.pg_proc" in database "postgres" failed: ERROR: truncate requires a Boolean value For TRUNCATE, we actually get to the same error, and INDEX_CLEANUP just defers with the separator between the two terms. I think that we could live with that for simplicity's sake. Perhaps others have different opinions though. + if (vacopts.index_cleanup != NULL) Checking directly for NULL-ness here is inconsistent with the previous callers. +$node->issues_sql_like( + [ 'vacuumdb', '--index-cleanup=true', 'postgres' ], + qr/statement: VACUUM \(INDEX_CLEANUP true\).*;/, + 'vacuumdb --index-cleanup=true') We should have a failure test here instead of testing two times the same boolean parameter with opposite values to make sure that we still complain on invalid input values. + Specify that <command>VACUUM</command> should attempt to remove + index entries pointing to dead tuples. If this option is not specified + the behavior depends on <literal>vacuum_index_cleanup</literal> option + for the table to be vacuumed. The description of other commands do not mention directly VACUUM, and have a more straight-forward description about the goal of the option. So the first sentence could be reworked as follows: Removes index entries pointing to dead tuples. And the second for --truncate: Truncates any empty pages at the end of the relation. My 2c. -- Michael
Вложения
On Fri, May 17, 2019 at 11:09 AM Michael Paquier <michael@paquier.xyz> wrote: > > On Wed, May 15, 2019 at 03:44:22PM +0900, Masahiko Sawada wrote: > > I've attached new version patch that takes the way to let the backend > > parser do all work. > > I was wondering how the error handling gets by not having the parsing > on the frontend, and it could be worse: > $ vacuumdb --index-cleanup=popo > vacuumdb: vacuuming database "postgres" > vacuumdb: error: vacuuming of table "pg_catalog.pg_proc" in database > "postgres" failed: ERROR: index_cleanup requires a Boolean value > $ vacuumdb --truncate=popo > vacuumdb: vacuuming database "postgres" > vacuumdb: error: vacuuming of table "pg_catalog.pg_proc" in database > "postgres" failed: ERROR: truncate requires a Boolean value > > For TRUNCATE, we actually get to the same error, and INDEX_CLEANUP > just defers with the separator between the two terms. I think that we > could live with that for simplicity's sake. +1 > Perhaps others have different opinions though. Is it helpful for user if we show executed SQL when fails as the frontend programs does in executeCommand? $ vacuumdb --index-cleanup=foo postgres vacuumdb: vacuuming database "postgres" vacuumdb: error: vacuuming of table "pg_catalog.pg_proc" in database "postgres" failed: ERROR: index_cleanup requires a Boolean value vacuumdb: query was: VACUUM (INDEX_CLEANUP foo) pg_catalog.pg_proc; > > + if (vacopts.index_cleanup != NULL) > Checking directly for NULL-ness here is inconsistent with the previous > callers. > > +$node->issues_sql_like( > + [ 'vacuumdb', '--index-cleanup=true', 'postgres' ], > + qr/statement: VACUUM \(INDEX_CLEANUP true\).*;/, > + 'vacuumdb --index-cleanup=true') > We should have a failure test here instead of testing two times the > same boolean parameter with opposite values to make sure that we still > complain on invalid input values. Fixed. > > + Specify that <command>VACUUM</command> should attempt to remove > + index entries pointing to dead tuples. If this option is not specified > + the behavior depends on <literal>vacuum_index_cleanup</literal> option > + for the table to be vacuumed. > The description of other commands do not mention directly VACUUM, and > have a more straight-forward description about the goal of the option. > So the first sentence could be reworked as follows: > Removes index entries pointing to dead tuples. > > And the second for --truncate: > Truncates any empty pages at the end of the relation. Fixed. Regards, -- Masahiko Sawada NIPPON TELEGRAPH AND TELEPHONE CORPORATION NTT Open Source Software Center
Hi, On 2019-05-15 15:44:22 +0900, Masahiko Sawada wrote: > From de60d212b50a6412e483c995b83e28c5597089ad Mon Sep 17 00:00:00 2001 > From: Masahiko Sawada <sawada.mshk@gmail.com> > Date: Thu, 9 May 2019 20:02:05 +0900 > Subject: [PATCH v3 1/2] Add --index-cleanup option to vacuumdb. > From 59e3146f585e288d41738daa9a1d18687e2851d1 Mon Sep 17 00:00:00 2001 > From: Masahiko Sawada <sawada.mshk@gmail.com> > Date: Wed, 15 May 2019 15:27:51 +0900 > Subject: [PATCH v3 2/2] Add --truncate option to vacuumdb. > My impression is that these are better treated as feature work, to be tackled in v13. I see no urgency to push this for v12. There's still some disagreements on how parts of this are implemented, and we've beta1 coming up. IMO we should just register this patch for the next CF, and drop the open item. Greetings, Andres Freund
On Fri, May 17, 2019 at 01:11:53PM -0700, Andres Freund wrote: > My impression is that these are better treated as feature work, to be > tackled in v13. I see no urgency to push this for v12. There's still > some disagreements on how parts of this are implemented, and we've beta1 > coming up. It is true that we have lived without some options in vacuumdb while these were already introduced at the SQL level, so I am in favor of what you suggest here. Fujii-san, what do you think? -- Michael
Вложения
On Sat, May 18, 2019 at 7:19 PM Michael Paquier <michael@paquier.xyz> wrote: > > On Fri, May 17, 2019 at 01:11:53PM -0700, Andres Freund wrote: > > My impression is that these are better treated as feature work, to be > > tackled in v13. I see no urgency to push this for v12. There's still > > some disagreements on how parts of this are implemented, and we've beta1 > > coming up. > > It is true that we have lived without some options in vacuumdb while > these were already introduced at the SQL level, so I am in favor of > what you suggest here. Fujii-san, what do you think? I'm ok to drop this from open items for v12 because this is not a bug. Let's work on this next CommitFest. Regards, -- Fujii Masao
On Mon, May 20, 2019 at 10:17:31AM +0900, Fujii Masao wrote: > I'm ok to drop this from open items for v12 because this is not a bug. > Let's work on this next CommitFest. Okay, I have moved out the item from the list of opened ones. -- Michael