Re: Add index scan progress to pg_stat_progress_vacuum - Mailing list pgsql-hackers

On Fri, Jan 20, 2023 at 11:38 PM Imseih (AWS), Sami <simseih@amazon.com> wrote:
>
> >    Number of indexes that will be vacuumed or cleaned up. This counter only
> >    advances when the phase is vacuuming indexes or cleaning up indexes.
>
> I agree, this reads better.
>
>     ---
>     -        /* Report that we are now vacuuming indexes */
>     -        pgstat_progress_update_param(PROGRESS_VACUUM_PHASE,
>     -
>     PROGRESS_VACUUM_PHASE_VACUUM_INDEX);
>     +        /*
>     +         * Report that we are now vacuuming indexes
>     +         * and the number of indexes to vacuum.
>     +         */
>     +        progress_start_val[0] = PROGRESS_VACUUM_PHASE_VACUUM_INDEX;
>     +        progress_start_val[1] = vacrel->nindexes;
>     +        pgstat_progress_update_multi_param(2, progress_start_index,
>     progress_start_val);
>
> >    According to our code style guideline[1], we limit line lengths so
> >    that the code is readable in an 80-column window. Some comments
>  >   updated in this patch seem too short.
>
> I will correct this.
>
> >    I think it's better to define "void *arg".
>
> Agree
>
> >    ---
> >    +                                /*
> >    +                                 * A Leader process that receives this message
> >    +                                 * must be ready to update progress.
> >    +                                 */
> >    +                                Assert(pcxt->parallel_progress_callback);
> >    +                                Assert(pcxt->parallel_progress_callback_arg);
> >    +
> >    +                                /* Report progress */
> >    +
> >    pcxt->parallel_progress_callback(pcxt->parallel_progress_callback_arg);
>
> >    I think the parallel query infra should not require
> >    parallel_progress_callback_arg to always be set. I think it can be
> >    NULL.
>
> This assertion is inside the new 'P' message type handling.
> If a leader is consuming this message, they must have a
> progress callback set. Right now we only set the callback
> in the parallel vacuum case only, so not all leaders will be prepared
> to handle this case.
>
> Would you agree this is needed for safety?

I think it makes sense that we assume pcxt->parallel_progress_callback
is always not NULL but my point is that in the future one might want
to use this callback without the argument and we should allow it. If
parallel vacuum assumes pcxt->parallel_progress_callback_arg is not
NULL, I think we should add an assertion in
parallel_vacuum_update_progress(), but not in HandleParallelMessage().

>
>         case 'P':               /* Parallel progress reporting */
>             {
>                 /*
>                  * A Leader process that receives this message
>                  * must be ready to update progress.
>                  */
>                 Assert(pcxt->parallel_progress_callback);
>                 Assert(pcxt->parallel_progress_callback_arg);
>
>     ---
> >    +void
> >    +parallel_vacuum_update_progress(void *arg)
> >    +{
> >    +        ParallelVacuumState *pvs = (ParallelVacuumState *)arg;
> >    +
> >    +        Assert(!IsParallelWorker());
> >    +
> >    +        if (pvs)
> >    +                pgstat_progress_update_param(PROGRESS_VACUUM_INDEX_COMPLETED,
> >    +
> >       pg_atomic_add_fetch_u32(&(pvs->shared->nindexes_completed), 1));
> >    +}
>
> >    Since parallel vacuum always sets the arg, I think we don't need to check it.
>
> The arg is only set during parallel vacuum. During non-parallel vacuum,
> It's NULL. This check can be removed, but I realize now that we do need
> an Assert(pvs). Do you agree?

Agreed to add the assertion in this function.

Regards,

-- 
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com