Horton

Thread

Commits

  1. Fix the usage of parallel and full options of vacuum command.

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

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

    tushar <tushar.ahuja@enterprisedb.com> — 2020-04-08T12:22:12Z

    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

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

    Robert Haas <robertmhaas@gmail.com> — 2020-04-08T12:29:07Z

    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

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

    Mahendra Singh Thalor <mahi6run@gmail.com> — 2020-04-08T14:24:49Z

    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

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

    Robert Haas <robertmhaas@gmail.com> — 2020-04-08T15:57:08Z

    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

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

    Justin Pryzby <pryzby@telsasoft.com> — 2020-04-08T16:41:54Z

    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.

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

    Mahendra Singh Thalor <mahi6run@gmail.com> — 2020-04-08T18:36:04Z

    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

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

    Justin Pryzby <pryzby@telsasoft.com> — 2020-04-08T18:38:54Z

    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

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

    Michael Paquier <michael@paquier.xyz> — 2020-04-09T01:36:54Z

    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

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

    Amit Kapila <amit.kapila16@gmail.com> — 2020-04-09T05:35:50Z

    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

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

    Amit Kapila <amit.kapila16@gmail.com> — 2020-04-09T05:52:17Z

    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

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

    Masahiko Sawada <masahiko.sawada@2ndquadrant.com> — 2020-04-09T06:23:48Z

    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

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

    Michael Paquier <michael@paquier.xyz> — 2020-04-09T06:44:26Z

    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

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

    Amit Kapila <amit.kapila16@gmail.com> — 2020-04-09T07:01:55Z

    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

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

    Amit Kapila <amit.kapila16@gmail.com> — 2020-04-09T07:03:57Z

    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

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

    Michael Paquier <michael@paquier.xyz> — 2020-04-09T07:20:13Z

    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

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

    Justin Pryzby <pryzby@telsasoft.com> — 2020-04-09T07:57:02Z

    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

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

    Masahiko Sawada <masahiko.sawada@2ndquadrant.com> — 2020-04-09T08:07:48Z

    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

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

    Justin Pryzby <pryzby@telsasoft.com> — 2020-04-09T08:33:19Z

    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

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

    Robert Haas <robertmhaas@gmail.com> — 2020-04-09T14:00:52Z

    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

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

    Amit Kapila <amit.kapila16@gmail.com> — 2020-04-10T05:04:02Z

    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

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

    Amit Kapila <amit.kapila16@gmail.com> — 2020-04-10T05:07:08Z

    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

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

    Masahiko Sawada <masahiko.sawada@2ndquadrant.com> — 2020-04-10T05:13:12Z

    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

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

    Justin Pryzby <pryzby@telsasoft.com> — 2020-04-10T13:35:32Z

    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

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

    Amit Kapila <amit.kapila16@gmail.com> — 2020-04-13T09:24:53Z

    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

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

    Masahiko Sawada <masahiko.sawada@2ndquadrant.com> — 2020-04-13T10:52:37Z

    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

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

    Amit Kapila <amit.kapila16@gmail.com> — 2020-04-13T12:25:43Z

    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

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

    Michael Paquier <michael@paquier.xyz> — 2020-04-14T02:22:42Z

    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

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

    Amit Kapila <amit.kapila16@gmail.com> — 2020-04-14T03:25:15Z

    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

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

    Amit Kapila <amit.kapila16@gmail.com> — 2020-04-15T03:24:17Z

    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

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

    Justin Pryzby <pryzby@telsasoft.com> — 2020-04-15T03:33:01Z

    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

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

    Amit Kapila <amit.kapila16@gmail.com> — 2020-04-15T03:42:37Z

    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

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

    Amit Kapila <amit.kapila16@gmail.com> — 2020-04-16T06:01:59Z

    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

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

    Masahiko Sawada <masahiko.sawada@2ndquadrant.com> — 2020-04-16T07:39:00Z

    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