Re: [patch] Concurrent table reindex per-index progress reporting

On Thu, Sep 24, 2020 at 09:19:18PM +0200, Matthias van de Meent wrote:
> While working on a PG12-instance I noticed that the progress reporting of
> concurrent index creation for non-index relations fails to update the
> index/relation OIDs that it's currently working on in the
> pg_stat_progress_create_index view after creating the indexes. Please find
> attached a patch which properly sets these values at the appropriate places.
>
> Any thoughts?

I agree that this is a bug and that we had better report correctly the
heap and index IDs worked on, as these could also be part of a toast
relation from the parent table reindexed. However, your patch is
visibly incorrect in the two places you are updating.

> + * Configure progress reporting to report for this index.
> + * While we're at it, reset the phase as it is cleared by start_command.
> + */
> + pgstat_progress_start_command(PROGRESS_COMMAND_CREATE_INDEX, heapId);
> + pgstat_progress_update_param(PROGRESS_CREATEIDX_INDEX_OID, newIndexId);
> + pgstat_progress_update_param(PROGRESS_CREATEIDX_PHASE,
> + PROGRESS_CREATEIDX_PHASE_WAIT_1);

First, updating PROGRESS_CREATEIDX_PHASE here to WAIT_1 makes no
sense, because this is not a wait phase, and index_build() called
within index_concurrently_build() will set this state correctly a bit
after so IMO it is fine to leave that unset here, and the build phase
is by far the bulk of the operation that needs tracking. I think that
you are also missing to update PROGRESS_CREATEIDX_COMMAND to
PROGRESS_CREATEIDX_COMMAND_REINDEX_CONCURRENTLY, and
PROGRESS_CREATEIDX_ACCESS_METHOD_OID to the correct index AM OID,
similarly to reindex_index().

> + /*
> + * Configure progress reporting to report for this index.
> + * While we're at it, reset the phase as it is cleared by start_command.
> + */
> + pgstat_progress_start_command(PROGRESS_COMMAND_CREATE_INDEX, heapId);
> + pgstat_progress_update_param(PROGRESS_CREATEIDX_INDEX_OID, newIndexId);
> + pgstat_progress_update_param(PROGRESS_CREATEIDX_PHASE,
> + PROGRESS_CREATEIDX_PHASE_WAIT_2);
> +
> validate_index(heapId, newIndexId, snapshot);

Basically the same three comments here: PROGRESS_CREATEIDX_PHASE set
to WAIT_2 makes no real sense, and validate_index() would update the
phase as it should be. This should set PROGRESS_CREATEIDX_COMMAND to
PROGRESS_CREATEIDX_COMMAND_REINDEX_CONCURRENTLY, and
PROGRESS_CREATEIDX_ACCESS_METHOD_OID to the correct index AM OID.

While reviewing this code, I also noticed that the wait state set just
before dropping the indexes was wrong. The code was using WAIT_4, but
this has to be WAIT_5.

And this gives me the attached. This also means that we still set the
table and relation OID to the last index worked on for WAIT_2, WAIT_4
and WAIT_5, but we cannot set the command start to relationOid either
as given in input of ReindexRelationConcurrently() as this could be a
single index given for REINDEX INDEX CONCURRENTLY. Matthias, does
that look right to you?
--
Michael