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