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

On Thu, Apr 9, 2020 at 7:31 PM Robert Haas <robertmhaas@gmail.com> wrote:

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.

I would prefer later as I don't find it a good idea to unnecessarily
block some syntax.

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

On Fri, 10 Apr 2020 at 14:04, Amit Kapila <amit.kapila16@gmail.com> 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?

I totally agree.

Regards,

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

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

On Fri, Apr 10, 2020 at 7:05 PM Justin Pryzby <pryzby@telsasoft.com> wrote:

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).

I think it is better to add a new test for temporary table which has
less data. We don't want to increase test timings to test the
combination of options. I changed that in the attached patch. I will
commit this tomorrow unless you or anyone else has any more comments.

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

On Mon, 13 Apr 2020 at 18:25, Amit Kapila <amit.kapila16@gmail.com> wrote:

On Fri, Apr 10, 2020 at 7:05 PM Justin Pryzby <pryzby@telsasoft.com> wrote:

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).

I think it is better to add a new test for temporary table which has
less data. We don't want to increase test timings to test the
combination of options. I changed that in the attached patch. I will
commit this tomorrow unless you or anyone else has any more comments.

Thank you for updating the patch!

I think we can update the documentation as well. Currently, the
documentation says "This option can't be used with the FULL option."
but we can say instead, for example, "VACUUM FULL can't use parallel
vacuum.".

Also, I'm concerned that the documentation says that VACUUM FULL
cannot use parallel vacuum and we compute the parallel degree when
PARALLEL option is omitted, but the following command is accepted:

postgres(1:55514)=# vacuum (full on) test;
VACUUM

Instead, we can say:

In plain VACUUM (without FULL), if the PARALLEL option is omitted,
then VACUUM decides the number of workers based on the number of
indexes that support parallel vacuum operation on the relation which
is further limited by max_parallel_maintenance_workers.

(it just adds "In plain VACUUM (without FULL)" to the beginning of the
original sentence.)

What do you think?

Regards,

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

On Mon, Apr 13, 2020 at 4:23 PM Masahiko Sawada
<masahiko.sawada@2ndquadrant.com> wrote:

On Mon, 13 Apr 2020 at 18:25, Amit Kapila <amit.kapila16@gmail.com> wrote:

On Fri, Apr 10, 2020 at 7:05 PM Justin Pryzby <pryzby@telsasoft.com> wrote:

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).

I think it is better to add a new test for temporary table which has
less data. We don't want to increase test timings to test the
combination of options. I changed that in the attached patch. I will
commit this tomorrow unless you or anyone else has any more comments.

Thank you for updating the patch!

I think we can update the documentation as well. Currently, the
documentation says "This option can't be used with the FULL option."
but we can say instead, for example, "VACUUM FULL can't use parallel
vacuum.".

I am not very sure about this. I don't think the current text is wrong
especially when you see the value we can specify there is described
as: "Specifies a non-negative integer value passed to the selected
option.". However, we can consider changing it if others also think
the proposed text or something like that is better than current text.

Also, I'm concerned that the documentation says that VACUUM FULL
cannot use parallel vacuum and we compute the parallel degree when
PARALLEL option is omitted, but the following command is accepted:

postgres(1:55514)=# vacuum (full on) test;
VACUUM

Instead, we can say:

In plain VACUUM (without FULL), if the PARALLEL option is omitted,
then VACUUM decides the number of workers based on the number of
indexes that support parallel vacuum operation on the relation which
is further limited by max_parallel_maintenance_workers.

(it just adds "In plain VACUUM (without FULL)" to the beginning of the
original sentence.)

Yeah, something on these lines would be a good idea. Note that, we are
already planning to slightly change this particular sentence in
another patch [1]/messages/by-id/20200322021801.GB2563@telsasoft.com.

[1]: /messages/by-id/20200322021801.GB2563@telsasoft.com

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

On Mon, Apr 13, 2020 at 05:55:43PM +0530, Amit Kapila wrote:

On Mon, Apr 13, 2020 at 4:23 PM Masahiko Sawada
<masahiko.sawada@2ndquadrant.com> wrote:
I am not very sure about this. I don't think the current text is wrong
especially when you see the value we can specify there is described
as: "Specifies a non-negative integer value passed to the selected
option.". However, we can consider changing it if others also think
the proposed text or something like that is better than current text.

FWIW, the current formulation in the docs looked fine to me.

