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

Hi,

Thank you for updating the patch!

On Tue, Nov 29, 2022 at 8:57 AM Imseih (AWS), Sami <simseih@amazon.com> wrote:
>
> >   I think that indexes_total should be 0 also when INDEX_CLEANUP is off.
>
> Patch updated for handling of INDEX_CLEANUP = off, with an update to
> the documentation as well.
>
> >    I think we don't need to reset it at the end of index vacuuming. There
> >    is a small window before switching to the next phase. If we reset this
> >    value while showing "index vacuuming" phase, the user might get
> >    confused. Instead, we can reset it at the beginning of the index
> >    vacuuming.
>
> No, I think the way it's currently done is correct. We want to reset the number
> of indexes completed before we increase the num_index_scans ( index vacuum cycle ).
> This ensures that when we enter a new index cycle, the number of indexes completed
> are already reset. The 2 fields that matter here is how many indexes are vacuumed in the
> currently cycle and this is called out in the documentation as such.
>

Agreed.

Here are comments on v16 patch.

--- a/contrib/bloom/blvacuum.c
+++ b/contrib/bloom/blvacuum.c
@@ -15,12 +15,14 @@
 #include "access/genam.h"
 #include "bloom.h"
 #include "catalog/storage.h"
+#include "commands/progress.h"
 #include "commands/vacuum.h"
 #include "miscadmin.h"
 #include "postmaster/autovacuum.h"
 #include "storage/bufmgr.h"
 #include "storage/indexfsm.h"
 #include "storage/lmgr.h"
+#include "utils/backend_progress.h"

I think we don't need to include them here. Probably the same is true
for other index AMs.

---
                vacuum_delay_point();
+               if (info->report_parallel_progress && (blkno %
REPORT_PARALLEL_VACUUM_EVERY_PAGES) == 0)
+                       parallel_vacuum_update_progress();
+

                buffer = ReadBufferExtended(index, MAIN_FORKNUM, blkno,

There is an extra new line.

---
+     <row>
+      <entry><literal>ParallelVacuumFinish</literal></entry>
+      <entry>Waiting for parallel vacuum workers to finish computing.</entry>
+     </row>

How about "Waiting for parallel vacuum workers to finish index vacuum"?

---
vacrel->rel = rel;
vac_open_indexes(vacrel->rel, RowExclusiveLock, &vacrel->nindexes,
                                 &vacrel->indrels);
+
if (instrument && vacrel->nindexes > 0)
{
        /* Copy index names used by instrumentation (not error reporting) */


This added line is not necessary.

---
         /* Counter for vacuuming and cleanup */
         pg_atomic_uint32 idx;
+
+        /*
+         * Counter for vacuuming and cleanup progress reporting.
+         * This value is used to report index vacuum/cleanup progress
+         * in parallel_vacuum_progress_report. We keep this
+         * counter to avoid having to loop through
+         * ParallelVacuumState->indstats to determine the number
+         * of indexes completed.
+         */
+        pg_atomic_uint32 idx_completed_progress;

I think the name of idx_completed_progress is very confusing. Since
the idx of PVShared refers to the current index in the pvs->indstats[]
whereas idx_completed_progress is the number of vacuumed indexes. How
about "nindexes_completed"?

---
+                /*
+                 * Set the shared parallel vacuum progress. This will be used
+                 * to periodically update progress.h with completed indexes
+                 * in a parallel vacuum. See comments in
parallel_vacuum_update_progress
+                 * for more details.
+                 */
+                ParallelVacuumProgress =
&(pvs->shared->idx_completed_progress);
+

Since we pass pvs to parallel_wait_for_workers_to_finish(), we don't
need to have ParallelVacuumProgress. I see
parallel_vacuum_update_progress() uses this value but I think it's
better to pass ParallelVacuumState to via IndexVacuumInfo.

---
+                /*
+                 * To wait for parallel workers to finish,
+                 * first call parallel_wait_for_workers_to_finish
+                 * which is responsible for reporting the
+                 * number of indexes completed.
+                 *
+                 * Afterwards, WaitForParallelWorkersToFinish is called
+                 * to do the real work of waiting for parallel workers
+                 * to finish.
+                 *
+                 * Note: Both routines will acquire a WaitLatch in their
+                 * respective loops.
+                 */

How about something like:

Wait for all indexes to be vacuumed while updating the parallel vacuum
index progress. And then wait for all workers to finish.

---
         RelationGetRelationName(indrel));
         }

+        if (ivinfo.report_parallel_progress)
+                parallel_vacuum_update_progress();
+

I think it's better to update the progress info after updating
pvs->shared->idx_completed_progress.

---
+/*
+ * Check if we are done vacuuming indexes and report
+ * progress.

How about "Waiting for all indexes to be vacuumed while updating the
parallel index vacuum progress"?

+ *
+ * We nap using with a WaitLatch to avoid a busy loop.
+ *
+ * Note: This function should be used by the leader process only,
+ * and it's up to the caller to ensure this.
+ */

I think these comments are not necessary.

+void
+parallel_wait_for_workers_to_finish(ParallelVacuumState *pvs)

How about "parallel_vacuum_wait_to_finish"?

---
+/*
+ * Read the shared ParallelVacuumProgress and update progress.h
+ * with indexes vacuumed so far. This function is called periodically
+ * by index AMs as well as parallel_vacuum_process_one_index.
+ *
+ * To avoid unnecessarily updating progress, we check the progress
+ * values from the backend entry and only update if the value
+ * of completed indexes increases.
+ *
+ * Note: This function should be used by the leader process only,
+ * and it's up to the caller to ensure this.
+ */
+void
+parallel_vacuum_update_progress(void)
+{
+        volatile PgBackendStatus *beentry = MyBEEntry;
+
+        Assert(!IsParallelWorker);
+
+        if (beentry && ParallelVacuumProgress)
+        {
+                int parallel_vacuum_current_value =
beentry->st_progress_param[PROGRESS_VACUUM_INDEX_COMPLETED];
+                int parallel_vacuum_new_value =
pg_atomic_read_u32(ParallelVacuumProgress);
+
+                if (parallel_vacuum_new_value > parallel_vacuum_current_value)
+
pgstat_progress_update_param(PROGRESS_VACUUM_INDEX_COMPLETED,
parallel_vacuum_new_value);
+        }
+}

parallel_vacuum_update_progress() is typically called every 1GB so I
think we don't need to worry about unnecessary update. Also, I think
this code doesn't work when pgstat_track_activities is false. Instead,
I think that in parallel_wait_for_workers_to_finish(), we can check
the value of pvs->nindexes_completed and update the progress if there
is an update or it's first time.

---
+                (void) WaitLatch(MyLatch, WL_TIMEOUT | WL_LATCH_SET |
WL_EXIT_ON_PM_DEATH, PARALLEL_VACUUM_PROGRESS_TIMEOUT,
+
WAIT_EVENT_PARALLEL_VACUUM_FINISH);
+                ResetLatch(MyLatch);

I think we don't necessarily need to use
PARALLEL_VACUUM_PROGRESS_TIMEOUT here. Probably we can use 1000L
instead. If we want to use PARALLEL_VACUUM_PROGRESS_TIMEOUT, we need
comments for that:

+#define PARALLEL_VACUUM_PROGRESS_TIMEOUT       1000

---
-        WAIT_EVENT_XACT_GROUP_UPDATE
+        WAIT_EVENT_XACT_GROUP_UPDATE,
+        WAIT_EVENT_PARALLEL_VACUUM_FINISH
 } WaitEventIPC;

 Enums of WaitEventIPC should be defined in alphabetical order.

---
cfbot fails.

Regards,

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