From 05299c00c6933eaf0e97b8b5f512e190c5abe8d3 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <bossartn@amazon.com>
Date: Mon, 5 Mar 2018 21:46:01 +0000
Subject: [PATCH v6 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-55F57D116B4E@amazon.com
---
 doc/src/sgml/ref/analyze.sgml                      |  12 ++
 doc/src/sgml/ref/vacuum.sgml                       |  12 ++
 src/backend/commands/analyze.c                     |   2 +-
 src/backend/commands/vacuum.c                      |  55 ++++++++-
 src/backend/parser/gram.y                          |   2 +
 src/backend/postmaster/autovacuum.c                |   2 +-
 src/include/nodes/parsenodes.h                     |   2 +-
 src/test/isolation/expected/vacuum-skip-locked.out | 129 +++++++++++++++++++++
 src/test/isolation/isolation_schedule              |   1 +
 src/test/isolation/specs/vacuum-skip-locked.spec   |  53 +++++++++
 src/test/regress/expected/vacuum.out               |   9 ++
 src/test/regress/sql/vacuum.sql                    |   9 ++
 12 files changed, 280 insertions(+), 8 deletions(-)
 create mode 100644 src/test/isolation/expected/vacuum-skip-locked.out
 create mode 100644 src/test/isolation/specs/vacuum-skip-locked.spec

diff --git a/doc/src/sgml/ref/analyze.sgml b/doc/src/sgml/ref/analyze.sgml
index 10b3a9a..4e6ae38 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
+    SKIP LOCKED
 
 <phrase>and <replaceable class="parameter">table_and_columns</replaceable> is:</phrase>
 
@@ -77,6 +78,17 @@ ANALYZE [ VERBOSE ] [ <replaceable class="parameter">table_and_columns</replacea
    </varlistentry>
 
    <varlistentry>
+    <term><literal>SKIP LOCKED</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>
      <para>
diff --git a/doc/src/sgml/ref/vacuum.sgml b/doc/src/sgml/ref/vacuum.sgml
index b760e8e..1ef5bc9 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
+    SKIP LOCKED
 
 <phrase>and <replaceable class="parameter">table_and_columns</replaceable> is:</phrase>
 
@@ -161,6 +162,17 @@ VACUUM [ FULL ] [ FREEZE ] [ VERBOSE ] [ ANALYZE ] [ <replaceable class="paramet
    </varlistentry>
 
    <varlistentry>
+    <term><literal>SKIP LOCKED</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>
      <para>
diff --git a/src/backend/commands/analyze.c b/src/backend/commands/analyze.c
index 5f21fcb..68f84c9 100644
--- a/src/backend/commands/analyze.c
+++ b/src/backend/commands/analyze.c
@@ -143,7 +143,7 @@ analyze_rel(Oid relid, RangeVar *relation, int options,
 	 * matter if we ever try to accumulate stats on dead tuples.) If the rel
 	 * has been dropped since we last saw it, we don't need to process it.
 	 */
-	if (!(options & VACOPT_NOWAIT))
+	if (!(options & VACOPT_SKIP_LOCKED))
 		onerel = try_relation_open(relid, ShareUpdateExclusiveLock);
 	else if (ConditionalLockRelationOid(relid, ShareUpdateExclusiveLock))
 		onerel = try_relation_open(relid, NoLock);
diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c
index 7aca69a..62eb95c 100644
--- a/src/backend/commands/vacuum.c
+++ b/src/backend/commands/vacuum.c
@@ -46,6 +46,7 @@
 #include "storage/procarray.h"
 #include "utils/acl.h"
 #include "utils/fmgroids.h"
+#include "utils/formatting.h"
 #include "utils/guc.h"
 #include "utils/memutils.h"
 #include "utils/snapmgr.h"
@@ -68,7 +69,7 @@ static BufferAccessStrategy vac_strategy;
 
 
 /* non-export function prototypes */
-static List *expand_vacuum_rel(VacuumRelation *vrel);
+static List *expand_vacuum_rel(VacuumRelation *vrel, int options, const char *stmttype);
 static List *get_all_vacuum_rels(void);
 static void vac_truncate_clog(TransactionId frozenXID,
 				  MultiXactId minMulti,
@@ -250,6 +251,13 @@ vacuum(int options, List *relations, VacuumParams *params,
 	{
 		List	   *newrels = NIL;
 		ListCell   *lc;
+		const char *stmttype_lower;
+
+		/*
+		 * We must downcase the statement type for log message consistency
+		 * between expand_vacuum_rel(), vacuum_rel(), and analyze_rel().
+		 */
+		stmttype_lower = asc_tolower(stmttype, strlen(stmttype));
 
 		foreach(lc, relations)
 		{
@@ -257,7 +265,7 @@ vacuum(int options, List *relations, VacuumParams *params,
 			List	   *sublist;
 			MemoryContext old_context;
 
-			sublist = expand_vacuum_rel(vrel);
+			sublist = expand_vacuum_rel(vrel, options, stmttype_lower);
 			old_context = MemoryContextSwitchTo(vac_context);
 			newrels = list_concat(newrels, sublist);
 			MemoryContextSwitchTo(old_context);
@@ -423,7 +431,7 @@ vacuum(int options, List *relations, VacuumParams *params,
  * are made in vac_context.
  */
 static List *
-expand_vacuum_rel(VacuumRelation *vrel)
+expand_vacuum_rel(VacuumRelation *vrel, int options, const char *stmttype)
 {
 	List	   *vacrels = NIL;
 	MemoryContext oldcontext;
@@ -444,11 +452,48 @@ expand_vacuum_rel(VacuumRelation *vrel)
 		bool		include_parts;
 
 		/*
+		 * Since autovacuum workers supply OIDs when calling vacuum(), no
+		 * autovacuum worker should reach this code, and we can make
+		 * assumptions about the logging levels we should use in the checks
+		 * below.
+		 */
+		Assert(!IsAutoVacuumWorkerProcess());
+
+		/*
 		 * 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);
+		if (!(options & VACOPT_SKIP_LOCKED))
+			relid = RangeVarGetRelid(vrel->relation, AccessShareLock, false);
+		else
+		{
+			relid = RangeVarGetRelid(vrel->relation, NoLock, false);
+
+			/*
+			 * The relation may go away before we can obtain the lock, so we
+			 * must verify that it still exists afterwards.  If the lock is
+			 * unavailable or the relation has disappeared, emit the same log
+			 * statement that vacuum_rel() and analyze_rel() would.
+			 */
+			if (!ConditionalLockRelationOid(relid, AccessShareLock))
+			{
+				ereport(WARNING,
+						(errcode(ERRCODE_LOCK_NOT_AVAILABLE),
+						 errmsg("skipping %s of \"%s\" --- lock not available",
+								stmttype, vrel->relation->relname)));
+				return vacrels;
+			}
+			else if (!SearchSysCacheExists1(RELOID, ObjectIdGetDatum(relid)))
+			{
+				UnlockRelationOid(relid, AccessShareLock);
+				ereport(WARNING,
+						(errcode(ERRCODE_UNDEFINED_TABLE),
+						 errmsg("skipping %s of \"%s\" --- relation no longer exists",
+								stmttype, vrel->relation->relname)));
+				return vacrels;
+			}
+		}
 
 		/*
 		 * Make a returnable VacuumRelation for this rel.
@@ -1395,7 +1440,7 @@ vacuum_rel(Oid relid, RangeVar *relation, int options, VacuumParams *params)
 	 * If we've been asked not to wait for the relation lock, acquire it first
 	 * in non-blocking mode, before calling try_relation_open().
 	 */
-	if (!(options & VACOPT_NOWAIT))
+	if (!(options & VACOPT_SKIP_LOCKED))
 		onerel = try_relation_open(relid, lmode);
 	else if (ConditionalLockRelationOid(relid, lmode))
 		onerel = try_relation_open(relid, NoLock);
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index 1d7317c..977a030 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)));
 				}
+			| SKIP LOCKED		{ $$ = VACOPT_SKIP_LOCKED; }
 		;
 
 AnalyzeStmt: analyze_keyword opt_verbose opt_vacuum_relation_list
@@ -10565,6 +10566,7 @@ analyze_option_list:
 
 analyze_option_elem:
 			VERBOSE				{ $$ = VACOPT_VERBOSE; }
+			| SKIP LOCKED		{ $$ = VACOPT_SKIP_LOCKED; }
 		;
 
 analyze_keyword:
diff --git a/src/backend/postmaster/autovacuum.c b/src/backend/postmaster/autovacuum.c
index 21f5e2c..77cbfe9 100644
--- a/src/backend/postmaster/autovacuum.c
+++ b/src/backend/postmaster/autovacuum.c
@@ -2883,7 +2883,7 @@ table_recheck_autovac(Oid relid, HTAB *table_toast_map,
 		tab->at_vacoptions = VACOPT_SKIPTOAST |
 			(dovacuum ? VACOPT_VACUUM : 0) |
 			(doanalyze ? VACOPT_ANALYZE : 0) |
-			(!wraparound ? VACOPT_NOWAIT : 0);
+			(!wraparound ? VACOPT_SKIP_LOCKED : 0);
 		tab->at_params.freeze_min_age = freeze_min_age;
 		tab->at_params.freeze_table_age = freeze_table_age;
 		tab->at_params.multixact_freeze_min_age = multixact_freeze_min_age;
diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index ac292bc..744fdc4 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_SKIP_LOCKED = 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-skip-locked.out b/src/test/isolation/expected/vacuum-skip-locked.out
new file mode 100644
index 0000000..cb11672
--- /dev/null
+++ b/src/test/isolation/expected/vacuum-skip-locked.out
@@ -0,0 +1,129 @@
+Parsed test spec with 2 sessions
+
+starting permutation: lock_share vac_specified commit
+step lock_share: 
+	BEGIN;
+	LOCK part1 IN SHARE MODE;
+
+WARNING:  skipping vacuum of "part1" --- lock not available
+step vac_specified: VACUUM (SKIP LOCKED) part1, part2;
+step commit: 
+	COMMIT;
+
+
+starting permutation: lock_share vac_all_parts commit
+step lock_share: 
+	BEGIN;
+	LOCK part1 IN SHARE MODE;
+
+step vac_all_parts: VACUUM (SKIP LOCKED) parted;
+step commit: 
+	COMMIT;
+
+
+starting permutation: lock_share analyze_specified commit
+step lock_share: 
+	BEGIN;
+	LOCK part1 IN SHARE MODE;
+
+WARNING:  skipping analyze of "part1" --- lock not available
+step analyze_specified: ANALYZE (SKIP LOCKED) part1, part2;
+step commit: 
+	COMMIT;
+
+
+starting permutation: lock_share analyze_all_parts commit
+step lock_share: 
+	BEGIN;
+	LOCK part1 IN SHARE MODE;
+
+step analyze_all_parts: ANALYZE (SKIP LOCKED) parted;
+step commit: 
+	COMMIT;
+
+
+starting permutation: lock_share vac_analyze_specified commit
+step lock_share: 
+	BEGIN;
+	LOCK part1 IN SHARE MODE;
+
+WARNING:  skipping vacuum of "part1" --- lock not available
+step vac_analyze_specified: VACUUM (ANALYZE, SKIP LOCKED) part1, part2;
+step commit: 
+	COMMIT;
+
+
+starting permutation: lock_share vac_analyze_all_parts commit
+step lock_share: 
+	BEGIN;
+	LOCK part1 IN SHARE MODE;
+
+step vac_analyze_all_parts: VACUUM (ANALYZE, SKIP LOCKED) parted;
+step commit: 
+	COMMIT;
+
+
+starting permutation: lock_access_exclusive vac_specified commit
+step lock_access_exclusive: 
+	BEGIN;
+	LOCK part1 IN ACCESS EXCLUSIVE MODE;
+
+WARNING:  skipping vacuum of "part1" --- lock not available
+step vac_specified: VACUUM (SKIP LOCKED) part1, part2;
+step commit: 
+	COMMIT;
+
+
+starting permutation: lock_access_exclusive vac_all_parts commit
+step lock_access_exclusive: 
+	BEGIN;
+	LOCK part1 IN ACCESS EXCLUSIVE MODE;
+
+step vac_all_parts: VACUUM (SKIP LOCKED) parted;
+step commit: 
+	COMMIT;
+
+
+starting permutation: lock_access_exclusive analyze_specified commit
+step lock_access_exclusive: 
+	BEGIN;
+	LOCK part1 IN ACCESS EXCLUSIVE MODE;
+
+WARNING:  skipping analyze of "part1" --- lock not available
+step analyze_specified: ANALYZE (SKIP LOCKED) part1, part2;
+step commit: 
+	COMMIT;
+
+
+starting permutation: lock_access_exclusive analyze_all_parts commit
+step lock_access_exclusive: 
+	BEGIN;
+	LOCK part1 IN ACCESS EXCLUSIVE MODE;
+
+step analyze_all_parts: ANALYZE (SKIP LOCKED) parted; <waiting ...>
+step commit: 
+	COMMIT;
+
+step analyze_all_parts: <... completed>
+
+starting permutation: lock_access_exclusive vac_analyze_specified commit
+step lock_access_exclusive: 
+	BEGIN;
+	LOCK part1 IN ACCESS EXCLUSIVE MODE;
+
+WARNING:  skipping vacuum of "part1" --- lock not available
+step vac_analyze_specified: VACUUM (ANALYZE, SKIP LOCKED) part1, part2;
+step commit: 
+	COMMIT;
+
+
+starting permutation: lock_access_exclusive vac_analyze_all_parts commit
+step lock_access_exclusive: 
+	BEGIN;
+	LOCK part1 IN ACCESS EXCLUSIVE MODE;
+
+step vac_analyze_all_parts: VACUUM (ANALYZE, SKIP LOCKED) parted; <waiting ...>
+step commit: 
+	COMMIT;
+
+step vac_analyze_all_parts: <... completed>
diff --git a/src/test/isolation/isolation_schedule b/src/test/isolation/isolation_schedule
index 74d7d59..520734b 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-skip-locked
diff --git a/src/test/isolation/specs/vacuum-skip-locked.spec b/src/test/isolation/specs/vacuum-skip-locked.spec
new file mode 100644
index 0000000..96c3bf4
--- /dev/null
+++ b/src/test/isolation/specs/vacuum-skip-locked.spec
@@ -0,0 +1,53 @@
+# Test for the SKIP LOCKED 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_share"
+{
+	BEGIN;
+	LOCK part1 IN SHARE MODE;
+}
+step "lock_access_exclusive"
+{
+	BEGIN;
+	LOCK part1 IN ACCESS EXCLUSIVE MODE;
+}
+step "commit"
+{
+	COMMIT;
+}
+
+session "s2"
+step "vac_specified"		{ VACUUM (SKIP LOCKED) part1, part2; }
+step "vac_all_parts"		{ VACUUM (SKIP LOCKED) parted; }
+step "analyze_specified"	{ ANALYZE (SKIP LOCKED) part1, part2; }
+step "analyze_all_parts"	{ ANALYZE (SKIP LOCKED) parted; }
+step "vac_analyze_specified"	{ VACUUM (ANALYZE, SKIP LOCKED) part1, part2; }
+step "vac_analyze_all_parts"	{ VACUUM (ANALYZE, SKIP LOCKED) parted; }
+
+permutation "lock_share" "vac_specified" "commit"
+permutation "lock_share" "vac_all_parts" "commit"
+permutation "lock_share" "analyze_specified" "commit"
+permutation "lock_share" "analyze_all_parts" "commit"
+permutation "lock_share" "vac_analyze_specified" "commit"
+permutation "lock_share" "vac_analyze_all_parts" "commit"
+permutation "lock_access_exclusive" "vac_specified" "commit"
+permutation "lock_access_exclusive" "vac_all_parts" "commit"
+permutation "lock_access_exclusive" "analyze_specified" "commit"
+permutation "lock_access_exclusive" "analyze_all_parts" "commit"
+permutation "lock_access_exclusive" "vac_analyze_specified" "commit"
+permutation "lock_access_exclusive" "vac_analyze_all_parts" "commit"
diff --git a/src/test/regress/expected/vacuum.out b/src/test/regress/expected/vacuum.out
index d66e2aa..5adfdb1 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 SKIP LOCKED on non-existing
+-- relation still errors out.
+ANALYZE (SKIP LOCKED, VERBOSE) does_not_exist;
+ERROR:  relation "does_not_exist" does not exist
+ANALYZE (VERBOSE, SKIP LOCKED) does_not_exist;
+ERROR:  relation "does_not_exist" does not exist
+-- nowait option
+VACUUM (SKIP LOCKED) vactst;
+ANALYZE (SKIP LOCKED) 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 275ce2e..908a288 100644
--- a/src/test/regress/sql/vacuum.sql
+++ b/src/test/regress/sql/vacuum.sql
@@ -93,6 +93,15 @@ ANALYZE vactst (i), vacparted (does_not_exist);
 ANALYZE (VERBOSE) does_not_exist;
 ANALYZE (nonexistant-arg) does_not_exist;
 
+-- ensure argument order independence, and that SKIP LOCKED on non-existing
+-- relation still errors out.
+ANALYZE (SKIP LOCKED, VERBOSE) does_not_exist;
+ANALYZE (VERBOSE, SKIP LOCKED) does_not_exist;
+
+-- nowait option
+VACUUM (SKIP LOCKED) vactst;
+ANALYZE (SKIP LOCKED) vactst;
+
 DROP TABLE vaccluster;
 DROP TABLE vactst;
 DROP TABLE vacparted;
-- 
2.7.3.AMZN

