Re: parallel vacuum comments

On Thu, Dec 2, 2021 at 6:01 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> I've attached updated patches.
>

I have a few comments on v4-0001.
1.
In parallel_vacuum_process_all_indexes(), we can combine the two
checks for vacuum/cleanup at the beginning of the function and I think
it is better to keep the variable name as bulkdel or cleanup instead
of vacuum as that is more specific and clear.

2. The patch seems to be calling parallel_vacuum_should_skip_index
thrice even before starting parallel vacuum. It has a call to find the
number of blocks which has to be performed for each index. I
understand it might not be too costly to call this but it seems better
to remember this info like we are doing in the current code. We can
probably set parallel_workers_can_process in parallel_vacuum_begin and
then again update in parallel_vacuum_process_all_indexes. Won't doing
something like that be better?

3.  /*
  * Copy the index bulk-deletion result returned from ambulkdelete and
@@ -2940,19 +2935,20 @@ parallel_process_one_index(Relation indrel,
  * Since all vacuum workers write the bulk-deletion result at different
  * slots we can write them without locking.
  */
- if (shared_istat && !shared_istat->updated && istat_res != NULL)
+ if (!pindstats->istat_updated && istat_res != NULL)
  {
- memcpy(&shared_istat->istat, istat_res, sizeof(IndexBulkDeleteResult));
- shared_istat->updated = true;
+ memcpy(&(pindstats->istat), istat_res, sizeof(IndexBulkDeleteResult));
+ pindstats->istat_updated = true;

  /* Free the locally-allocated bulk-deletion result */
  pfree(istat_res);
-
- /* return the pointer to the result from shared memory */
- return &shared_istat->istat;
  }

I think here now we copy the results both for local and parallel
cleanup. Isn't it better to write something about that in comments as
it is not clear from current comments?

4.
+ /* Estimate size for shared information -- PARALLEL_VACUUM_KEY_SHARED */
+ est_shared_len = MAXALIGN(sizeof(LVShared));
  shm_toc_estimate_chunk(&pcxt->estimator, est_shared_len);

Do we need MAXALIGN here? I think previously we required it here
because immediately after this we were writing index stats but now
those are allocated separately. Normally, shm_toc_estimate_chunk()
aligns the size but sometimes we need to do it so as to avoid
unaligned accessing of shared mem. I am really not sure whether we
require it for dead_items, do you remember why it is there in the
first place.

5. The below-updated comment based on one of my previous suggestions
seems to be missing in this version.
+ /*
+ * Not safe, if the index supports parallel cleanup conditionally,
+ * but we have already processed the index (for bulkdelete).  We do
+ * this to avoid the need to invoke workers when parallel index
+ * cleanup doesn't need to scan the index.  See the comments for
+ * option VACUUM_OPTION_PARALLEL_COND_CLEANUP to know when indexes
+ * support parallel cleanup conditionally.
+ */

-- 
With Regards,
Amit Kapila.