Re: Add index scan progress to pg_stat_progress_vacuum

Hi,

Thank you for updating the patch!

On Tue, Nov 29, 2022 at 8:57 AM Imseih (AWS), Sami <simseih(at)amazon(dot)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