Yeah, something on these lines would be a good idea. Note that, we are
already planning to slightly change this particular sentence in
another patch [1].

[1] - /messages/by-id/20200322021801.GB2563@telsasoft.com

Makes sense. I have two comments.

         ereport(ERROR,
                 (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
-                 errmsg("cannot specify both FULL and PARALLEL options")));
+                 errmsg("VACUUM FULL cannot be performed in parallel")));
Better to avoid a full sentence here [1]?  This should be a "cannot do
foo" errror. 

-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
+VACUUM (PARALLEL 0, FULL TRUE) tmp; -- can specify parallel disabled (even though that's implied by FULL)

To fully close the gap in the tests, I would also add a test for
(PARALLEL 1, FULL false) where FULL directly specified, even if that
sounds like a nit. That's fine to test even on a temporary table.

[1]: https://www.postgresql.org/docs/devel/error-style-guide.html -- Michael
--
Michael

On Tue, Apr 14, 2020 at 7:52 AM Michael Paquier <michael@paquier.xyz> wrote:

Makes sense. I have two comments.

ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
-                 errmsg("cannot specify both FULL and PARALLEL options")));
+                 errmsg("VACUUM FULL cannot be performed in parallel")));
Better to avoid a full sentence here [1]?  This should be a "cannot do
foo" errror.

I could see similar error messages in other places like below:
CONCURRENTLY cannot be used when the materialized view is not populated
CONCURRENTLY and WITH NO DATA options cannot be used together
COPY delimiter cannot be newline or carriage return

Also, I am not sure if it violates the style we have used in code. It
seems the error message is short, succinct and conveys the required
information.

