Re: [HACKERS] Block level parallel vacuum

On Thu, Jan 16, 2020 at 1:02 AM Mahendra Singh Thalor
<mahi6run@gmail.com> wrote:
>
> On Wed, 15 Jan 2020 at 19:31, Mahendra Singh Thalor <mahi6run@gmail.com> wrote:
> >
> > On Wed, 15 Jan 2020 at 19:04, Mahendra Singh Thalor <mahi6run@gmail.com> wrote:
> > >
> > >
> > > 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"
> > >

I am not an expert in this matter, but I am not sure if your
suggestion is correct.  I thought an article is required here, but I
could be wrong.  Can you please clarify?

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

I thought about it but was not sure to add a test for it.  We might
not want to add a test for each and every case as that will increase
the number and time of tests without a significant advantage.  Now
that you have pointed this, I can add a test for it unless someone
else thinks otherwise.

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

But how can we meaningfully test the functionality of the delay?  It
would be tricky to come up with a portable test that can always
produce consistent results.

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

I see some variance in results, have you run with autovacuum as off.
I was expecting that this might speed up some cases where parallel
vacuum is used by default.

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