Обсуждение: [patch] Concurrent table reindex per-index progress reporting
Hi,
Matthias van de Meent
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?
Matthias van de Meent
Вложения
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