Re: Add index scan progress to pg_stat_progress_vacuum

On Fri, Apr 07, 2023 at 07:27:17PM +0000, Imseih (AWS), Sami wrote:
> If called by a worker, it will send a 'P' message to the front end
> passing both the progress index, i.e. PROGRESS_VACUUM_INDEXES_PROCESSED
> And the value to increment by, i.e. 1 for index vacuum progress.
>
> With that, the additional shared memory counters in ParallelVacuumState
> are not needed, and the poke of the worker to the leader goes directly
> through a generic backend_progress API.

Thanks for the new version.  This has unfortunately not been able to
make the cut for v16, but let's see it done at the beginning of the
v17 cycle.

+void
+pgstat_progress_parallel_incr_param(int index, int64 incr)
+{
+   /*
+    * Parallel workers notify a leader through a 'P'
+    * protocol message to update progress, passing the
+    * progress index and increment value. Leaders can
+    * just call pgstat_progress_incr_param directly.
+    */
+   if (IsParallelWorker())
+   {
+       static StringInfoData progress_message;
+
+       initStringInfo(&progress_message);
+
+       pq_beginmessage(&progress_message, 'P');
+       pq_sendint32(&progress_message, index);
+       pq_sendint64(&progress_message, incr);
+       pq_endmessage(&progress_message);
+   }
+   else
+       pgstat_progress_incr_param(index, incr);
+}

I see.  You need to handle both the leader and worker case because
parallel_vacuum_process_one_index() can be called by either of them.

+       case 'P':               /* Parallel progress reporting */

Perhaps this comment should say that this is only about incremental
progress reporting, for the moment.

+    * Increase and report the number of index scans. Also, we reset the progress
+    * counters.

The counters reset are the two index counts, perhaps this comment
should mention this fact.

+               /* update progress */
+               int index = pq_getmsgint(msg, 4);
+               int incr = pq_getmsgint(msg, 1);
[...]
+       pq_beginmessage(&progress_message, 'P');
+       pq_sendint32(&progress_message, index);
+       pq_sendint64(&progress_message, incr);
+       pq_endmessage(&progress_message);

It seems to me that the receiver side is missing one pq_getmsgend()?
incr is defined and sent as an int64 on the sender side, hence the
receiver should use pq_getmsgint64(), no?  pq_getmsgint(msg, 1) means
to receive only one byte, see pqformat.c.  And the order is reversed?

There may be a case in the future about making 'P' more complicated
with more arguments, but what you have here should be sufficient for
your use-case.  Were there plans to increment more data for some
different and/or new progress indexes in the VACUUM path, by the way?
Most of that looked a bit tricky to me as this was AM-dependent, but I
may have missed something.
--
Michael