Re: [HACKERS] Block level parallel vacuum

On Tue, Jan 28, 2020 at 2:13 AM Mahendra Singh Thalor
<mahi6run@gmail.com> wrote:
>
> On Sat, 25 Jan 2020 at 12:11, Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Fri, Jan 24, 2020 at 4:58 PM Mahendra Singh Thalor
> > <mahi6run@gmail.com> wrote:
> > >
> > > On Thu, 23 Jan 2020 at 15:32, Mahendra Singh Thalor <mahi6run@gmail.com> wrote:
> > > >
> > > > On Wed, 22 Jan 2020 at 12:48, Masahiko Sawada
> > > > <masahiko.sawada@2ndquadrant.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
uselike "degree for parallel vacuum"
 
>

I am not sure if 'degree' makes it very clear.  How about "use this
many background workers for vacuum, if available"?

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

Which message are you advocating here "cannot use the "parallel"
option when performing full" or "cannot specify both FULL and PARALLEL
options"?  The message used in this patch is mainly because of
consistency with nearby messages in the vacuumdb utility. If you are
advocating to change "cannot specify both FULL and PARALLEL options"
to match what we are using in this patch, then it is better to do that
separately and maybe ask for more opinions.  I think I understand your
desire to use the same message at both places, but it seems to me the
messages used in both the places are to maintain consistency with the
nearby code or the message used for a similar purpose.

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