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

>    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?

        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?

--

Regards,

Sami Imseih
Amazon Web Services (AWS)