Re: Add index scan progress to pg_stat_progress_vacuum

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