Re: Vacuum o/p with (full 1, parallel 0) option throwing an error - Mailing list pgsql-hackers

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