Vacuum o/p with (full 1, parallel 0) option throwing an error

On Wed, Apr 8, 2020 at 8:22 AM tushar <tushar.ahuja@enterprisedb.com> wrote:

I just came across this scenario where - vaccum o/p with (full 1,
parallel 0) option not working

--working

postgres=# vacuum (parallel 1, full 0 ) foo;
VACUUM
postgres=#

--Not working

postgres=# vacuum (full 1, parallel 0 ) foo;
ERROR: cannot specify both FULL and PARALLEL options

I think it should work.

Uh, why? There's a clear error message which matches what you tried to do.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Wed, 8 Apr 2020 at 17:59, Robert Haas <robertmhaas@gmail.com> wrote:

On Wed, Apr 8, 2020 at 8:22 AM tushar <tushar.ahuja@enterprisedb.com> wrote:

I just came across this scenario where - vaccum o/p with (full 1,
parallel 0) option not working

--working

postgres=# vacuum (parallel 1, full 0 ) foo;
VACUUM
postgres=#

--Not working

postgres=# vacuum (full 1, parallel 0 ) foo;
ERROR: cannot specify both FULL and PARALLEL options

I think it should work.

Uh, why? There's a clear error message which matches what you tried to do.

I think, Tushar point is that either we should allow both
vacuum(parallel 0, full 1) and vacuum(parallel 1, full 0) or in the
both cases, we should through error.

--
Thanks and Regards
Mahendra Singh Thalor
EnterpriseDB: http://www.enterprisedb.com

On Wed, Apr 8, 2020 at 10:25 AM Mahendra Singh Thalor
<mahi6run@gmail.com> wrote:

I think, Tushar point is that either we should allow both
vacuum(parallel 0, full 1) and vacuum(parallel 1, full 0) or in the
both cases, we should through error.

Oh, yeah, good point. Somebody must not've been careful enough with
the options-checking code.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Wed, Apr 08, 2020 at 11:57:08AM -0400, Robert Haas wrote:

On Wed, Apr 8, 2020 at 10:25 AM Mahendra Singh Thalor
<mahi6run@gmail.com> wrote:

I think, Tushar point is that either we should allow both
vacuum(parallel 0, full 1) and vacuum(parallel 1, full 0) or in the
both cases, we should through error.

Oh, yeah, good point. Somebody must not've been careful enough with
the options-checking code.

Actually I think someone was too careful.

From 9256cdb0a77fb33194727e265a346407921055ef Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryzbyj@telsasoft.com>
Date: Wed, 8 Apr 2020 11:38:36 -0500
Subject: [PATCH v1] parallel vacuum: options check to use same test as in
vacuumlazy.c

---
src/backend/commands/vacuum.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c
index 351d5215a9..660c854d49 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,7 +197,7 @@ 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")));
-- 
2.17.0

On Wed, 8 Apr 2020 at 22:11, Justin Pryzby <pryzby@telsasoft.com> wrote:

On Wed, Apr 08, 2020 at 11:57:08AM -0400, Robert Haas wrote:

On Wed, Apr 8, 2020 at 10:25 AM Mahendra Singh Thalor
<mahi6run@gmail.com> wrote:

I think, Tushar point is that either we should allow both
vacuum(parallel 0, full 1) and vacuum(parallel 1, full 0) or in the
both cases, we should through error.

Oh, yeah, good point. Somebody must not've been careful enough with
the options-checking code.

Actually I think someone was too careful.

From 9256cdb0a77fb33194727e265a346407921055ef Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryzbyj@telsasoft.com>
Date: Wed, 8 Apr 2020 11:38:36 -0500
Subject: [PATCH v1] parallel vacuum: options check to use same test as in
vacuumlazy.c

---
src/backend/commands/vacuum.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c
index 351d5215a9..660c854d49 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,7 +197,7 @@ 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")));
--
2.17.0

Thanks Justin for the patch.

Patch looks fine to me and it is fixing the issue. After applying this
patch, vacuum will work as:

1) vacuum (parallel 1, full 0);
-- vacuuming will be done with 1 parallel worker.
2) vacuum (parallel 0, full 1);
-- full vacuuming will be done.
3) vacuum (parallel 1, full 1);
-- will give error :ERROR: cannot specify both FULL and PARALLEL options

3rd example is telling that we can't give both FULL and PARALLEL
options but in 1st and 2nd, we are giving both FULL and PARALLEL
options and we are not giving any error. I think, irrespective of
value of both FULL and PARALLEL options, we should give error in all
the above mentioned three cases.

