On 2022-04-26 Tu 00:46, Nathan Bossart wrote: > On Tue, Apr 26, 2022 at 11:36:02AM +0900, Michael Paquier wrote: >> The refactoring logic to build the queries is clear to follow. I have >> a few comments about the shape of the patch, though. > Thanks for taking a look! > >> 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. > Makes sense. I fixed this in v13. > >> +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? > I think this might actually make things more complicated. In addition to > the compatibility table, we'd need to define the strings to use in the > error message somewhere. I can give this a try if you feel strongly about > it. > >> +$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? > Yes. I did this in v13.
committed. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com