From: Michael Paquier <michael@paquier.xyz>
To: Bossart, Nathan <bossartn@amazon.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: Mon, 7 Jan 2019 16:11:46 +0900
Message-ID: <20190107071146.GD22498@paquier.xyz> (raw)
In-Reply-To: <579F2B3A-4EA5-4032-AAD9-4AEE9219A4AB@amazon.com>
References: <FFE5373C-E26A-495B-B5C8-911EC4A41C5E@amazon.com>
<20181220020522.GM27104@paquier.xyz>
<7B4ACE8E-5D81-416B-B984-17E60F7ABABA@amazon.com>
<CA+TgmoaQ+h-gNAn3LL3Sn=_JVOGg8QDiRY1V6t4ECqPVWFTH=g@mail.gmail.com>
<33F59F9F-78A6-4ECE-BFE0-B1E4AAF40094@amazon.com>
<579F2B3A-4EA5-4032-AAD9-4AEE9219A4AB@amazon.com>
On Fri, Jan 04, 2019 at 11:49:46PM +0000, Bossart, Nathan wrote:
> Here's an updated set of patches with the following changes:
>
> - 0002 adds the parenthesized syntax for ANALYZE.
> - 0003 adds DISABLE_PAGE_SKIPPING for VACUUM.
> - 0003 also ensures SKIP_LOCKED is applied for --analyze-only.
> - 0004 alters vacuumdb to always use the catalog query to discover
> the list of tables to process.
> - 0003, 0005, and 0006 now fail in vacuum_one_database() if a
> specified option is not available on the server version.
> - If tables are listed along with --min-xid-age, --min-mxid-age, or
> --min-relation-size, each table is processed only if it satisfies
> the provided options.
I have been looking at the patch set, and 0001 can actually happen
only once 0005 is applied because this modifies the query doing on
HEAD a full scan of pg_class which would include at least catalog
tables so it can never be empty. For this reason it seems to me that
0001 and 0004 should be merged together as common refactoring, making
sure on the way that all relations exist before processing anything.
As 0005 and 0006 change similar portions of the code, we could also
merge these together in a second patch introducing the new options.
Keeping 0001, 0004, 0005 and 0006 separated eases the review, but I
would likely merge things when they make sense together to reduce
diff chunks.
0002 removes some carefully-written query generation, so as it is
never possible to generate something like ANALYZE FULL. HEAD splits
ANALYZE and VACUUM clearly, but 0002 merges them together so as
careless coding in vacuumdb.c makes it easier to generate command
strings which would fail in the backend relying on the option checks
to make sure that for example combining --full and --analyze-only
never happens. Introducing 0002 is mainly here for 0003, so let's
merge them together.
> 0004 introduces a slight change to existing behavior. Currently, if
> you specify a missing table, vacuumdb will process each table until
> it reaches the nonexistent one, at which point it will fail. After
> 0004 is applied, vacuumdb will fail during the catalog query, and no
> tables will be processed. I considered avoiding this change in
> behavior by doing an additional catalog lookup for each specified
> table to see whether it satisfies --min-xid-age, etc. However, this
> introduced quite a bit more complexity and increased the number of
> catalog queries needed.
Simplicity is always welcome, knowing that tables are missing before
doing any operations is more consistent with plain VACUUM/ANALYZE.
From patch 0004:
+ executeCommand(conn, "RESET search_path;", progname, echo);
+ res = executeQuery(conn, catalog_query.data, progname, echo);
+ termPQExpBuffer(&catalog_query);
+ PQclear(executeQuery(conn, ALWAYS_SECURE_SEARCH_PATH_SQL,
+ progname, echo));
We should really avoid that. There are other things like casts which
tend to be forgotten, and if the catalog lookup query gets more
complicated, I feel that this would again be forgotten, reintroducing
the issues that ALWAYS_SECURE_SEARCH_PATH_SQL is here to fix.
I have put my hands into what you sent, and worked on putting
0002/0003 first into shape, finishing with the attached. I have
simplified the query construction a bit, making it IMO easier to read
and to add new options, with comments documenting what's supported
across versions. I have also added a regression test for
--analyze-only --skip-locked. Then I have done some edit of the docs.
What do you think for this portion?
--
Michael
Attachments:vacuumdb-skip-disable.patch (text/x-diff, 7.8K) downloaddiff --git a/doc/src/sgml/ref/vacuumdb.sgml b/doc/src/sgml/ref/vacuumdb.sgmlindex 955a17a849..da4d51e763 100644--- a/doc/src/sgml/ref/vacuumdb.sgml+++ b/doc/src/sgml/ref/vacuumdb.sgml@@ -102,6 +102,23 @@ PostgreSQL documentation
</listitem>
</varlistentry>
+ <varlistentry>+ <term><option>--disable-page-skipping</option></term>+ <listitem>+ <para>+ Disable all page-skipping behavior during processing based on+ the visibility map, similarly to the option+ <literal>DISABLE_PAGE_SKIPPING</literal> for <command>VACUUM</command>.+ </para>+ <note>+ <para>+ This option is only available for servers running+ <productname>PostgreSQL</productname> 9.6 and later.+ </para>+ </note>+ </listitem>+ </varlistentry>+
<varlistentry>
<term><option>-e</option></term>
<term><option>--echo</option></term>
@@ -167,6 +184,21 @@ PostgreSQL documentation
</listitem>
</varlistentry>
+ <varlistentry>+ <term><option>--skip-locked</option></term>+ <listitem>+ <para>+ Skip relations that cannot be immediately locked for processing.+ </para>+ <note>+ <para>+ This option is only available for servers running+ <productname>PostgreSQL</productname> 12 and later.+ </para>+ </note>+ </listitem>+ </varlistentry>+
<varlistentry>
<term><option>-t <replaceable class="parameter">table</replaceable> [ (<replaceable class="parameter">column</replaceable> [,...]) ]</option></term>
<term><option>--table=<replaceable class="parameter">table</replaceable> [ (<replaceable class="parameter">column</replaceable> [,...]) ]</option></term>
diff --git a/src/bin/scripts/t/100_vacuumdb.pl b/src/bin/scripts/t/100_vacuumdb.plindex 4c477a27aa..4a2c71d665 100644--- a/src/bin/scripts/t/100_vacuumdb.pl+++ b/src/bin/scripts/t/100_vacuumdb.pl@@ -3,7 +3,7 @@ use warnings;
use PostgresNode;
use TestLib;
-use Test::More tests => 23;+use Test::More tests => 30;
program_help_ok('vacuumdb');
program_version_ok('vacuumdb');
@@ -33,6 +33,21 @@ $node->issues_sql_like(
[ 'vacuumdb', '-Z', 'postgres' ],
qr/statement: ANALYZE;/,
'vacuumdb -Z');
+$node->issues_sql_like(+ [ 'vacuumdb', '--disable-page-skipping', 'postgres' ],+ qr/statement: VACUUM \(DISABLE_PAGE_SKIPPING\);/,+ 'vacuumdb --disable-page-skipping');+$node->issues_sql_like(+ [ 'vacuumdb', '--skip-locked', 'postgres' ],+ qr/statement: VACUUM \(SKIP_LOCKED\);/,+ 'vacuumdb --skip-locked');+$node->issues_sql_like(+ [ 'vacuumdb', '--skip-locked', '--analyze-only', 'postgres' ],+ qr/statement: ANALYZE \(SKIP_LOCKED\);/,+ 'vacuumdb --skip-locked --analyze-only');+$node->command_fails(+ [qw(vacuumdb --analyze-only --disable-page-skipping postgres)],+ '--analyze-only and --disable-page-skipping specified together');
$node->command_ok([qw(vacuumdb -Z --table=pg_am dbname=template1)],
'vacuumdb with connection string');
diff --git a/src/bin/scripts/vacuumdb.c b/src/bin/scripts/vacuumdb.cindex e0a8303f3e..0b82f5ed50 100644--- a/src/bin/scripts/vacuumdb.c+++ b/src/bin/scripts/vacuumdb.c@@ -40,6 +40,8 @@ typedef struct vacuumingOptions
bool and_analyze;
bool full;
bool freeze;
+ bool disable_page_skipping;+ bool skip_locked;
} vacuumingOptions;
@@ -110,6 +112,8 @@ main(int argc, char *argv[])
{"jobs", required_argument, NULL, 'j'},
{"maintenance-db", required_argument, NULL, 2},
{"analyze-in-stages", no_argument, NULL, 3},
+ {"disable-page-skipping", no_argument, NULL, 4},+ {"skip-locked", no_argument, NULL, 5},
{NULL, 0, NULL, 0}
};
@@ -213,6 +217,12 @@ main(int argc, char *argv[])
case 3:
analyze_in_stages = vacopts.analyze_only = true;
break;
+ case 4:+ vacopts.disable_page_skipping = true;+ break;+ case 5:+ vacopts.skip_locked = true;+ break;
default:
fprintf(stderr, _("Try \"%s --help\" for more information.\n"), progname);
exit(1);
@@ -251,6 +261,12 @@ main(int argc, char *argv[])
progname, "freeze");
exit(1);
}
+ if (vacopts.disable_page_skipping)+ {+ fprintf(stderr, _("%s: cannot use the \"%s\" option when performing only analyze\n"),+ progname, "disable-page-skipping");+ exit(1);+ }
/* allow 'and_analyze' with 'analyze_only' */
}
@@ -367,6 +383,20 @@ vacuum_one_database(const char *dbname, vacuumingOptions *vacopts,
conn = connectDatabase(dbname, host, port, username, prompt_password,
progname, echo, false, true);
+ if (vacopts->disable_page_skipping && PQserverVersion(conn) < 90600)+ {+ fprintf(stderr, _("%s: cannot use the \"%s\" option on server versions older than PostgreSQL 9.6\n"),+ progname, "disable-page-skipping");+ exit(1);+ }++ if (vacopts->skip_locked && PQserverVersion(conn) < 120000)+ {+ fprintf(stderr, _("%s: cannot use the \"%s\" option on server versions older than PostgreSQL 12\n"),+ progname, "skip-locked");+ exit(1);+ }+
if (!quiet)
{
if (stage != ANALYZE_NO_STAGE)
@@ -630,23 +660,61 @@ prepare_vacuum_command(PQExpBuffer sql, PGconn *conn,
bool table_pre_qualified,
const char *progname, bool echo)
{
+ const char *paren = " (";+ const char *comma = ", ";+ const char *sep = paren;+
resetPQExpBuffer(sql);
if (vacopts->analyze_only)
{
appendPQExpBufferStr(sql, "ANALYZE");
- if (vacopts->verbose)- appendPQExpBufferStr(sql, " VERBOSE");++ /* parenthesized grammar of ANALYZE is supported since v11 */+ if (PQserverVersion(conn) >= 110000)+ {+ if (vacopts->skip_locked)+ {+ /* SKIP_LOCKED is supported since v12 */+ Assert(PQserverVersion(conn) >= 120000);+ appendPQExpBuffer(sql, "%sSKIP_LOCKED", sep);+ sep = comma;+ }+ if (vacopts->verbose)+ {+ appendPQExpBuffer(sql, "%sVERBOSE", sep);+ sep = comma;+ }+ if (sep != paren)+ appendPQExpBufferChar(sql, ')');+ }+ else+ {+ if (vacopts->verbose)+ appendPQExpBufferStr(sql, " VERBOSE");+ }
}
else
{
appendPQExpBufferStr(sql, "VACUUM");
++ /* parenthesized grammar of VACUUM is supported since v9.0 */
if (PQserverVersion(conn) >= 90000)
{
- const char *paren = " (";- const char *comma = ", ";- const char *sep = paren;-+ if (vacopts->disable_page_skipping)+ {+ /* DISABLE_PAGE_SKIPPING is supported since v9.6 */+ Assert(PQserverVersion(conn) >= 90600);+ appendPQExpBuffer(sql, "%sDISABLE_PAGE_SKIPPING", sep);+ sep = comma;+ }+ if (vacopts->skip_locked)+ {+ /* SKIP_LOCKED is supported since v12 */+ Assert(PQserverVersion(conn) >= 120000);+ appendPQExpBuffer(sql, "%sSKIP_LOCKED", sep);+ sep = comma;+ }
if (vacopts->full)
{
appendPQExpBuffer(sql, "%sFULL", sep);
@@ -1000,11 +1068,13 @@ help(const char *progname)
printf(_("\nOptions:\n"));
printf(_(" -a, --all vacuum all databases\n"));
printf(_(" -d, --dbname=DBNAME database to vacuum\n"));
+ printf(_(" --disable-page-skipping disable all page-skipping behavior\n"));
printf(_(" -e, --echo show the commands being sent to the server\n"));
printf(_(" -f, --full do full vacuuming\n"));
printf(_(" -F, --freeze freeze row transaction information\n"));
printf(_(" -j, --jobs=NUM use this many concurrent connections to vacuum\n"));
printf(_(" -q, --quiet don't write any messages\n"));
+ printf(_(" --skip-locked skip relations that cannot be immediately locked\n"));
printf(_(" -t, --table='TABLE[(COLUMNS)]' vacuum specific table(s) only\n"));
printf(_(" -v, --verbose write a lot of output\n"));
printf(_(" -V, --version output version information, then exit\n"));
signature.asc (application/pgp-signature, 832B) 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, robertmhaas@gmail.com
Subject: Re: A few new options for vacuumdb
In-Reply-To: <20190107071146.GD22498@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