Thoughts?

--
Thanks and Regards
Mahendra Singh Thalor
EnterpriseDB: http://www.enterprisedb.com

On Thu, Apr 09, 2020 at 12:06:04AM +0530, Mahendra Singh Thalor wrote:

On Wed, 8 Apr 2020 at 22:11, Justin Pryzby <pryzby@telsasoft.com> wrote:

On Wed, Apr 08, 2020 at 11:57:08AM -0400, Robert Haas wrote:

On Wed, Apr 8, 2020 at 10:25 AM Mahendra Singh Thalor
<mahi6run@gmail.com> wrote:

I think, Tushar point is that either we should allow both
vacuum(parallel 0, full 1) and vacuum(parallel 1, full 0) or in the
both cases, we should through error.

Oh, yeah, good point. Somebody must not've been careful enough with
the options-checking code.

Actually I think someone was too careful.

From 9256cdb0a77fb33194727e265a346407921055ef Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryzbyj@telsasoft.com>
Date: Wed, 8 Apr 2020 11:38:36 -0500
Subject: [PATCH v1] parallel vacuum: options check to use same test as in
vacuumlazy.c

---
src/backend/commands/vacuum.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c
index 351d5215a9..660c854d49 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,7 +197,7 @@ 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")));
--
2.17.0

Thanks Justin for the patch.

Patch looks fine to me and it is fixing the issue. After applying this
patch, vacuum will work as:

1) vacuum (parallel 1, full 0);
-- vacuuming will be done with 1 parallel worker.
2) vacuum (parallel 0, full 1);
-- full vacuuming will be done.
3) vacuum (parallel 1, full 1);
-- will give error :ERROR: cannot specify both FULL and PARALLEL options

3rd example is telling that we can't give both FULL and PARALLEL
options but in 1st and 2nd, we are giving both FULL and PARALLEL
options and we are not giving any error. I think, irrespective of
value of both FULL and PARALLEL options, we should give error in all
the above mentioned three cases.

I think the behavior is correct, but the error message could be improved, like:
|ERROR: cannot specify FULL with PARALLEL jobs
or similar.

--
Justin

On Wed, Apr 08, 2020 at 01:38:54PM -0500, Justin Pryzby wrote:

I think the behavior is correct, but the error message could be improved, like:
|ERROR: cannot specify FULL with PARALLEL jobs
or similar.

Perhaps "cannot use FULL and PARALLEL options together"? I think that
this patch needs tests in sql/vacuum.sql.
--
Michael

On Thu, Apr 9, 2020 at 7:07 AM Michael Paquier <michael@paquier.xyz> wrote:

On Wed, Apr 08, 2020 at 01:38:54PM -0500, Justin Pryzby wrote:

I think the behavior is correct, but the error message could be improved, like:
|ERROR: cannot specify FULL with PARALLEL jobs
or similar.

Perhaps "cannot use FULL and PARALLEL options together"?

We already have a similar message "cannot specify both PARSER and COPY
options", so I think the current message is fine.

I think that
this patch needs tests in sql/vacuum.sql.

We already have one test that is testing an invalid combination of
PARALLEL and FULL option, not sure of adding more on similar lines is
a good idea, but we can do that if it makes sense. What more tests
you have in mind which make sense here?

--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On Thu, Apr 9, 2020 at 12:09 AM Justin Pryzby <pryzby@telsasoft.com> wrote:

On Thu, Apr 09, 2020 at 12:06:04AM +0530, Mahendra Singh Thalor wrote:

On Wed, 8 Apr 2020 at 22:11, Justin Pryzby <pryzby@telsasoft.com> wrote:

Thanks Justin for the patch.

Patch looks fine to me and it is fixing the issue. After applying this
patch, vacuum will work as:

1) vacuum (parallel 1, full 0);
-- vacuuming will be done with 1 parallel worker.
2) vacuum (parallel 0, full 1);
-- full vacuuming will be done.
3) vacuum (parallel 1, full 1);
-- will give error :ERROR: cannot specify both FULL and PARALLEL options

3rd example is telling that we can't give both FULL and PARALLEL
options but in 1st and 2nd, we are giving both FULL and PARALLEL
options and we are not giving any error. I think, irrespective of
value of both FULL and PARALLEL options, we should give error in all
the above mentioned three cases.

I think the behavior is correct, but the error message could be improved,

Yeah, I also think that the behavior is fine. We can do what Mahendra
is saying but that will unnecessarily block some syntax and we might
need to introduce an extra variable to detect that in code.

