On Tue, Nov 16, 2021 at 11:23 AM Masahiko Sawada <sawada.mshk@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.