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

On Sat, Feb 18, 2023 at 11:46 AM Imseih (AWS), Sami <simseih@amazon.com> wrote:
>
> Thanks for the review!
>
> >    +     <row>
> >    +      <entry><literal>ParallelVacuumFinish</literal></entry>
> >    +      <entry>Waiting for parallel vacuum workers to finish index
> >    vacuum.</entry>
> >    +     </row>
>
> >    This change is out-of-date.
>
> That was an oversight. Thanks for catching.
>
> >    Total number of indexes that will be vacuumed or cleaned up. This
> >    number is reported as of the beginning of the vacuuming indexes phase
> >    or the cleaning up indexes phase.
>
> This is cleaner. I was being unnecessarily verbose in the original description.
>
> >    Number of indexes processed. This counter only advances when the phase
> >    is vacuuming indexes or cleaning up indexes.
>
> I agree.
>
> >    Also, index_processed sounds accurate to me. What do you think?
>
> At one point, II used index_processed, but decided to change it.
> "processed" makes sense also. I will use this.
>
> >    I think these settings are not necessary since the pcxt is palloc0'ed.
>
> Good point.
>
> >    Assert(pvs->pcxt->parallel_progress_callback_arg) looks wrong to me.
> >    If 'arg' is NULL, a SEGV happens.
>
> Correct, Assert(pvs) is all that is needed.
>
> >    I think it's better to update pvs->shared->nindexes_completed by both
> >    leader and worker processes who processed the index.
>
> No reason for that, since only the leader process can report process to
> backend_progress.
>
>
> >    I think it's better to make the function type consistent with the
> >    existing parallel_worker_main_type. How about
> >    parallel_progress_callback_type?
>
> Yes, that makes sense.
>
> > I've attached a patch that incorporates the above comments and has
> >    some suggestions of updating comments etc.
>
> I reviewed and incorporated these changes, with a slight change. See v24.
>
> -        * Increase and report the number of index. Also, we reset the progress
> -        * counters.
>
>
> +        * Increase and report the number of index scans. Also, we reset the progress
> +        * counters.
>
>
> Thanks

Thanks for updating the patch!

 #define PROGRESS_VACUUM_NUM_INDEX_VACUUMS              4
 #define PROGRESS_VACUUM_MAX_DEAD_TUPLES                        5
 #define PROGRESS_VACUUM_NUM_DEAD_TUPLES                        6
+#define PROGRESS_VACUUM_INDEX_TOTAL             7
+#define PROGRESS_VACUUM_INDEX_PROCESSED         8

-    s.param7 AS num_dead_tuples
+    s.param7 AS num_dead_tuples,
+    s.param8 AS indexes_total,
+    s.param9 AS indexes_processed

I think PROGRESS_VACUUM_INDEXES_TOTAL and
PROGRESS_VACUUM_INDEXES_PROCESSED are better for consistency. The rest
looks good to me.

Regards,

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