On Tue, Nov 16, 2021 at 11:23 AM Masahiko Sawada <sawada.m...@gmail.com> wrote: > > I've incorporated these comments and attached an updated patch. >
Review comments: ================ 1. index_can_participate_parallel_vacuum() { .. + /* + * Not safe, if the index supports parallel cleanup conditionally, + * but we have already processed the index (for bulkdelete). See the + * comments for option VACUUM_OPTION_PARALLEL_COND_CLEANUP to know + * when indexes support parallel cleanup conditionally. + */ + if (num_index_scans > 0 && + ((vacoptions & VACUUM_OPTION_PARALLEL_COND_CLEANUP) != 0)) .. } IIRC, we do this to avoid the need to invoke worker when parallel cleanup doesn't need to scan the index which means the work required to be performed by a worker would be minimal. If so, maybe we can write that in comments here or with VACUUM_OPTION_PARALLEL_COND_CLEANUP. If the above understanding is correct then is it correct to check num_index_scans here? AFAICS, this value is incremented in parallel_vacuum_all_indexes irrespective of whether it is invoked for bulk delete or clean up. OTOH, previously, this was done based on first_time variable which was in turn set based on vacrel->num_index_scans and that is incremented in lazy_vacuum_all_indexes(both in serial and parallel case). 2. The structure ParallelVacuumState contains both PVIndVacStatus and PVIndStats. Considering PVIndVacStatus is already present in PVIndStats, does ParallelVacuumState need to have both? 3. Why ParallelVacuumCtl is declared in vacuum.h? It appears to be only used in one function begin_parallel_vacuum, can't we just declare in vacuumparallel.c? As it is only required for one function and it is not that the number of individual parameters will be too huge, can't we do without having that structure. -- With Regards, Amit Kapila.