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
