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/