Re: parallel vacuum comments

On Wed, Dec 8, 2021 at 1:37 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> On Wed, Dec 8, 2021 at 12:22 PM houzj.fnst@fujitsu.com
> <houzj.fnst@fujitsu.com> wrote:
> >
> > On Tuesday, December 7, 2021 1:42 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> > > I've attached an updated patch. I've removed 0003 patch that added
> > > regression tests as per discussion. Regarding the terminology like "bulkdel"
> > > and "cleanup" you pointed out, I've done that in 0002 patch while moving the
> > > code to vacuumparallel.c. In that file, we can consistently use the terms
> > > "bulkdel" and "cleanup" instead of "vacuum"
> > > and "cleanup".
> > Hi,
> >
> > Thanks for updating the patch.
> > I noticed few minor things.
>
> Thank you for the comments!
>
> >
> > 0001
> > 1)
> >
> >                  * Skip processing indexes that are unsafe for workers (these are
> > -                * processed in do_serial_processing_for_unsafe_indexes() by leader)
> > +                * processed in parallel_vacuum_process_unsafe_indexes() by leader)
> >
> > It might be clearer to mention that the index to be skipped are unsafe OR not
> > worthwhile.
>
> Agreed. Will add the comments.
>
> >
> > 2)
> > +       /* Set index vacuum status and mark as parallel safe or not */
> > +       for (int i = 0; i < pvc->nindexes; i++)
> > +       {
> >         ...
> > +               pindstats->parallel_workers_can_process =
> > +                       parallel_vacuum_index_is_parallel_safe(vacrel,
> > +                                                                                                  vacrel->indrels[i],
> > +                                                                                                  vacuum);
> >
> > For the comments above the loop, maybe better to mention we are marking whether
> > worker can process the index(not only safe/unsafe).
>
> Right. WIll fix.
>
> >
> > 0002
> > 3)
> >
> > +               /*
> > +                * Skip indexes that are unsuitable target for parallel index vacuum
> > +                */
> > +               if (parallel_vacuum_should_skip_index(indrel))
> > +                       continue;
> > +
> >
> > It seems we can use will_parallel_vacuum[] here instead of invoking the function
> > again.
>
> Oops, I missed updating it in 0002 patch. Will fix.

I've attached updated patches. Please review them.

Regards,

-- 
Masahiko Sawada
EDB:  https://www.enterprisedb.com/