On Mon, Nov 25, 2019 at 5:36 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> 2.
> lazy_parallel_vacuum_or_cleanup_indexes()
> {
> ..
> ..
> }
>
> Here, it seems that we can increment/decrement the
> VacuumActiveNWorkers even when there is no work performed by the
> leader backend. How about moving increment/decrement inside function
> vacuum_or_cleanup_indexes_worker? In that case, we need to do it in
> this function when we are actually doing an index vacuum or cleanup.
> After doing that the other usage of increment/decrement of
> VacuumActiveNWorkers in other function heap_parallel_vacuum_main can
> be removed.
>
One of my colleague Mahendra who was testing this patch found that
stats for index reported by view pg_statio_all_tables are wrong for
parallel vacuum. I debugged the issue and found that there were two
problems in the stats related code.
1. The function get_indstats seem to be computing the wrong value of
stats for the last index.
2. The function lazy_parallel_vacuum_or_cleanup_indexes() was not
pointing to the computed stats when the parallel index scan is
skipped.
Find the above two fixes in the attached patch. This is on top of the
patches I sent yesterday [1].
Some more comments on v33-0002-Add-parallel-option-to-VACUUM-command
-------------------------------------------------------------------------------------------------------------
1. The code in function lazy_parallel_vacuum_or_cleanup_indexes()
that processes the indexes that have skipped parallel processing can
be moved to a separate function. Further, the newly added code by the
attached patch can also be moved to a separate function as the same
code is used in function vacuum_or_cleanup_indexes_worker().
2.
+void
+heap_parallel_vacuum_main(dsm_segment *seg, shm_toc *toc)
{
..
+ stats = (IndexBulkDeleteResult **)
+ palloc0(nindexes * sizeof(IndexBulkDeleteResult *));
..
}
It would be neat if we free this memory once it is used.
3.
+ /*
+ * Compute the number of indexes that can participate to parallel index
+ * vacuuming.
+ */
/to/in
4. The function lazy_parallel_vacuum_or_cleanup_indexes() launches
workers without checking whether it needs to do the same or not. For
ex. in cleanup phase, it is possible that we don't need to launch any
worker, so it will be waste. It might be that you are already
planning to handle it based on the previous comments/discussion in
which case you can ignore this.
[1] - https://www.postgresql.org/message-id/CAA4eK1LQ%2BYGjmSS-XqhuAa6eb%3DXykpx1LiT7UXJHmEKP%3D0QtsA%40mail.gmail.com
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
