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

Le 30/03/2022 à 23:22, Nathan Bossart a écrit :
> I took a look at the v4 patch.
>
> 'git-apply' complains about whitespace errors:

Fixed.


> +   <para>
> +    To clean all tables in the <literal>Foo</literal> and <literal>bar</literal> schemas
> +    only in a database named <literal>xyzzy</literal>:
> +<screen>
> +<prompt>$ </prompt><userinput>vacuumdb --schema='"Foo"' --schema='bar' xyzzy</userinput>
> +</screen></para>
>
> nitpicks: I think the phrasing should be "To only clean tables in the...".
> Also, is there any reason to use a schema name with a capital letter as an
> example?  IMO that just adds unnecessary complexity to the example.

I have though that an example of a schema with case sensitivity was
missing in the documentation but I agree with your comment, this is
probably not he best place to do that. Fixed.


> +$node->issues_sql_like(
> +    [ 'vacuumdb', '--schema', '"Foo"', 'postgres' ],
> +    qr/VACUUM "Foo".*/,
> +    'vacuumdb --schema schema only');
>
> IIUC there should only be one table in the schema.  Can we avoid matching
> "*" and check for the exact command instead?

Fixed.


> I think there should be a few more test cases.  For example, we should test
> using -n and -N at the same time, and we should test what happens when
> those options are used for missing schemas.

Fixed


> +    /*
> +     * 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 && (schemas.head != NULL))
> +    {
> +        pg_log_error("cannot vacuum all tables in schema(s) and specific table(s) at the same time");
> +        exit(1);
> +    }
>
> I think there might be some useful refactoring we can do that would
> simplify adding similar options in the future.  Specifically, can we have a
> global variable that stores the type of vacuumdb command (e.g., all,
> tables, or schemas)?  If so, perhaps the tables list could be renamed and
> reused for schemas (and any other objects that need listing in the future).

I don't think there will be much more options like this one that will be
added to this command but anyway I have changed the patch that way.


> +        if (schemas != NULL && schemas->head != NULL)
> +        {
> +            appendPQExpBufferStr(&catalog_query,
> +                                 " AND c.relnamespace");
> +            if (schema_is_exclude == TRI_YES)
> +                appendPQExpBufferStr(&catalog_query,
> +                                    " OPERATOR(pg_catalog.!=) ALL (ARRAY[");
> +            else if (schema_is_exclude == TRI_NO)
> +                appendPQExpBufferStr(&catalog_query,
> +                                    " OPERATOR(pg_catalog.=) ANY (ARRAY[");
> +
> +            for (cell = schemas->head; cell != NULL; cell = cell->next)
> +            {
> +                appendStringLiteralConn(&catalog_query, cell->val, conn);
> +
> +                if (cell->next != NULL)
> +                    appendPQExpBufferStr(&catalog_query, ", ");
> +            }
> +
> +            /* Finish formatting schema filter */
> +            appendPQExpBufferStr(&catalog_query, "]::pg_catalog.regnamespace[])\n");
> +        }
>
> IMO we should use a CTE for specified schemas like we do for the specified
> tables.  I wonder if we could even have a mostly-shared CTE code path for
> all vacuumdb commands with a list of names.

Fixed


> -    /*
> -     * 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 (!tables_listed)
> +    else
>      {
> +        /*
> +         * 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.
> +         */
> nitpick: This change seems unnecessary.

Fixed


Thanks for the review, all these changes are available in new version v6
of the patch and attached here.


Best regards,

-- 
Gilles Darold