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