like:
|ERROR: cannot specify FULL with PARALLEL jobs
or similar.

I don't see much problem with the current error message as a similar
message is used someplace else also as mentioned in my previous reply.
However, we can change it if we feel the current message is not
conveying the cause of the problem.

--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On Thu, 9 Apr 2020 at 14:52, Amit Kapila <amit.kapila16@gmail.com> wrote:

On Thu, Apr 9, 2020 at 12:09 AM Justin Pryzby <pryzby@telsasoft.com> wrote:

On Thu, Apr 09, 2020 at 12:06:04AM +0530, Mahendra Singh Thalor wrote:

On Wed, 8 Apr 2020 at 22:11, Justin Pryzby <pryzby@telsasoft.com> wrote:

Thanks Justin for the patch.

Patch looks fine to me and it is fixing the issue. After applying this
patch, vacuum will work as:

1) vacuum (parallel 1, full 0);
-- vacuuming will be done with 1 parallel worker.
2) vacuum (parallel 0, full 1);
-- full vacuuming will be done.
3) vacuum (parallel 1, full 1);
-- will give error :ERROR: cannot specify both FULL and PARALLEL options

3rd example is telling that we can't give both FULL and PARALLEL
options but in 1st and 2nd, we are giving both FULL and PARALLEL
options and we are not giving any error. I think, irrespective of
value of both FULL and PARALLEL options, we should give error in all
the above mentioned three cases.

I think the behavior is correct, but the error message could be improved,

Yeah, I also think that the behavior is fine.

Me too.

We can do what Mahendra
is saying but that will unnecessarily block some syntax and we might
need to introduce an extra variable to detect that in code.

ISTM we have been using the expression "specifying the option" in log
messages when a user wrote the option in the command. But now that
VACUUM command options can have a true/false it doesn't make sense to
say like "if the option is specified we cannot do that". So maybe
"cannot turn on FULL and PARALLEL options" or something would be
better, I think.

Regards,

--
Masahiko Sawada http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Thu, Apr 09, 2020 at 11:05:50AM +0530, Amit Kapila wrote:

On Thu, Apr 9, 2020 at 7:07 AM Michael Paquier <michael@paquier.xyz> wrote:

I think that
this patch needs tests in sql/vacuum.sql.

We already have one test that is testing an invalid combination of
PARALLEL and FULL option, not sure of adding more on similar lines is
a good idea, but we can do that if it makes sense. What more tests
you have in mind which make sense here?

As you say, vacuum.sql includes this test:
VACUUM (PARALLEL 2, FULL TRUE) pvactst; -- error, cannot use both PARALLEL and FULL
ERROR: cannot specify both FULL and PARALLEL options

But based on the discussion of this thread, it seems to me that we had
better stress more option combinations, particularly the two following
ones:
vacuum (full 0, parallel 1) foo;
vacuum (full 1, parallel 0) foo;

Without that, how do you make sure that the compatibility wanted does
not break again in the future? As of HEAD, the first one passes and
the second one fails. And as Tushar is telling us we want to
handle both cases in a consistent way.
--
Michael

On Thu, Apr 9, 2020 at 11:54 AM Masahiko Sawada
<masahiko.sawada@2ndquadrant.com> wrote:

On Thu, 9 Apr 2020 at 14:52, Amit Kapila <amit.kapila16@gmail.com> wrote:

We can do what Mahendra
is saying but that will unnecessarily block some syntax and we might
need to introduce an extra variable to detect that in code.

ISTM we have been using the expression "specifying the option" in log
messages when a user wrote the option in the command. But now that
VACUUM command options can have a true/false it doesn't make sense to
say like "if the option is specified we cannot do that". So maybe
"cannot turn on FULL and PARALLEL options" or something would be
better, I think.

Sure, we can change that, but isn't the existing example of similar
message "cannot specify both PARSER and COPY options" occurs when
both the options have valid values? If so, we can use a similar
principle here, no?

--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On Thu, Apr 9, 2020 at 12:14 PM Michael Paquier <michael@paquier.xyz> wrote:

On Thu, Apr 09, 2020 at 11:05:50AM +0530, Amit Kapila wrote:

On Thu, Apr 9, 2020 at 7:07 AM Michael Paquier <michael@paquier.xyz> wrote:

I think that
this patch needs tests in sql/vacuum.sql.

We already have one test that is testing an invalid combination of
PARALLEL and FULL option, not sure of adding more on similar lines is
a good idea, but we can do that if it makes sense. What more tests
you have in mind which make sense here?

