From: Michael Paquier <michael@paquier.xyz>
To: Bossart, Nathan <bossartn@amazon.com>
Cc: Masahiko Sawada <sawada.mshk@gmail.com>
Cc: Robert Haas <robertmhaas@gmail.com>
Cc: pgsql-hackers@postgresql.org <pgsql-hackers@postgresql.org>
Subject: Re: A few new options for vacuumdb
Date: Tue, 22 Jan 2019 15:07:30 +0900
Message-ID: <20190122060730.GD8719@paquier.xyz> (raw)
In-Reply-To: <5C971918-E566-41AB-895A-C4793D56B041@amazon.com>
References: <33F59F9F-78A6-4ECE-BFE0-B1E4AAF40094@amazon.com>
<579F2B3A-4EA5-4032-AAD9-4AEE9219A4AB@amazon.com>
<CAD21AoDHsFx_Rnp-AB2UT2ORc3_PmCx18-wfrA_38i038D5WZQ@mail.gmail.com>
<20190108020300.GH22498@paquier.xyz>
<FF67D651-A7AF-4E70-9401-805E3CFB4DC9@amazon.com>
<20190109010637.GC21835@paquier.xyz>
<CAD21AoAqsSoTFWjajz1yycwxDnLZSWfV7wT7mmMBqy2k-jkLUQ@mail.gmail.com>
<20190109043316.GH21835@paquier.xyz>
<CAD21AoCzekiuDaGN-JqtoLgaYqqxGQ6nTcNqN0bm1EkvmcUKPA@mail.gmail.com>
<5C971918-E566-41AB-895A-C4793D56B041@amazon.com>
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
Attachments:
[application/pgp-signature] signature.asc (832B, ../20190122060730.GD8719@paquier.xyz/2-signature.asc)
download
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: michael@paquier.xyz, bossartn@amazon.com, sawada.mshk@gmail.com, robertmhaas@gmail.com
Subject: Re: A few new options for vacuumdb
In-Reply-To: <20190122060730.GD8719@paquier.xyz>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox