Date: Fri, 10 Apr 2020 08:35:32 -0500
From: Justin Pryzby <pryzby@telsasoft.com>
To: Amit Kapila <amit.kapila16@gmail.com>
Cc: Masahiko Sawada <masahiko.sawada@2ndquadrant.com>,
	Mahendra Singh Thalor <mahi6run@gmail.com>,
	Robert Haas <robertmhaas@gmail.com>,
	tushar <tushar.ahuja@enterprisedb.com>,
	pgsql-hackers@postgresql.org
Subject: Re: Vacuum o/p with (full 1, parallel 0) option throwing an error
Message-ID: <20200410133532.GA2228@telsasoft.com>
References: 
 <CA+TgmoYkFYRnmTLgPLPm664UdQFBcJTcDZXzBP6dG0zTniYEWQ@mail.gmail.com>
 <20200408164154.GD2228@telsasoft.com>
 <CAKYtNAogYT+ZNN9Q3eCpdWHVsYR5R-o5p0NtOZHLOkYP-4dxsA@mail.gmail.com>
 <20200408183854.GG2228@telsasoft.com>
 <CAA4eK1J-CQ4_jt85um8WSeCRjkC4aEtB+Ft43_tFHLVS8n_CQA@mail.gmail.com>
 <CA+fd4k5TNziHyAYaeFyL8jHN9aiGnNPv7SJ5YGqj92uZyA_RfQ@mail.gmail.com>
 <CAA4eK1LYqw_MN9=MHcxF6ayvmN2NdaxaqJ5e+f2GobBpfC1AjA@mail.gmail.com>
 <CA+fd4k7cj3rO3prCydHQd_pXShQ2rYbUyEJWeE8o+sXtB_1xmg@mail.gmail.com>
 <20200409083319.GN2228@telsasoft.com>
 <CAA4eK1Ku4X-RYcT+h=ngE8Yt6fMCD2rXpAvG0rh=j=PS+K0zmg@mail.gmail.com>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="4ZVTVymsHR1TEBjP"
Content-Disposition: inline
In-Reply-To: 
 <CAA4eK1Ku4X-RYcT+h=ngE8Yt6fMCD2rXpAvG0rh=j=PS+K0zmg@mail.gmail.com>
User-Agent: Mutt/1.9.4 (2018-02-28)
Precedence: bulk


--4ZVTVymsHR1TEBjP
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

On Fri, Apr 10, 2020 at 10:34:02AM +0530, Amit Kapila wrote:
> On Thu, Apr 9, 2020 at 2:03 PM Justin Pryzby <pryzby@telsasoft.com> wrote:
> > On Thu, Apr 09, 2020 at 05:07:48PM +0900, Masahiko Sawada wrote:
> > > Yes but the difference is that we cannot disable PARSER or COPY by
> > > specifying options whereas we can do something like "VACUUM (FULL
> > > false) tbl" to disable FULL option. I might be misunderstanding the
> > > meaning of "specify" though.
> >
> > You have it right.
> >
> > We should fix the behavior, but change the error message for consistency with
> > that change, like so.
> >
> 
> Okay, but I think the error message suggested by Robert "ERROR: VACUUM
> FULL cannot be performed in parallel" sounds better than what you have
> proposed.  What do you think?

No problem.  I think I was trying to make my text similar to that from
14a4f6f37.

I realized that I didn't sq!uash my last patch, so it didn't include the
functional change (which is maybe what Robert was referring to).

-- 
Justin

--4ZVTVymsHR1TEBjP
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
 filename="v3-0001-Allow-specifying-parallel-0-with-vacuum-full.patch"

From 16ff7441791c481ab97b7eb8b2948e38626c7273 Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryzbyj@telsasoft.com>
Date: Wed, 8 Apr 2020 11:38:36 -0500
Subject: [PATCH v3] Allow specifying "(parallel 0)" with "vacuum(full)"..

..even though full implies parallel 0

Check options using the same test as in vacuumlazy.c

See also similar change long ago:
14a4f6f3748df4ff63bb2d2d01146b2b98df20ef

Discussion:
https://www.postgresql.org/message-id/58c8d171-e665-6fa3-a9d3-d9423b694dae%40enterprisedb.com
---
 src/backend/commands/vacuum.c        | 6 ++----
 src/test/regress/expected/vacuum.out | 5 +++--
 src/test/regress/sql/vacuum.sql      | 3 ++-
 3 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c
