Re: [Proposal] vacuumdb --schema only

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