Re: [Proposal] vacuumdb --schema only

Le 20/04/2022 à 19:38, Nathan Bossart a écrit :
> 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;
> }

Right, I must have gotten mixed up in the test results. Fixed.

>>> 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?

Fixed and regression tests added as well as some others to test the
filter options compatibility.

> +/*
> + * 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;
> [...]
> + }
> +}
> I don't think this handles all combinations. For example, the following
> command does not fail:
>
> vacuumdb -a -N test postgres

Right, I have fix them all in this new patch.

> 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.

I have tried to avoid reusing the same error message several time by
using a new enum and function filter_error(). I also use the same
messages with --schema and --exclude-schema related errors.

Patch v10 attached.

--
Gilles Darold