Re: vacuumdb and new VACUUM options

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