Re: Add index scan progress to pg_stat_progress_vacuum

Hi,

On 2022-11-04 13:27:34 +0000, Imseih (AWS), Sami wrote:
> diff --git a/src/backend/access/gin/ginvacuum.c b/src/backend/access/gin/ginvacuum.c
> index b4fa5f6bf8..3d5e4600dc 100644
> --- a/src/backend/access/gin/ginvacuum.c
> +++ b/src/backend/access/gin/ginvacuum.c
> @@ -633,6 +633,9 @@ ginbulkdelete(IndexVacuumInfo *info, IndexBulkDeleteResult *stats,
> UnlockReleaseBuffer(buffer);
> buffer = ReadBufferExtended(index, MAIN_FORKNUM, blkno,
> RBM_NORMAL, info->strategy);
> +
> + if (info->report_parallel_progress)
> + info->parallel_progress_callback(info->parallel_progress_arg);
> }
>
> /* right now we found leftmost page in entry's BTree */

I don't think any of these progress callbacks should be done while pinning a
buffer and ...

> @@ -677,6 +680,9 @@ ginbulkdelete(IndexVacuumInfo *info, IndexBulkDeleteResult *stats,
> buffer = ReadBufferExtended(index, MAIN_FORKNUM, blkno,
> RBM_NORMAL, info->strategy);
> LockBuffer(buffer, GIN_EXCLUSIVE);
> +
> + if (info->report_parallel_progress)
> + info->parallel_progress_callback(info->parallel_progress_arg);
> }
>
> MemoryContextDelete(gvs.tmpCxt);

... definitely not while holding a buffer lock.

I also don't understand why info->parallel_progress_callback exists? It's only
set to parallel_vacuum_progress_report(). Why make this stuff more expensive
than it has to already be?

> +parallel_vacuum_progress_report(void *arg)
> +{
> + ParallelVacuumState *pvs = (ParallelVacuumState *) arg;
> +
> + if (IsParallelWorker())
> + return;
> +
> + pgstat_progress_update_param(PROGRESS_VACUUM_INDEX_COMPLETED,
> + pg_atomic_read_u32(&(pvs->shared->idx_completed_progress)));
> +}

So each of the places that call this need to make an additional external
function call for each page, just to find that there's nothing to do or to
make yet another indirect function call. This should probably a static inline
function.

This is called, for every single page, just to read the number of indexes
completed by workers? A number that barely ever changes?

This seems all wrong.

Greetings,

Andres Freund