Thread: Vacuum o/p with (full 1, parallel 0) option throwing an error
Hi, 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. -- regards,tushar EnterpriseDB https://www.enterprisedb.com/ The Enterprise PostgreSQL Company
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
Attachment
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
Attachment
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
Attachment
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
Attachment
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
Attachment
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
Attachment
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
Attachment
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]. [1] - https://www.postgresql.org/message-id/20200322021801.GB2563%40telsasoft.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] - https://www.postgresql.org/message-id/20200322021801.GB2563%40telsasoft.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
Attachment
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
Attachment
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