Re: [Proposal] vacuumdb --schema only

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