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

On Fri, 25 Sep 2020 at 08:44, Michael Paquier <michael(at)paquier(dot)xyz> wrote:
>
> 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.

Thanks for checking the patch, I should've checked it more than I did.

>
> > + * 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().

Updating to WAIT_1 was to set it back to whatever the state was before
we would get into the loop and start_command was called. I quite
apparently didn't take enough care to set all fields that could be
set, though, so thanks for noticing.

> > + /*
> > + * 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.

Basically the same response: I didn't take enough care to correctly
reset the state, while being conservative in what I did put back.

>
> 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?

It looks great, thanks!

-Matthias