As you say, vacuum.sql includes this test:
VACUUM (PARALLEL 2, FULL TRUE) pvactst; -- error, cannot use both PARALLEL and FULL
ERROR: cannot specify both FULL and PARALLEL options

But based on the discussion of this thread, it seems to me that we had
better stress more option combinations, particularly the two following
ones:
vacuum (full 0, parallel 1) foo;
vacuum (full 1, parallel 0) foo;

Without that, how do you make sure that the compatibility wanted does
not break again in the future? As of HEAD, the first one passes and
the second one fails. And as Tushar is telling us we want to
handle both cases in a consistent way.

We can add more tests to validate the syntax, but my worry was to not
increase test timing by doing (parallel) vacuum. So maybe we can do
such syntax validation on empty tables or you have any better idea?

--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On Thu, Apr 09, 2020 at 12:33:57PM +0530, Amit Kapila wrote:

We can add more tests to validate the syntax, but my worry was to not
increase test timing by doing (parallel) vacuum. So maybe we can do
such syntax validation on empty tables or you have any better idea?

Using empty tables for positive tests is the least expensive option.
--
Michael

On Thu, Apr 09, 2020 at 12:31:55PM +0530, Amit Kapila wrote:

Sure, we can change that, but isn't the existing example of similar
message "cannot specify both PARSER and COPY options" occurs when
both the options have valid values? If so, we can use a similar
principle here, no?

A better comparison is with this one:

src/bin/pg_dump/pg_restore.c: pg_log_error("cannot specify both --single-transaction and multiple jobs");

but it doesn't say just: "..specify both --single and --jobs", which would be
wrong in the same way, and which we already dealt with some time ago:

commit 14a4f6f3748df4ff63bb2d2d01146b2b98df20ef
Author: Alvaro Herrera <alvherre@alvh.no-ip.org>
Date: Tue Apr 14 00:06:35 2009 +0000

pg_restore -jN does not equate "multiple jobs", so partly revert the
previous patch.

Per note from Tom.

--
Justin

On Thu, 9 Apr 2020 at 16:02, Amit Kapila <amit.kapila16@gmail.com> wrote:

On Thu, Apr 9, 2020 at 11:54 AM Masahiko Sawada
<masahiko.sawada@2ndquadrant.com> wrote:

On Thu, 9 Apr 2020 at 14:52, Amit Kapila <amit.kapila16@gmail.com> wrote:

We can do what Mahendra
is saying but that will unnecessarily block some syntax and we might
need to introduce an extra variable to detect that in code.

ISTM we have been using the expression "specifying the option" in log
messages when a user wrote the option in the command. But now that
VACUUM command options can have a true/false it doesn't make sense to
say like "if the option is specified we cannot do that". So maybe
"cannot turn on FULL and PARALLEL options" or something would be
better, I think.

Sure, we can change that, but isn't the existing example of similar
message "cannot specify both PARSER and COPY options" occurs when
both the options have valid values? If so, we can use a similar
principle here, no?

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.

Regards,

--
Masahiko Sawada http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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.

--
Justin

On Thu, Apr 9, 2020 at 1:36 AM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Thu, Apr 9, 2020 at 7:07 AM Michael Paquier <michael@paquier.xyz> wrote:

On Wed, Apr 08, 2020 at 01:38:54PM -0500, Justin Pryzby wrote:

I think the behavior is correct, but the error message could be improved, like:
|ERROR: cannot specify FULL with PARALLEL jobs
or similar.

Perhaps "cannot use FULL and PARALLEL options together"?

We already have a similar message "cannot specify both PARSER and COPY
options", so I think the current message is fine.

So, whether the error message is OK depends on the details. The
situation as I understand it is that a vacuum cannot be both parallel
and full. If you disallow every command that includes both key words,
then the message seems fine. But suppose you allow

VACUUM (PARALLEL 1, FULL 0) foo;

There's no technical problem here, because the vacuum is not both
parallel and full. But if you allow that case, then there is an error
message problem, perhaps, because your error message says that you
cannot specify both options, but here you did specify both options,
and yet it worked. So really if this case is allowed a more accurate
error message would be something like:

ERROR: VACUUM FULL cannot be performed in parallel

But if you used this latter error message yet disallowed VACUUM
(PARALLEL 1, FULL 0) then it again wouldn't make sense, because the
error message would be now forbidding something that you never tried
to do.

The point is that we need to decide whether we're going to complain
whenever both options are specified in the syntax, or whether we're
going to complain when they're combined in a way that we don't
support. The error message we choose should match whatever decision we
make there.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

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?

--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com