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
Description: PGP signature
