[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

On Fri, 25 Sep 2020 at 08:44, Michael Paquier <michael@paquier.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

On Fri, Sep 25, 2020 at 11:32:11AM +0200, Matthias van de Meent wrote:

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.

I have been considering that, and I can see your point. Not setting
the phase has the disadvantage to set it to "initializing" when
starting the command tracking, even for a very short time, and that
could be confusing as we state in the docs that this refers to the
point where the indexes are created. So, instead, I have actually set
the phase to "build" or "validate". As we are updating three times
the same four fields (phase, table OID, index OID and index AM) for
the concurrent creation, index build and validation, I have switched
the implementation to use a multi-param update instead everywhere, and
applied it down to 12. Thanks for the report, Matthias.
--
Michael