On Fri, May 17, 2019 at 11:09 AM Michael Paquier <mich...@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