On Sat, 25 Jan 2020 at 12:11, Amit Kapila <amit.kapil...@gmail.com> wrote: > > On Fri, Jan 24, 2020 at 4:58 PM Mahendra Singh Thalor > <mahi6...@gmail.com> wrote: > > > > On Thu, 23 Jan 2020 at 15:32, Mahendra Singh Thalor <mahi6...@gmail.com> wrote: > > > > > > On Wed, 22 Jan 2020 at 12:48, Masahiko Sawada > > > <masahiko.saw...@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. -- Thanks and Regards Mahendra Singh Thalor EnterpriseDB: http://www.enterprisedb.com
