Re: [Proposal] vacuumdb --schema only

On Thu, Apr 14, 2022 at 10:27:46PM +0200, Gilles Darold wrote:
> Attached v8 of the patch that tries to address the remarks above, fixes
> patch apply failure to master and replace calls to pg_log_error+exit
> with pg_fatal.

Thanks for the new patch.

> +enum trivalue schema_is_exclude = TRI_DEFAULT;
> +
> +/*
> + * The kind of object use in the command line filter.
> + *   OBJFILTER_NONE: no filter used
> + *   OBJFILTER_SCHEMA: -n | --schema or -N | --exclude-schema
> + *   OBJFILTER_TABLE: -t | --table
> + */
> +enum VacObjectFilter
> +{
> +	OBJFILTER_NONE,
> +	OBJFILTER_TABLE,
> +	OBJFILTER_SCHEMA
> +};
> +
> +enum VacObjectFilter objfilter = OBJFILTER_NONE;

I still think we ought to remove schema_is_exclude in favor of adding
OBJFILTER_SCHEMA_EXCLUDE to the enum.  I think that would simplify some of
the error handling and improve readability.  IMO we should add
OBJFILTER_ALL, too.

> -					simple_string_list_append(&tables, optarg);
> +					/* When filtering on schema name, filter by table is not allowed. */
> +					if (schema_is_exclude != TRI_DEFAULT)
> +						pg_fatal("cannot vacuum all tables in schema(s) and specific table(s) at the same time");
> +					simple_string_list_append(&objects, optarg);
> +					objfilter = OBJFILTER_TABLE;
>  					tbl_count++;
>  					break;
>  				}
> @@ -202,6 +224,32 @@ main(int argc, char *argv[])
>  									  &vacopts.parallel_workers))
>  					exit(1);
>  				break;
> +			case 'n':			/* include schema(s) */
> +				{
> +					/* When filtering on schema name, filter by table is not allowed. */
> +					if (tbl_count)
> +						pg_fatal("cannot vacuum all tables in schema(s) and specific table(s) at the same time");
> +
> +					if (schema_is_exclude == TRI_YES)
> +						pg_fatal("cannot vacuum all tables in schema(s) and exclude specific schema(s) at the same time");
> +					simple_string_list_append(&objects, optarg);
> +					objfilter = OBJFILTER_SCHEMA;
> +					schema_is_exclude = TRI_NO;
> +					break;
> +				}
> +			case 'N':			/* exclude schema(s) */
> +				{
> +					/* When filtering on schema name, filter by table is not allowed. */
> +					if (tbl_count)
> +						pg_fatal("cannot vacuum all tables in schema(s) and specific table(s) at the same time");
> +					if (schema_is_exclude == TRI_NO)
> +						pg_fatal("cannot vacuum all tables in schema(s) and exclude specific schema(s) at the same time");
> +
> +					simple_string_list_append(&objects, optarg);
> +					objfilter = OBJFILTER_SCHEMA;
> +					schema_is_exclude = TRI_YES;
> +					break;

I was expecting these to check objfilter.  For example:

	case 'N':
		{
			if (objfilter == OBJFILTER_TABLE)
				pg_fatal("...");
			else if (objfilter == OBJFILTER_SCHEMA)
				pg_fatal("...");
			else if (objfilter == OBJFILTER_ALL)
				pg_fatal("...");

			simple_string_list_append(&objects, optarg);
			objfilter = OBJFILTER_SCHEMA_EXCLUDE;
			break;
		}

Another possible improvement could be to move the pg_fatal() calls to a
helper function that generates the message based on the current objfilter
setting and the current option.  I'm envisioning something like
check_objfilter(VacObjFilter curr_objfilter, VacObjFilter curr_option).

> +	/*
> +	 * When filtering on schema name, filter by table is not allowed.
> +	 * The schema name can already be set to a fqdn table name.
> +	 */
> +	if (tbl_count && objfilter == OBJFILTER_SCHEMA && objects.head != NULL)
> +		pg_fatal("cannot vacuum all tables in schema(s) and specific table(s) at the same time");

Isn't this redundant with the error in the option handling?

> -		if (tables.head != NULL)
> +		if (objfilter == OBJFILTER_SCHEMA && objects.head != NULL)
> +		{
> +			if (schema_is_exclude == TRI_YES)
> +				pg_fatal("cannot exclude from vacuum specific schema(s) in all databases");
> +			else if (schema_is_exclude == TRI_NO)
> +				pg_fatal("cannot vacuum specific schema(s) in all databases");
> +		}
> +
> +		if (objfilter == OBJFILTER_TABLE && objects.head != NULL)
>  			pg_fatal("cannot vacuum specific table(s) in all databases");

I think we could move all these into check_objfilter(), too.

nitpick: Why do we need to check that objects.head is not NULL?  Isn't the
objfilter check enough?

>  	/*
> -	 * If no tables were listed, filter for the relevant relation types.  If
> -	 * tables were given via --table, don't bother filtering by relation type.
> -	 * Instead, let the server decide whether a given relation can be
> -	 * processed in which case the user will know about it.
> +	 * If no tables were listed or that a filter by schema is used, filter
> +	 * for the relevant relation types.  If tables were given via --table,
> +	 * don't bother filtering by relation type.  Instead, let the server
> +	 * decide whether a given relation can be processed in which case the
> +	 * user will know about it.  If there is a filter by schema the use of
> +	 * --table is not possible so we have to filter by relation type too.
>  	 */
> -	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.

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.

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