Re: parallel vacuum comments

On Wed, Dec 15, 2021 at 1:33 PM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
>
> I've attached an updated patch. The patch incorporated several changes
> from the last version:
>
> * Rename parallel_vacuum_begin() to parallel_vacuum_init()
> * Unify the terminology; use "index bulk-deletion" and "index cleanup"
> instead of "index vacuum" and "index cleanup".
>

I am not sure it is a good idea to do this as part of the main patch
as the intention of that is to just refactor parallel vacuum code. I
suggest doing this as a separate patch. Also, can we move the common
code to be shared between vacuumparallel.c and vacuumlazy.c as a
separate patch?

Few other comments and questions:
============================
1. /* Outsource everything to parallel variant */
- parallel_vacuum_process_all_indexes(vacrel, true);
+ LVSavedErrInfo saved_err_info;
+
+ /*
+ * Outsource everything to parallel variant. Since parallel vacuum will
+ * set the error context on an error we temporarily disable setting our
+ * error context.
+ */
+ update_vacuum_error_info(vacrel, &saved_err_info,
+ VACUUM_ERRCB_PHASE_UNKNOWN,
+ InvalidBlockNumber, InvalidOffsetNumber);
+
+ parallel_vacuum_bulkdel_all_indexes(vacrel->pvs, vacrel->old_live_tuples);
+
+ /* Revert to the previous phase information for error traceback */
+ restore_vacuum_error_info(vacrel, &saved_err_info);

Is this change because you want a separate error callback for parallel
vacuum? If so, I suggest we can discuss this as a separate patch from
the refactoring patch.

2. Is introducing bulkdel_one_index/cleanup_one_index related to new
error context, or "Unify the terminology" task? Is there any other
reason for the same?

3. Why did you introduce
parallel_vacuum_bulkdel_all_indexes()/parallel_vacuum_cleanup_all_indexes()?
Is it because of your task "Unify the terminology"?

4.
@@ -3086,7 +2592,6 @@ lazy_cleanup_one_index(Relation indrel,
IndexBulkDeleteResult *istat,
ivinfo.report_progress = false;
ivinfo.estimated_count = estimated_count;
ivinfo.message_level = elevel;
-
ivinfo.num_heap_tuples = reltuples;

This seems like an unrelated change.

--
With Regards,
Amit Kapila.