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

Le 18/04/2022 à 23:56, Nathan Bossart a écrit :
> 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.

Fixed.


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

Fixed.


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

I agree, done.


>> +    /*
>> +     * 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?

Fixed.


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

Done.


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

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


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


New patch attached v9.


-- 
Gilles Darold