Re: A few new options for vacuumdb

On Mon, Jan 21, 2019 at 10:09:09PM +0000, Bossart, Nathan wrote:
> Here's a new patch set that should address the feedback in this
> thread. The changes in this version include:
>
> - 0001 is a small fix to the 'vacuumdb --disable-page-skipping'
> documentation. My suggestion is to keep it short and simple like
> --full, --freeze, --skip-locked, --verbose, and --analyze. The
> DISABLE_PAGE_SKIPPING option is well-described in the VACUUM
> documentation, and IMO it is reasonably obvious that such vacuumdb
> options correspond to the VACUUM options.

Okay, committed this one.

> - v3-0002 is essentially v2-0001 and v2-0004 combined. I've also
> added a comment explaining the importance of fully qualifying the
> catalog query used to discover tables to process.

Regarding the search_path business, there is actually similar business
in expand_table_name_patterns() for pg_dump if you look close by, so
as users calling --table don't have to specify the schema, and just
stick with patterns.

+ /*
+ * Prepare the list of tables to process by querying the catalogs.
+ *
+ * Since we execute the constructed query with the default search_path
+ * (which could be unsafe), it is important to fully qualify everything.
+ */
It is not only important, but also absolutely mandatory, so let's make
the comment insisting harder on that point. From this point of view,
the query that you are building is visibly correct.

+ appendStringLiteralConn(&catalog_query, just_table, conn);
+ appendPQExpBuffer(&catalog_query, "::pg_catalog.regclass\n");
+
+ pg_free(just_table);
+
+ cell = cell->next;
+ if (cell == NULL)
+ appendPQExpBuffer(&catalog_query, " )\n");
Hm. It seems to me that you are duplicating what
processSQLNamePattern() does, so we ought to use it. And it is
possible to control the generation of the different WHERE clauses with
a single query based on the number of elements in the list. Perhaps I
am missing something? It looks unnecessary to export
split_table_columns_spec() as well.

- qr/statement: ANALYZE;/,
+ qr/statement: ANALYZE pg_catalog\./,
Or we could just use "ANALYZE \.;/", perhaps patching it first.
Perhaps my regexp here is incorrect, but I just mean to check for an
ANALYZE query which begins by "ANALYZE " and finishes with ";",
without caring about what is in-between. This would make the tests
more portable.

> - 0003 includes additional documentation changes explaining the main
> uses of --min-xid-age and --min-mxid-age and linking to the
> existing wraparound documentation.

+$node->issues_sql_like(
+ [ 'vacuumdb', '--table=pg_class', '--min-mxid-age=123456789',
'postgres'],
+ qr/AND GREATEST\(pg_catalog.mxid_age\(c.relminmxid\),
pg_catalog.mxid_age\(t.relminmxid\)\) OPERATOR\(pg_catalog.>=\) 123456789/,
+ 'vacuumdb --table --min-mxid-age');
+$node->issues_sql_like(
+ [ 'vacuumdb', '--min-xid-age=987654321', 'postgres' ],
+ qr/AND GREATEST\(pg_catalog.age\(c.relfrozenxid\),
pg_catalog.age\(t.relfrozenxid\)\) OPERATOR\(pg_catalog.>=\) 987654321/,
+ 'vacuumdb --min-xid-age');
It may be better to use numbers which make sure that no relations are
actually fetched, so as if the surrounding tests are messed up we
never make them longer than necessary just to check the shape of the
query.
--
Michael