vacuumdb and new VACUUM options

classic Classic list List threaded Threaded
25 messages Options
12
Reply | Threaded
Open this post in threaded view
|

vacuumdb and new VACUUM options

Fujii Masao-2
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


Reply | Threaded
Open this post in threaded view
|

Re: vacuumdb and new VACUUM options

Masahiko Sawada
On Wed, May 8, 2019 at 2:41 AM Fujii Masao <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: vacuumdb and new VACUUM options

Michael Paquier-2
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

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: vacuumdb and new VACUUM options

Julien Rouhaud
On Wed, May 8, 2019 at 9:06 AM Michael Paquier <[hidden email]> 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!


Reply | Threaded
Open this post in threaded view
|

Re: vacuumdb and new VACUUM options

Fujii Masao-2
In reply to this post by Masahiko Sawada
On Wed, May 8, 2019 at 9:32 AM Masahiko Sawada <[hidden email]> wrote:

>
> On Wed, May 8, 2019 at 2:41 AM Fujii Masao <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: vacuumdb and new VACUUM options

Euler Taveira
Em qua, 8 de mai de 2019 às 14:19, Fujii Masao <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: vacuumdb and new VACUUM options

Michael Paquier-2
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 <[hidden email]> 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

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: vacuumdb and new VACUUM options

Masahiko Sawada
On Thu, May 9, 2019 at 10:01 AM Michael Paquier <[hidden email]> 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 <[hidden email]> 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

v2-0001-Add-index-cleanup-option-to-vacuumdb.patch (9K) Download Attachment
v2-0002-Add-truncate-option-to-vacuumdb.patch (7K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: vacuumdb and new VACUUM options

Kyotaro HORIGUCHI-2
At Thu, 9 May 2019 20:14:51 +0900, Masahiko Sawada <[hidden email]> wrote in <CAD21AoBmA9H3ZRuQFF+9io9PKhP+ePS=[hidden email]>

> On Thu, May 9, 2019 at 10:01 AM Michael Paquier <[hidden email]> 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 <[hidden email]> 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



Reply | Threaded
Open this post in threaded view
|

Re: vacuumdb and new VACUUM options

Julien Rouhaud
On Thu, May 9, 2019 at 1:39 PM Kyotaro HORIGUCHI
<[hidden email]> wrote:

>
> At Thu, 9 May 2019 20:14:51 +0900, Masahiko Sawada <[hidden email]> wrote in <CAD21AoBmA9H3ZRuQFF+9io9PKhP+ePS=[hidden email]>
> > On Thu, May 9, 2019 at 10:01 AM Michael Paquier <[hidden email]> 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 <[hidden email]> 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...".


Reply | Threaded
Open this post in threaded view
|

Re: vacuumdb and new VACUUM options

Masahiko Sawada
On Fri, May 10, 2019 at 9:03 PM Julien Rouhaud <[hidden email]> wrote:

>
> On Thu, May 9, 2019 at 1:39 PM Kyotaro HORIGUCHI
> <[hidden email]> wrote:
> >
> > At Thu, 9 May 2019 20:14:51 +0900, Masahiko Sawada <[hidden email]> wrote in <CAD21AoBmA9H3ZRuQFF+9io9PKhP+ePS=[hidden email]>
> > > On Thu, May 9, 2019 at 10:01 AM Michael Paquier <[hidden email]> 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 <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: vacuumdb and new VACUUM options

Michael Paquier-2
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

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: vacuumdb and new VACUUM options

Fujii Masao-2
On Tue, May 14, 2019 at 10:01 AM Michael Paquier <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: vacuumdb and new VACUUM options

Fujii Masao-2
In reply to this post by Masahiko Sawada
On Thu, May 9, 2019 at 8:20 PM Masahiko Sawada <[hidden email]> wrote:

>
> On Thu, May 9, 2019 at 10:01 AM Michael Paquier <[hidden email]> 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 <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: vacuumdb and new VACUUM options

Michael Paquier-2
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

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: vacuumdb and new VACUUM options

Masahiko Sawada
On Wed, May 15, 2019 at 7:51 AM Michael Paquier <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: vacuumdb and new VACUUM options

Andres Freund
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


Reply | Threaded
Open this post in threaded view
|

Re: vacuumdb and new VACUUM options

Masahiko Sawada
On Wed, May 15, 2019 at 11:45 AM Andres Freund <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: vacuumdb and new VACUUM options

Masahiko Sawada
On Wed, May 15, 2019 at 1:01 PM Masahiko Sawada <[hidden email]> wrote:

>
> On Wed, May 15, 2019 at 11:45 AM Andres Freund <[hidden email]> 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

v3-0001-Add-index-cleanup-option-to-vacuumdb.patch (8K) Download Attachment
v3-0002-Add-truncate-option-to-vacuumdb.patch (7K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: vacuumdb and new VACUUM options

Michael Paquier-2
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

signature.asc (849 bytes) Download Attachment
12