Re: parallel vacuum comments

On Wed, Dec 8, 2021 at 12:22 PM houzj(dot)fnst(at)fujitsu(dot)com
<houzj(dot)fnst(at)fujitsu(dot)com> wrote:
>
> On Tuesday, December 7, 2021 1:42 PM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)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.

Regards,

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