On Wed, Dec 15, 2021 at 1:33 PM Masahiko Sawada <sawada.m...@gmail.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.
