Re: vacuumdb and new VACUUM options

On Fri, May 17, 2019 at 11:09 AM Michael Paquier <michael(at)paquier(dot)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