Re: [HACKERS] Block level parallel vacuum

On Sat, 25 Jan 2020 at 12:11, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Fri, Jan 24, 2020 at 4:58 PM Mahendra Singh Thalor
> <mahi6run(at)gmail(dot)com> wrote:
> >
> > On Thu, 23 Jan 2020 at 15:32, Mahendra Singh Thalor <mahi6run(at)gmail(dot)com>
wrote:
> > >
> > > On Wed, 22 Jan 2020 at 12:48, Masahiko Sawada
> > > <masahiko(dot)sawada(at)2ndquadrant(dot)com> wrote:
> > > >
> > > > Attached the updated version patch.
> > >
> > > Thanks Sawada-san for the re-based patch.
> > >
> > > I reviewed and tested this patch. Patch looks good to me.
> >
> > As offline, suggested by Amit Kapila, I verified vacuumdb "-P" option
> > functionality with older versions(<13) and also I tested vacuumdb by
> > giving "-j" option with "-P". All are working as per expectation and I
> > didn't find any issue with these options.
> >
>
> I have made few modifications in the patch.
>
> 1. I think we should try to block the usage of 'full' and 'parallel'
> option in the utility rather than allowing the server to return an
> error.
> 2. It is better to handle 'P' option in getopt_long in the order of
> its declaration in long_options array.
> 3. Added an Assert for server version while handling of parallel option.
> 4. Added a few sentences in the documentation.
>
> What do you guys think of the attached?
>

I took one more review round. Below are some review comments:

1.
-P, --parallel=PARALLEL_DEGREE do parallel vacuum

I think, "do parallel vacuum" should be modified. Without specifying -P, we
are still doing parallel vacuum so we can use like "degree for parallel
vacuum"

2. Error message inconsistent for FULL and parallel option:
*Error for normal vacuum:*
ERROR: cannot specify both FULL and PARALLEL options

*Error for vacuumdb:*
error: cannot use the "parallel" option when performing full

I think, both the places, we should use 2nd error message as it is giving
more clarity.

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