-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
+VACUUM (PARALLEL 0, FULL TRUE) tmp; -- can specify parallel disabled (even though that's implied by FULL)

To fully close the gap in the tests, I would also add a test for
(PARALLEL 1, FULL false) where FULL directly specified, even if that
sounds like a nit. That's fine to test even on a temporary table.

Okay, I will do this once we agree on the error message stuff.

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

On Tue, Apr 14, 2020 at 8:55 AM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Tue, Apr 14, 2020 at 7:52 AM Michael Paquier <michael@paquier.xyz> wrote:

-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
+VACUUM (PARALLEL 0, FULL TRUE) tmp; -- can specify parallel disabled (even though that's implied by FULL)

To fully close the gap in the tests, I would also add a test for
(PARALLEL 1, FULL false) where FULL directly specified, even if that
sounds like a nit. That's fine to test even on a temporary table.

Okay, I will do this once we agree on the error message stuff.

I have changed one of the existing tests to test the option suggested
by you. Additionally, I have changed the docs as per suggestion from
Sawada-san. I haven't changed the error message. Let me know if you
have any more comments?

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

On Wed, Apr 15, 2020 at 08:54:17AM +0530, Amit Kapila wrote:

On Tue, Apr 14, 2020 at 8:55 AM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Tue, Apr 14, 2020 at 7:52 AM Michael Paquier <michael@paquier.xyz> wrote:

-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
+VACUUM (PARALLEL 0, FULL TRUE) tmp; -- can specify parallel disabled (even though that's implied by FULL)

To fully close the gap in the tests, I would also add a test for
(PARALLEL 1, FULL false) where FULL directly specified, even if that
sounds like a nit. That's fine to test even on a temporary table.

Okay, I will do this once we agree on the error message stuff.

I have changed one of the existing tests to test the option suggested
by you. Additionally, I have changed the docs as per suggestion from
Sawada-san. I haven't changed the error message. Let me know if you
have any more comments?

You did:
|...then the number of workers is determined based on the number of
|indexes that support parallel vacuum operation on the [-relation,-]{+relation+} and is further
|limited by <xref linkend="guc-max-parallel-workers-maintenance"/>.

I'd suggest to say instead:
|...then the number of workers is determined based on the number of
|indexes ON THE RELATION that support parallel vacuum operation, and is further
|limited by <xref linkend="guc-max-parallel-workers-maintenance"/>.

--
Justin

On Wed, Apr 15, 2020 at 9:03 AM Justin Pryzby <pryzby@telsasoft.com> wrote:

On Wed, Apr 15, 2020 at 08:54:17AM +0530, Amit Kapila wrote:

On Tue, Apr 14, 2020 at 8:55 AM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Tue, Apr 14, 2020 at 7:52 AM Michael Paquier <michael@paquier.xyz> wrote:

-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
+VACUUM (PARALLEL 0, FULL TRUE) tmp; -- can specify parallel disabled (even though that's implied by FULL)

To fully close the gap in the tests, I would also add a test for
(PARALLEL 1, FULL false) where FULL directly specified, even if that
sounds like a nit. That's fine to test even on a temporary table.

Okay, I will do this once we agree on the error message stuff.

I have changed one of the existing tests to test the option suggested
by you. Additionally, I have changed the docs as per suggestion from
Sawada-san. I haven't changed the error message. Let me know if you
have any more comments?

You did:
|...then the number of workers is determined based on the number of
|indexes that support parallel vacuum operation on the [-relation,-]{+relation+} and is further
|limited by <xref linkend="guc-max-parallel-workers-maintenance"/>.

I'd suggest to say instead:
|...then the number of workers is determined based on the number of
|indexes ON THE RELATION that support parallel vacuum operation, and is further
|limited by <xref linkend="guc-max-parallel-workers-maintenance"/>.

I have not changed this now but I find your suggestion better than
existing wording. I'll change this before committing the patch unless
there are more comments.

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

On Wed, Apr 15, 2020 at 9:12 AM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Wed, Apr 15, 2020 at 9:03 AM Justin Pryzby <pryzby@telsasoft.com> wrote:

On Wed, Apr 15, 2020 at 08:54:17AM +0530, Amit Kapila wrote:

On Tue, Apr 14, 2020 at 8:55 AM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Tue, Apr 14, 2020 at 7:52 AM Michael Paquier <michael@paquier.xyz> wrote:

-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
+VACUUM (PARALLEL 0, FULL TRUE) tmp; -- can specify parallel disabled (even though that's implied by FULL)

To fully close the gap in the tests, I would also add a test for
(PARALLEL 1, FULL false) where FULL directly specified, even if that
sounds like a nit. That's fine to test even on a temporary table.

Okay, I will do this once we agree on the error message stuff.

I have changed one of the existing tests to test the option suggested
by you. Additionally, I have changed the docs as per suggestion from
Sawada-san. I haven't changed the error message. Let me know if you
have any more comments?

You did:
|...then the number of workers is determined based on the number of
|indexes that support parallel vacuum operation on the [-relation,-]{+relation+} and is further
|limited by <xref linkend="guc-max-parallel-workers-maintenance"/>.

I'd suggest to say instead:
|...then the number of workers is determined based on the number of
|indexes ON THE RELATION that support parallel vacuum operation, and is further
|limited by <xref linkend="guc-max-parallel-workers-maintenance"/>.

I have not changed this now but I find your suggestion better than
existing wording. I'll change this before committing the patch unless
there are more comments.

Pushed.

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

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

On Wed, Apr 15, 2020 at 9:12 AM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Wed, Apr 15, 2020 at 9:03 AM Justin Pryzby <pryzby@telsasoft.com> wrote:

On Wed, Apr 15, 2020 at 08:54:17AM +0530, Amit Kapila wrote:

On Tue, Apr 14, 2020 at 8:55 AM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Tue, Apr 14, 2020 at 7:52 AM Michael Paquier <michael@paquier.xyz> wrote:

-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
+VACUUM (PARALLEL 0, FULL TRUE) tmp; -- can specify parallel disabled (even though that's implied by FULL)

To fully close the gap in the tests, I would also add a test for
(PARALLEL 1, FULL false) where FULL directly specified, even if that
sounds like a nit. That's fine to test even on a temporary table.

Okay, I will do this once we agree on the error message stuff.

I have changed one of the existing tests to test the option suggested
by you. Additionally, I have changed the docs as per suggestion from
Sawada-san. I haven't changed the error message. Let me know if you
have any more comments?

You did:
|...then the number of workers is determined based on the number of
|indexes that support parallel vacuum operation on the [-relation,-]{+relation+} and is further
|limited by <xref linkend="guc-max-parallel-workers-maintenance"/>.

I'd suggest to say instead:
|...then the number of workers is determined based on the number of
|indexes ON THE RELATION that support parallel vacuum operation, and is further
|limited by <xref linkend="guc-max-parallel-workers-maintenance"/>.

I have not changed this now but I find your suggestion better than
existing wording. I'll change this before committing the patch unless
there are more comments.

Pushed.

Thanks! I've updated the Open Items page.

Regards,

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