On 2018-01-11 08:14:42 +0900, Michael Paquier wrote:
> On Wed, Jan 10, 2018 at 05:26:43PM +0000, Bossart, Nathan wrote:
> > Perhaps any logging changes for VACOPT_NOWAIT could be handled in a
> > separate thread. For now, I've updated 0003 to remove the logging
> > changes.
>
> Thanks. I am marking those as ready for committer, you are providing the
> set patch patch which offer the most consistent experience.
I was working on committing 0002 and 0003, when I noticed that the
second patch doesn't actually fully works. NOWAIT does what it says on
the tin iff the table is locked with a lower lock level than access
exclusive. But if AEL is used, the command is blocked in
static List *
expand_vacuum_rel(VacuumRelation *vrel)
...
/*
* We transiently take AccessShareLock to protect the syscache
lookup
* below, as well as find_all_inheritors's expectation that the
caller
* holds some lock on the starting relation.
*/
relid = RangeVarGetRelid(vrel->relation, AccessShareLock,
false);
ISTM has been added after the patches initially were proposed. See
http://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=19de0ab23ccba12567c18640f00b49f01471018d
I'm a bit disappointed that the tests didn't catch this, they're clearly
not fully there. They definitely should test the AEL case, as
demonstrated here.
Independent of that, I'm also concerned that NOWAIT isn't implemented
consistently with other commands. Aren't we erroring out in other uses
of NOWAIT? ISTM a more appropriate keyword would have been SKIP
LOCKED. I think the behaviour makes sense, but I'd rename the internal
flag and the grammar to use SKIP LOCKED.
Lightly edited patches attached. Please preserve commit messages while
fixing these issues.
Greetings,
Andres Freund
>From 3a59265b8ae9837a16aed76773d638f7392a395b Mon Sep 17 00:00:00 2001
From: Andres Freund <and...@anarazel.de>
Date: Sat, 3 Mar 2018 15:47:53 -0800
Subject: [PATCH v5 1/2] Add parenthesized options syntax for ANALYZE.
This is analogous to the syntax allowed for VACUUM. This allows us to
avoid making new options reserved keywords and makes it easier to
allow arbitrary argument order. Oh, and it's consistent with the other
commands, too.
Author: Nathan Bossart
Reviewed-By: Michael Paquier, Masahiko Sawada
Discussion: https://postgr.es/m/d3fc73e2-9b1a-4db4-8180-55f57d116...@amazon.com
---
doc/src/sgml/ref/analyze.sgml | 14 +++++++++++++-
src/backend/parser/gram.y | 17 +++++++++++++++++
src/test/regress/expected/vacuum.out | 7 +++++++
src/test/regress/sql/vacuum.sql | 4 ++++
4 files changed, 41 insertions(+), 1 deletion(-)
diff --git a/doc/src/sgml/ref/analyze.sgml b/doc/src/sgml/ref/analyze.sgml
index 83b07a03003..10b3a9a6733 100644
--- a/doc/src/sgml/ref/analyze.sgml
+++ b/doc/src/sgml/ref/analyze.sgml
@@ -21,9 +21,14 @@ PostgreSQL documentation
<refsynopsisdiv>
<synopsis>
+ANALYZE [ ( <replaceable class="parameter">option</replaceable> [, ...] ) ] [ <replaceable class="parameter">table_and_columns</replaceable> [, ...] ]
ANALYZE [ VERBOSE ] [ <replaceable class="parameter">table_and_columns</replaceable> [, ...] ]
-<phrase>where <replaceable class="parameter">table_and_columns</replaceable> is:</phrase>
+<phrase>where <replaceable class="parameter">option</replaceable> can be one of:</phrase>
+
+ VERBOSE
+
+<phrase>and <replaceable class="parameter">table_and_columns</replaceable> is:</phrase>
<replaceable class="parameter">table_name</replaceable> [ ( <replaceable class="parameter">column_name</replaceable> [, ...] ) ]
</synopsis>
@@ -49,6 +54,13 @@ ANALYZE [ VERBOSE ] [ <replaceable class="parameter">table_and_columns</replacea
It is further possible to give a list of column names for a table,
in which case only the statistics for those columns are collected.
</para>
+
+ <para>
+ When the option list is surrounded by parentheses, the options can be
+ written in any order. The parenthesized syntax was added in
+ <productname>PostgreSQL</productname> 11; the unparenthesized syntax
+ is deprecated.
+ </para>
</refsect1>
<refsect1>
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index d99f2be2c97..1d7317c6eac 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -306,6 +306,7 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
%type <ival> opt_lock lock_type cast_context
%type <ival> vacuum_option_list vacuum_option_elem
+ analyze_option_list analyze_option_elem
%type <boolean> opt_or_replace
opt_grant_grant_option opt_grant_admin_option
opt_nowait opt_if_exists opt_with_data
@@ -10548,6 +10549,22 @@ AnalyzeStmt: analyze_keyword opt_verbose opt_vacuum_relation_list
n->rels = $3;
$$ = (Node *)n;
}
+ | analyze_keyword '(' analyze_option_list ')' opt_vacuum_relation_list
+ {
+ VacuumStmt *n = makeNode(VacuumStmt);
+ n->options = VACOPT_ANALYZE | $3;
+ n->rels = $5;
+ $$ = (Node *) n;
+ }
+ ;
+
+analyze_option_list:
+ analyze_option_elem { $$ = $1; }
+ | analyze_option_list ',' analyze_option_elem { $$ = $1 | $3; }
+ ;
+
+analyze_option_elem:
+ VERBOSE { $$ = VACOPT_VERBOSE; }
;
analyze_keyword:
diff --git a/src/test/regress/expected/vacuum.out b/src/test/regress/expected/vacuum.out
index c440c7ea58f..d66e2aa3b70 100644
--- a/src/test/regress/expected/vacuum.out
+++ b/src/test/regress/expected/vacuum.out
@@ -112,6 +112,13 @@ ANALYZE vactst, does_not_exist, vacparted;
ERROR: relation "does_not_exist" does not exist
ANALYZE vactst (i), vacparted (does_not_exist);
ERROR: column "does_not_exist" of relation "vacparted" does not exist
+-- parenthesized syntax for ANALYZE
+ANALYZE (VERBOSE) does_not_exist;
+ERROR: relation "does_not_exist" does not exist
+ANALYZE (nonexistant-arg) does_not_exist;
+ERROR: syntax error at or near "nonexistant"
+LINE 1: ANALYZE (nonexistant-arg) does_not_exist;
+ ^
DROP TABLE vaccluster;
DROP TABLE vactst;
DROP TABLE vacparted;
diff --git a/src/test/regress/sql/vacuum.sql b/src/test/regress/sql/vacuum.sql
index 92eaca2a93b..275ce2e270f 100644
--- a/src/test/regress/sql/vacuum.sql
+++ b/src/test/regress/sql/vacuum.sql
@@ -89,6 +89,10 @@ ANALYZE vacparted (b), vactst;
ANALYZE vactst, does_not_exist, vacparted;
ANALYZE vactst (i), vacparted (does_not_exist);
+-- parenthesized syntax for ANALYZE
+ANALYZE (VERBOSE) does_not_exist;
+ANALYZE (nonexistant-arg) does_not_exist;
+
DROP TABLE vaccluster;
DROP TABLE vactst;
DROP TABLE vacparted;
--
2.15.1.354.g95ec6b1b33.dirty
>From 1f38a54097328bb80a279d63474fdf31bf52fabe Mon Sep 17 00:00:00 2001
From: Andres Freund <and...@anarazel.de>
Date: Sat, 3 Mar 2018 16:09:18 -0800
Subject: [PATCH v5 2/2] Add NOWAIT option to VACUUM and ANALYZE.
FIXME: blocking doesn't work for AEL
FIXME: NOWAIT isn't actually implemented, closer to SKIP LOCKED
Author: Nathan Bossart
Reviewed-By: Michael Paquier, Andres Freund, Masahiko Sawada
Discussion: https://postgr.es/m/d3fc73e2-9b1a-4db4-8180-55f57d116...@amazon.com
---
doc/src/sgml/ref/analyze.sgml | 12 +++++
doc/src/sgml/ref/vacuum.sgml | 12 +++++
src/backend/parser/gram.y | 2 +
src/include/nodes/parsenodes.h | 2 +-
src/test/isolation/expected/vacuum-nowait.out | 64 +++++++++++++++++++++++++++
src/test/isolation/isolation_schedule | 1 +
src/test/isolation/specs/vacuum-nowait.spec | 42 ++++++++++++++++++
src/test/regress/expected/vacuum.out | 9 ++++
src/test/regress/sql/vacuum.sql | 8 ++++
9 files changed, 151 insertions(+), 1 deletion(-)
create mode 100644 src/test/isolation/expected/vacuum-nowait.out
create mode 100644 src/test/isolation/specs/vacuum-nowait.spec
diff --git a/doc/src/sgml/ref/analyze.sgml b/doc/src/sgml/ref/analyze.sgml
index 10b3a9a6733..05882c30328 100644
--- a/doc/src/sgml/ref/analyze.sgml
+++ b/doc/src/sgml/ref/analyze.sgml
@@ -27,6 +27,7 @@ ANALYZE [ VERBOSE ] [ <replaceable class="parameter">table_and_columns</replacea
<phrase>where <replaceable class="parameter">option</replaceable> can be one of:</phrase>
VERBOSE
+ NOWAIT
<phrase>and <replaceable class="parameter">table_and_columns</replaceable> is:</phrase>
@@ -76,6 +77,17 @@ ANALYZE [ VERBOSE ] [ <replaceable class="parameter">table_and_columns</replacea
</listitem>
</varlistentry>
+ <varlistentry>
+ <term><literal>NOWAIT</literal></term>
+ <listitem>
+ <para>
+ Specifies that <command>ANALYZE</command> should not wait for any
+ conflicting locks to be released: if a relation cannot be locked
+ immediately without waiting, the relation is skipped.
+ </para>
+ </listitem>
+ </varlistentry>
+
<varlistentry>
<term><replaceable class="parameter">table_name</replaceable></term>
<listitem>
diff --git a/doc/src/sgml/ref/vacuum.sgml b/doc/src/sgml/ref/vacuum.sgml
index b760e8ede18..1d7ffef734f 100644
--- a/doc/src/sgml/ref/vacuum.sgml
+++ b/doc/src/sgml/ref/vacuum.sgml
@@ -31,6 +31,7 @@ VACUUM [ FULL ] [ FREEZE ] [ VERBOSE ] [ ANALYZE ] [ <replaceable class="paramet
VERBOSE
ANALYZE
DISABLE_PAGE_SKIPPING
+ NOWAIT
<phrase>and <replaceable class="parameter">table_and_columns</replaceable> is:</phrase>
@@ -160,6 +161,17 @@ VACUUM [ FULL ] [ FREEZE ] [ VERBOSE ] [ ANALYZE ] [ <replaceable class="paramet
</listitem>
</varlistentry>
+ <varlistentry>
+ <term><literal>NOWAIT</literal></term>
+ <listitem>
+ <para>
+ Specifies that <command>VACUUM</command> should not wait for any
+ conflicting locks to be released: if a relation cannot be locked
+ immediately without waiting, the relation is skipped.
+ </para>
+ </listitem>
+ </varlistentry>
+
<varlistentry>
<term><replaceable class="parameter">table_name</replaceable></term>
<listitem>
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index 1d7317c6eac..54ac7aaaee6 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -10538,6 +10538,7 @@ vacuum_option_elem:
errmsg("unrecognized VACUUM option \"%s\"", $1),
parser_errposition(@1)));
}
+ | NOWAIT { $$ = VACOPT_NOWAIT; }
;
AnalyzeStmt: analyze_keyword opt_verbose opt_vacuum_relation_list
@@ -10565,6 +10566,7 @@ analyze_option_list:
analyze_option_elem:
VERBOSE { $$ = VACOPT_VERBOSE; }
+ | NOWAIT { $$ = VACOPT_NOWAIT; }
;
analyze_keyword:
diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index ac292bc6e7a..56e22ae29a2 100644
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -3125,7 +3125,7 @@ typedef enum VacuumOption
VACOPT_VERBOSE = 1 << 2, /* print progress info */
VACOPT_FREEZE = 1 << 3, /* FREEZE option */
VACOPT_FULL = 1 << 4, /* FULL (non-concurrent) vacuum */
- VACOPT_NOWAIT = 1 << 5, /* don't wait to get lock (autovacuum only) */
+ VACOPT_NOWAIT = 1 << 5, /* don't wait to get lock */
VACOPT_SKIPTOAST = 1 << 6, /* don't process the TOAST table, if any */
VACOPT_DISABLE_PAGE_SKIPPING = 1 << 7 /* don't skip any pages */
} VacuumOption;
diff --git a/src/test/isolation/expected/vacuum-nowait.out b/src/test/isolation/expected/vacuum-nowait.out
new file mode 100644
index 00000000000..225ec23760e
--- /dev/null
+++ b/src/test/isolation/expected/vacuum-nowait.out
@@ -0,0 +1,64 @@
+Parsed test spec with 2 sessions
+
+starting permutation: lock vac_specified commit
+step lock:
+ BEGIN;
+ LOCK part1 IN SHARE MODE;
+
+WARNING: skipping vacuum of "part1" --- lock not available
+step vac_specified: VACUUM (NOWAIT) part1, part2;
+step commit:
+ COMMIT;
+
+
+starting permutation: lock vac_all_parts commit
+step lock:
+ BEGIN;
+ LOCK part1 IN SHARE MODE;
+
+step vac_all_parts: VACUUM (NOWAIT) parted;
+step commit:
+ COMMIT;
+
+
+starting permutation: lock analyze_specified commit
+step lock:
+ BEGIN;
+ LOCK part1 IN SHARE MODE;
+
+WARNING: skipping analyze of "part1" --- lock not available
+step analyze_specified: ANALYZE (NOWAIT) part1, part2;
+step commit:
+ COMMIT;
+
+
+starting permutation: lock analyze_all_parts commit
+step lock:
+ BEGIN;
+ LOCK part1 IN SHARE MODE;
+
+step analyze_all_parts: ANALYZE (NOWAIT) parted;
+step commit:
+ COMMIT;
+
+
+starting permutation: lock vac_analyze_specified commit
+step lock:
+ BEGIN;
+ LOCK part1 IN SHARE MODE;
+
+WARNING: skipping vacuum of "part1" --- lock not available
+step vac_analyze_specified: VACUUM (ANALYZE, NOWAIT) part1, part2;
+step commit:
+ COMMIT;
+
+
+starting permutation: lock vac_analyze_all_parts commit
+step lock:
+ BEGIN;
+ LOCK part1 IN SHARE MODE;
+
+step vac_analyze_all_parts: VACUUM (ANALYZE, NOWAIT) parted;
+step commit:
+ COMMIT;
+
diff --git a/src/test/isolation/isolation_schedule b/src/test/isolation/isolation_schedule
index 74d7d59546a..140b637eafa 100644
--- a/src/test/isolation/isolation_schedule
+++ b/src/test/isolation/isolation_schedule
@@ -66,3 +66,4 @@ test: async-notify
test: vacuum-reltuples
test: timeouts
test: vacuum-concurrent-drop
+test: vacuum-nowait
diff --git a/src/test/isolation/specs/vacuum-nowait.spec b/src/test/isolation/specs/vacuum-nowait.spec
new file mode 100644
index 00000000000..498b0a0583a
--- /dev/null
+++ b/src/test/isolation/specs/vacuum-nowait.spec
@@ -0,0 +1,42 @@
+# Test for the NOWAIT option for VACUUM and ANALYZE commands.
+#
+# This also verifies that log messages are not emitted for skipped relations
+# that were not specified in the VACUUM or ANALYZE command.
+
+setup
+{
+ CREATE TABLE parted (a INT) PARTITION BY LIST (a);
+ CREATE TABLE part1 PARTITION OF parted FOR VALUES IN (1);
+ CREATE TABLE part2 PARTITION OF parted FOR VALUES IN (2);
+}
+
+teardown
+{
+ DROP TABLE IF EXISTS parted;
+}
+
+session "s1"
+step "lock"
+{
+ BEGIN;
+ LOCK part1 IN SHARE MODE;
+}
+step "commit"
+{
+ COMMIT;
+}
+
+session "s2"
+step "vac_specified" { VACUUM (NOWAIT) part1, part2; }
+step "vac_all_parts" { VACUUM (NOWAIT) parted; }
+step "analyze_specified" { ANALYZE (NOWAIT) part1, part2; }
+step "analyze_all_parts" { ANALYZE (NOWAIT) parted; }
+step "vac_analyze_specified" { VACUUM (ANALYZE, NOWAIT) part1, part2; }
+step "vac_analyze_all_parts" { VACUUM (ANALYZE, NOWAIT) parted; }
+
+permutation "lock" "vac_specified" "commit"
+permutation "lock" "vac_all_parts" "commit"
+permutation "lock" "analyze_specified" "commit"
+permutation "lock" "analyze_all_parts" "commit"
+permutation "lock" "vac_analyze_specified" "commit"
+permutation "lock" "vac_analyze_all_parts" "commit"
diff --git a/src/test/regress/expected/vacuum.out b/src/test/regress/expected/vacuum.out
index d66e2aa3b70..f06d975f26c 100644
--- a/src/test/regress/expected/vacuum.out
+++ b/src/test/regress/expected/vacuum.out
@@ -119,6 +119,15 @@ ANALYZE (nonexistant-arg) does_not_exist;
ERROR: syntax error at or near "nonexistant"
LINE 1: ANALYZE (nonexistant-arg) does_not_exist;
^
+-- ensure argument order independence, and that NOWAIT on non-existing
+-- relation still errors out.
+ANALYZE (NOWAIT, VERBOSE) does_not_exist;
+ERROR: relation "does_not_exist" does not exist
+ANALYZE (VERBOSE, NOWAIT) does_not_exist;
+ERROR: relation "does_not_exist" does not exist
+-- nowait option
+VACUUM (NOWAIT) vactst;
+ANALYZE (NOWAIT) vactst;
DROP TABLE vaccluster;
DROP TABLE vactst;
DROP TABLE vacparted;
diff --git a/src/test/regress/sql/vacuum.sql b/src/test/regress/sql/vacuum.sql
index 275ce2e270f..5fb6ad20051 100644
--- a/src/test/regress/sql/vacuum.sql
+++ b/src/test/regress/sql/vacuum.sql
@@ -92,6 +92,14 @@ ANALYZE vactst (i), vacparted (does_not_exist);
-- parenthesized syntax for ANALYZE
ANALYZE (VERBOSE) does_not_exist;
ANALYZE (nonexistant-arg) does_not_exist;
+-- ensure argument order independence, and that NOWAIT on non-existing
+-- relation still errors out.
+ANALYZE (NOWAIT, VERBOSE) does_not_exist;
+ANALYZE (VERBOSE, NOWAIT) does_not_exist;
+
+-- nowait option
+VACUUM (NOWAIT) vactst;
+ANALYZE (NOWAIT) vactst;
DROP TABLE vaccluster;
DROP TABLE vactst;
--
2.15.1.354.g95ec6b1b33.dirty
