Re: [Proposal] vacuumdb --schema only

Thanks for the new patch!  I think this is on the right track.

On Wed, Apr 20, 2022 at 05:15:02PM +0200, Gilles Darold wrote:
> Le 18/04/2022 à 23:56, Nathan Bossart a écrit :
>> > -    if (!tables_listed)
>> > +    if (!objects_listed || objfilter == OBJFILTER_SCHEMA)
>> Do we need to check for objects_listed here?  IIUC we can just check for
>> objfilter != OBJFILTER_TABLE.
> 
> Yes we need it otherwise test 'vacuumdb with view' fail because we are not
> trying to vacuum the view so the PG doesn't report:
> 
>     WARNING:  cannot vacuum non-tables or special system tables

My point is that the only time we don't want to filter for relevant
relation types is when the user provides a list of tables.  So my
suggestion would be to simplify this to the following:

    if (objfilter != OBJFILTER_TABLE)
    {
        appendPQExpBufferStr(...);
        has_where = true;
    }

>> Unless I'm missing something, schema_is_exclude appears to only be used for
>> error checking and doesn't impact the generated catalog query.  It looks
>> like the relevant logic disappeared after v4 of the patch.
> 
> Right, removed.

I don't think -N works at the moment.  I tested it out, and vacuumdb was
still processing tables in schemas I excluded.  Can we add a test case for
this, too?

> +/*
> + * Verify that the filters used at command line are compatible
> + */
> +void
> +check_objfilter(VacObjectFilter curr_objfilter, VacObjectFilter curr_option)
> +{
> +    switch (curr_option)
> +    {
> +        case OBJFILTER_NONE:
> +            break;
> +        case OBJFILTER_DATABASE:
> +            /* When filtering on database name, vacuum on all database is not allowed. */
> +            if (curr_objfilter == OBJFILTER_ALL)
> +                pg_fatal("cannot vacuum all databases and a specific one at the same time");
> +            break;
> +        case OBJFILTER_ALL:
> +            /* When vacuuming all database, filter on database name is not allowed. */
> +            if (curr_objfilter == OBJFILTER_DATABASE)
> +                pg_fatal("cannot vacuum all databases and a specific one at the same time");
> +            /* When vacuuming all database, filter on schema name is not allowed. */
> +            if (curr_objfilter == OBJFILTER_SCHEMA)
> +                pg_fatal("cannot vacuum specific schema(s) in all databases");
> +            /* When vacuuming all database, schema exclusion is not allowed. */
> +            if (curr_objfilter == OBJFILTER_SCHEMA_EXCLUDE)
> +                pg_fatal("cannot exclude from vacuum specific schema(s) in all databases");
> +            /* When vacuuming all database, filter on table name is not allowed. */
> +            if (curr_objfilter == OBJFILTER_TABLE)
> +                pg_fatal("cannot vacuum specific table(s) in all databases");
> +            break;
> +        case OBJFILTER_TABLE:
> +            /* When filtering on table name, filter by schema is not allowed. */
> +            if (curr_objfilter == OBJFILTER_SCHEMA)
> +                pg_fatal("cannot vacuum all tables in schema(s) and specific table(s) at the same time");
> +            /* When filtering on table name, schema exclusion is not allowed. */
> +            if (curr_objfilter == OBJFILTER_SCHEMA_EXCLUDE)
> +                pg_fatal("cannot vacuum specific table(s) and exclude specific schema(s) at the same time");
> +            break;
> +        case OBJFILTER_SCHEMA:
> +            /* When filtering on schema name, filter by table is not allowed. */
> +            if (curr_objfilter == OBJFILTER_TABLE)
> +                pg_fatal("cannot vacuum all tables in schema(s) and specific table(s) at the same time");
> +            /* When filtering on schema name, schema exclusion is not allowed. */
> +            if (curr_objfilter == OBJFILTER_SCHEMA_EXCLUDE)
> +                pg_fatal("cannot vacuum all tables in schema(s) and exclude specific schema(s) at the same time");
> +            /* filtering on schema name can not be use on all database. */
> +            if (curr_objfilter == OBJFILTER_ALL)
> +                pg_fatal("cannot vacuum specific schema(s) in all databases");
> +            break;
> +        case OBJFILTER_SCHEMA_EXCLUDE:
> +            /* When filtering on schema exclusion, filter by table is not allowed. */
> +            if (curr_objfilter == OBJFILTER_TABLE)
> +                pg_fatal("cannot vacuum all tables in schema(s) and specific table(s) at the same time");
> +            /* When filetring on schema exclusion, filter by schema is not allowed. */
> +            if (curr_objfilter == OBJFILTER_SCHEMA)
> +                pg_fatal("cannot vacuum all tables in schema(s) and exclude specific schema(s) at the same time");
> +            break;
> +    }
> +}

I don't think this handles all combinations.  For example, the following
command does not fail:

    vacuumdb -a -N test postgres

Furthermore, do you think it'd be possible to dynamically generate the
message?  If it doesn't add too much complexity, this might be a nice way
to simplify this function.

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