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