On Mon, Apr 25, 2022 at 09:18:53AM -0700, Nathan Bossart wrote: > I've marked it as ready-for-committer.
The refactoring logic to build the queries is clear to follow. I have a few comments about the shape of the patch, though. case 'a': - alldb = true; + check_objfilter(OBJFILTER_ALL_DBS); break; The cross-option checks are usually done after all the options switches are check. Why does this need to be different? It does not strike me as a huge problem to do one filter check at the end. +void +check_objfilter(VacObjFilter curr_option) +{ + objfilter |= curr_option; + + if ((objfilter & OBJFILTER_ALL_DBS) && + (objfilter & OBJFILTER_DATABASE)) + pg_fatal("cannot vacuum all databases and a specific one at the same time"); The addition of more OBJFILTER_* (unlikely going to happen, but who knows) would make it hard to know which option should not interact with each other. Wouldn't it be better to use a kind of compatibility table for that? As one OBJFILTER_* matches with one option, you could simplify the number of strings in need of translation by switching to an error message like "cannot use options %s and %s together", or something like that? +$node->command_fails( + [ 'vacuumdb', '-a', '-d', 'postgres' ], + 'cannot use options -a and -d at the same time'); This set of tests had better use command_fails_like() to make sure that the correct error patterns from check_objfilter() show up? -- Michael
signature.asc
Description: PGP signature