index 351d5215a9..9cb65d76a9 100644
--- a/src/backend/commands/vacuum.c
+++ b/src/backend/commands/vacuum.c
@@ -104,7 +104,6 @@ ExecVacuum(ParseState *pstate, VacuumStmt *vacstmt, bool isTopLevel)
 	bool		freeze = false;
 	bool		full = false;
 	bool		disable_page_skipping = false;
-	bool		parallel_option = false;
 	ListCell   *lc;
 
 	/* Set default value */
@@ -145,7 +144,6 @@ ExecVacuum(ParseState *pstate, VacuumStmt *vacstmt, bool isTopLevel)
 			params.truncate = get_vacopt_ternary_value(opt);
 		else if (strcmp(opt->defname, "parallel") == 0)
 		{
-			parallel_option = true;
 			if (opt->arg == NULL)
 			{
 				ereport(ERROR,
@@ -199,10 +197,10 @@ ExecVacuum(ParseState *pstate, VacuumStmt *vacstmt, bool isTopLevel)
 		   !(params.options & (VACOPT_FULL | VACOPT_FREEZE)));
 	Assert(!(params.options & VACOPT_SKIPTOAST));
 
-	if ((params.options & VACOPT_FULL) && parallel_option)
+	if ((params.options & VACOPT_FULL) && params.nworkers > 0)
 		ereport(ERROR,
 				(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
-				 errmsg("cannot specify both FULL and PARALLEL options")));
+				 errmsg("VACUUM FULL cannot be performed in parallel")));
 
 	/*
 	 * Make sure VACOPT_ANALYZE is specified if any column lists are present.
diff --git a/src/test/regress/expected/vacuum.out b/src/test/regress/expected/vacuum.out
index 0cfe28e63f..f90c498813 100644
--- a/src/test/regress/expected/vacuum.out
+++ b/src/test/regress/expected/vacuum.out
@@ -115,14 +115,15 @@ LINE 1: VACUUM (PARALLEL -1) pvactst;
                 ^
 VACUUM (PARALLEL 2, INDEX_CLEANUP FALSE) pvactst;
 VACUUM (PARALLEL 2, FULL TRUE) pvactst; -- error, cannot use both PARALLEL and FULL
-ERROR:  cannot specify both FULL and PARALLEL options
+ERROR:  VACUUM FULL cannot be performed in parallel
+VACUUM (PARALLEL 0, FULL TRUE) pvactst; -- can specify parallel disabled (even though that's implied by FULL)
 VACUUM (PARALLEL) pvactst; -- error, cannot use PARALLEL option without parallel degree
 ERROR:  parallel option requires a value between 0 and 1024
 LINE 1: VACUUM (PARALLEL) pvactst;
                 ^
 CREATE TEMPORARY TABLE tmp (a int PRIMARY KEY);
 CREATE INDEX tmp_idx1 ON tmp (a);
-VACUUM (PARALLEL 1) tmp; -- disables parallel vacuum option
+VACUUM (PARALLEL 1) tmp; -- parallel vacuum disabled for temp tables
 WARNING:  disabling parallel option of vacuum on "tmp" --- cannot vacuum temporary tables in parallel
 RESET min_parallel_index_scan_size;
 DROP TABLE pvactst;
diff --git a/src/test/regress/sql/vacuum.sql b/src/test/regress/sql/vacuum.sql
index cf741f7b11..dade1e5e57 100644
--- a/src/test/regress/sql/vacuum.sql
+++ b/src/test/regress/sql/vacuum.sql
@@ -99,10 +99,11 @@ VACUUM (PARALLEL 0) pvactst; -- disable parallel vacuum
 VACUUM (PARALLEL -1) pvactst; -- error
 VACUUM (PARALLEL 2, INDEX_CLEANUP FALSE) pvactst;
 VACUUM (PARALLEL 2, FULL TRUE) pvactst; -- error, cannot use both PARALLEL and FULL
+VACUUM (PARALLEL 0, FULL TRUE) pvactst; -- can specify parallel disabled (even though that's implied by FULL)
 VACUUM (PARALLEL) pvactst; -- error, cannot use PARALLEL option without parallel degree
 CREATE TEMPORARY TABLE tmp (a int PRIMARY KEY);
 CREATE INDEX tmp_idx1 ON tmp (a);
-VACUUM (PARALLEL 1) tmp; -- disables parallel vacuum option
+VACUUM (PARALLEL 1) tmp; -- parallel vacuum disabled for temp tables
 RESET min_parallel_index_scan_size;
 DROP TABLE pvactst;
 
-- 
2.17.0


--4ZVTVymsHR1TEBjP--