Re: [HACKERS] Block level parallel vacuum

On Wed, 27 Nov 2019 at 23:14, Masahiko Sawada <
masahiko(dot)sawada(at)2ndquadrant(dot)com> wrote:

> On Wed, 27 Nov 2019 at 13:26, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> >
> > On Wed, Nov 27, 2019 at 8:14 AM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
> wrote:
> > >
> > > On Wed, Nov 27, 2019 at 12:52 AM Masahiko Sawada
> > > <masahiko(dot)sawada(at)2ndquadrant(dot)com> wrote:
> > > >
> > > >
> > > > I've incorporated the comments I got so far including the above and
> > > > the memory alignment issue.
> > > >
> > >
> > > Thanks, I will look into the new version.
> > >
> >
> > Few comments:
> > -----------------------
> > 1.
> > +static void
> > +vacuum_or_cleanup_indexes_worker(Relation *Irel, int nindexes,
> > + IndexBulkDeleteResult **stats,
> > + LVShared *lvshared,
> > + LVDeadTuples *dead_tuples)
> > +{
> > + /* Increment the active worker count */
> > + pg_atomic_add_fetch_u32(VacuumActiveNWorkers, 1);
> >
> > The above code is wrong because it is possible that this function is
> > called even when there are no workers in which case
> > VacuumActiveNWorkers will be NULL.
> >
> > 2.
> > + /* Take over the shared balance value to heap scan */
> > + VacuumCostBalance = pg_atomic_read_u32(VacuumSharedCostBalance);
> >
> > We can carry over shared balance only if the same is active.
> >
> > 3.
> > + if (Irel[i]->rd_indam->amparallelvacuumoptions ==
> > + VACUUM_OPTION_NO_PARALLEL)
> > + {
> > +
> > /* Set NULL as this index does not support parallel vacuum */
> > + lvshared->bitmap[i >> 3] |= 0 << (i & 0x07);
> >
> > Can we avoid setting this for each index by initializing bitmap as all
> > NULL's as is done in the attached patch?
> >
> > 4.
> > + /*
> > + * Variables to control parallel index vacuuming. Index statistics
> > + * returned from ambulkdelete and amvacuumcleanup is nullable
> > variable
> > + * length. 'offset' is NULL bitmap. Note that a 0 indicates a null,
> > + * while 1 indicates non-null. The index statistics follows
> > at end of
> > + * struct.
> > + */
> >
> > This comment is not clear, so I have re-worded it. See, if the
> > changed comment makes sense.
> >
> > I have fixed all the above issues, made a couple of other cosmetic
> > changes and modified a few comments. See the changes in
> > v34-0002-delta-amit. I am attaching just the delta patch on top of
> > v34-0002-Add-parallel-option-to-VACUUM-command.
> >
>
> Thank you for reviewing this patch. All changes you made looks good to me.
>
> I thought I already have posted all v34 patches but didn't, sorry. So
> I've attached v35 patch set that incorporated your changes and it
> includes Dilip's patch for gist index (0001). These patches can be
> applied on top of the current HEAD and make check should pass.
>

Thanks for the re-based patches.

On the top of v35 patch, I can see one compilation warning.

> parallel.c: In function ‘LaunchParallelWorkers’:
> parallel.c:502:2: warning: ISO C90 forbids mixed declarations and code
> [-Wdeclaration-after-statement]
> int i;
> ^
>

Above warning is due to one extra semicolon added at the end of declaration
line in v35-0003 patch. Please fix this in next version.
+ int nworkers_to_launch = Min(nworkers, pcxt->nworkers);;

I will continue my testing on the top of v35 patch set and will post
results.

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