Re: Report index currently being vacuumed in pg_stat_progress_vacuum

Hi,

> I think it's fine for a progress view to have an entry per backend,
> including parallel workers, so more than one row for a single command.

My worry is that if we do not separate high level command stats from
per-worker (or leader) stats, we will end up with a confusing view and
tie our hands in the future.

For example, in v6 we have heap_blks_total and heap_blks_scanned, which
represent aggregate progress for how much of the table has been scanned,
while index_blks_total and index_blks_done represent how much one worker
has processed for a specific index. This is probably OK today only
because we do not support parallel heap scan.

But if we ever support parallel heap scan during VACUUM, what would
heap_blks_total and heap_blks_scanned represent at that point? Would
they remain command-level aggregate progress, or become per-worker
progress?

My point is that the existing progress views have historically exposed
command-level aggregate stats. Trying to also use them for per-worker
stats in the same view does not seem right to me.

> If we ever implement parallel index rebuilding for REINDEX TABLE,
> where different workers rebuild different indexes, having one entry
> per worker in pg_stat_progress_create_index seems the straightforward
> thing to do.

For parallel REINDEX TABLE, a command-level progress model with
indexes_total and
indexes_done would seem more natural, similar to VACUUM.

One separate question is whether each individual index rebuild would
itself run in parallel.
Today, an individual REINDEX INDEX may already use parallel build, but
that is leader-driven
parallelism for one index. Parallel workers do not in turn spawn more
parallel workers. So
if REINDEX TABLE were implemented by assigning different indexes to
different workers, It seems
likely each worker’s assigned index rebuild to be serial unless we
introduced a more complex
nested-parallel design.

> A dedicated view for workers, say
> pg_stat_progress_create_index_worker, would end up with mostly the
> same columns as pg_stat_progress_create_index.

That may be the case, but at least the meaning of each view will be well
understood and clear. Not mixing per-command vs per-worker stats.

> I can also see Sami's point that the patch makes many of the existing
> columns of pg_stat_progress_vacuum no-op. But I'm not sure a dedicated
> worker view really avoids that. If a user wants the progress of one
> vacuum command they would join the two views, get one row per
> participant anyway, and the leader's heap columns would be repeated on
> every row.

I don't really think most users will actually perform this join. They would
start with the command-level view and if they need to understand more
of what the worker is doing they can query a separate worker specific view.

So my hang up is the semantic differences each row will carry and
how hard it may be for a (monitoring) user to reason about this. It is
also clear that others are not as worried about this as me, so I will
concede, because getting this information is important.

As far as v6: The code looks overall good to me, but I have some
comments.

1/

I do think it will be better to do one more split of v6-0001 to separate
current_index_relid and index_blks_*; they are 2 distinct features. Also
you can fold the 0002 into the index_blks_* commit.

Also, there are some comment updates still needed:

2/

+ <structfield>index_blks_done</structfield>. The remaining columns track
+ command-level heap progress that only the leader maintains; they read as
+ zero on worker rows.

We should mention mode and started_by as NULL, such as:

".... read as zero on worker rows, except that
<structfield>mode</structfield> and
<structfield>started_by</structfield> appear as <literal>NULL</literal>."

3/

+ working. A worker row with a <literal>NULL</literal>
+ <structfield>phase</structfield> is a worker that has been launched but has
+ not started on an index yet, which also happens when every index was
+ claimed by another participant before this worker got to it. Because a
+ table can be vacuumed by only one

I don't think phase will ever be NULL here since phase will be set to
"initializing" at minimum.

4/

/*
* Perform work within a launched parallel process.
*
* Since parallel vacuum workers perform only index vacuum or index cleanup,
* we don't need to report progress information.
*/
void
parallel_vacuum_main(dsm_segment *seg, shm_toc *toc)
{

This comment is now out-of-date and should be updated.

--
Sami Imseih
Amazon Web Services (AWS)