Re: [HACKERS] Block level parallel vacuum

On Wed, 15 Jan 2020 at 19:31, Mahendra Singh Thalor <mahi6run(at)gmail(dot)com> wrote:
>
> On Wed, 15 Jan 2020 at 19:04, Mahendra Singh Thalor <mahi6run(at)gmail(dot)com> wrote:
> >
> > On Wed, 15 Jan 2020 at 17:27, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> > >
> > > On Wed, Jan 15, 2020 at 10:05 AM Masahiko Sawada
> > > <masahiko(dot)sawada(at)2ndquadrant(dot)com> wrote:
> > > >
> > > > Thank you for updating the patch! I have a few small comments.
> > > >
> > >
> > > I have adapted all your changes, fixed the comment by Mahendra related
> > > to initializing parallel state only when there are at least two
> > > indexes. Additionally, I have changed a few comments (make the
> > > reference to parallel vacuum consistent, at some places we were
> > > referring it as 'parallel lazy vacuum' and at other places it was
> > > 'parallel index vacuum').
> > >
> > > > The
> > > > rest looks good to me.
> > > >
> > >
> > > Okay, I think the patch is in good shape. I am planning to read it a
> > > few more times (at least 2 times) and then probably will commit it
> > > early next week (Monday or Tuesday) unless there are any major
> > > comments. I have already committed the API patch (4d8a8d0c73).
> > >
> >
> > Hi,
> > Thanks Amit for fixing review comments.
> >
> > I reviewed v48 patch and below are some comments.
> >
> > 1.
> > + * based on the number of indexes. -1 indicates a parallel vacuum is
> >
> > I think, above should be like "-1 indicates that parallel vacuum is"
> >
> > 2.
> > +/* Variables for cost-based parallel vacuum */
> >
> > At the end of comment, there is 2 spaces. I think, it should be only 1 space.
> >
> > 3.
> > I think, we should add a test case for parallel option(when degree is not specified).
> > Ex:
> > postgres=# VACUUM (PARALLEL) tmp;
> > ERROR: parallel option requires a value between 0 and 1024
> > LINE 1: VACUUM (PARALLEL) tmp;
> > ^
> > postgres=#
> >
> > Because above error is added in this parallel patch, so we should have test case for this to increase code coverage.
> >
>
> Hi
> Below are some more review comments for v48 patch.
>
> 1.
> #include "storage/bufpage.h"
> #include "storage/lockdefs.h"
> +#include "storage/shm_toc.h"
> +#include "storage/dsm.h"
>
> Here, order of header file is not alphabetically. (storage/dsm.h
> should come before storage/lockdefs.h)
>
> 2.
> + /* No index supports parallel vacuum */
> + if (nindexes_parallel == 0)
> + return 0;
> +
> + /* The leader process takes one index */
> + nindexes_parallel--;
>
> Above code can be rearranged as:
>
> + /* The leader process takes one index */
> + nindexes_parallel--;
> +
> + /* No index supports parallel vacuum */
> + if (nindexes_parallel <= 0)
> + return 0;
>
> If we do like this, then in some cases, we can skip some calculations
> of parallel workers.
>
> --
> Thanks and Regards
> Mahendra Singh Thalor
> EnterpriseDB: http://www.enterprisedb.com

Hi,
I checked code coverage and time taken by vacuum.sql test with and
without v48 patch. Below are some findings (I ran "make check-world
-i" to get coverage.)

1.
With v45 patch, compute_parallel_delay is never called so function hit
is zero. I think, we can add some delay options into vacuum.sql test
to hit function.

2.
I checked time taken by vacuum.sql test. Execution time is almost same
with and without v45 patch.

Without v45 patch:
Run1) vacuum ... ok 701 ms
Run2) vacuum ... ok 549 ms
Run3) vacuum ... ok 559 ms
Run4) vacuum ... ok 480 ms

With v45 patch:
Run1) vacuum ... ok 842 ms
Run2) vacuum ... ok 808 ms
Run3) vacuum ... ok 774 ms
Run4) vacuum ... ok 792 ms

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