Re: [Proposal] vacuumdb --schema only

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.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com