Re: [HACKERS] Block level parallel vacuum

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

--
Masahiko Sawada http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services