Hi, On 2023-01-07 01:59:40 +0000, Imseih (AWS), Sami wrote: > --- a/src/backend/access/nbtree/nbtree.c > +++ b/src/backend/access/nbtree/nbtree.c > @@ -998,6 +998,8 @@ btvacuumscan(IndexVacuumInfo *info, IndexBulkDeleteResult > *stats, > if (info->report_progress) > > pgstat_progress_update_param(PROGRESS_SCAN_BLOCKS_DONE, > > scanblkno); > + if (info->report_parallel_progress && (scanblkno % > REPORT_PARALLEL_VACUUM_EVERY_PAGES) == 0) > + > parallel_vacuum_update_progress(info->parallel_vacuum_state); > } > }
I still think it's wrong to need multiple pgstat_progress_*() calls within one scan. If we really need it, it should be abstracted into a helper function that wrapps all the vacuum progress stuff needed for an index. > @@ -688,7 +703,13 @@ parallel_vacuum_process_all_indexes(ParallelVacuumState > *pvs, int num_index_scan > */ > if (nworkers > 0) > { > - /* Wait for all vacuum workers to finish */ > + /* > + * Wait for all indexes to be vacuumed while > + * updating the parallel vacuum index progress, > + * and then wait for all workers to finish. > + */ > + parallel_vacuum_wait_to_finish(pvs); > + > WaitForParallelWorkersToFinish(pvs->pcxt); > > for (int i = 0; i < pvs->pcxt->nworkers_launched; i++) I don't think it's a good idea to have two difference routines that wait for workers to exit. And certainly not when one of them basically just polls in a regular interval as parallel_vacuum_wait_to_finish(). I think the problem here is that you're basically trying to work around the lack of an asynchronous state update mechanism between leader and workers. The workaround is to add a lot of different places that poll whether there has been any progress. And you're not doing that by integrating with the existing machinery for interrupt processing (i.e. CHECK_FOR_INTERRUPTS()), but by developing a new mechanism. I think your best bet would be to integrate with HandleParallelMessages(). Greetings, Andres Freund