On Thu, Dec 16, 2021 at 1:56 PM Amit Kapila <amit.kapil...@gmail.com> wrote: > > 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.
Okay. > Also, can we move the common > code to be shared between vacuumparallel.c and vacuumlazy.c as a > separate patch? You mean vac_tid_reaped() and vac_cmp_itemptr() etc.? If so, do both vacuumparallel.c and vacuumlazy.c have the same functions? > > 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. Because it seems natural to me that the leader and worker use the same error callback. Okay, I'll remove that change in the next version 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? Because otherwise both vacuumlazy.c and vacuumparallel.c will have the same functions. > > 3. Why did you introduce > parallel_vacuum_bulkdel_all_indexes()/parallel_vacuum_cleanup_all_indexes()? > Is it because of your task "Unify the terminology"? This is because parallel bulk-deletion and cleanup require different numbers of inputs (num_table_tuples etc.) and the caller (vacuumlazy.c) cannot set them directly to ParallelVacuumState. > > 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. Yes, but I think it's an unnecessary break so we can change it together. Should it be done in a separate patch? Regards, -- Masahiko Sawada EDB: https://www.enterprisedb.com/
