Re: [Proposal] vacuumdb --schema only - Mailing list pgsql-hackers

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