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
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c index f1b5f87e6a..d43051aea9 100644 --- a/src/backend/commands/indexcmds.c +++ b/src/backend/commands/indexcmds.c @@ -3409,6 +3409,7 @@ ReindexRelationConcurrently(Oid relationOid, int options) Oid oldIndexId = lfirst_oid(lc); Oid newIndexId = lfirst_oid(lc2); Oid heapId; + Oid indexam; /* Start new transaction for this index's concurrent build */ StartTransactionCommand(); @@ -3429,8 +3430,21 @@ ReindexRelationConcurrently(Oid relationOid, int options) */ indexRel = index_open(oldIndexId, ShareUpdateExclusiveLock); heapId = indexRel->rd_index->indrelid; + /* The access method of the old and new indexes match */ + indexam = indexRel->rd_rel->relam; index_close(indexRel, NoLock); + /* + * Update progress for the index to build, with the correct parent + * table involved. + */ + pgstat_progress_start_command(PROGRESS_COMMAND_CREATE_INDEX, heapId); + pgstat_progress_update_param(PROGRESS_CREATEIDX_COMMAND, + PROGRESS_CREATEIDX_COMMAND_REINDEX_CONCURRENTLY); + pgstat_progress_update_param(PROGRESS_CREATEIDX_INDEX_OID, newIndexId); + pgstat_progress_update_param(PROGRESS_CREATEIDX_ACCESS_METHOD_OID, + indexam); + /* Perform concurrent build of new index */ index_concurrently_build(heapId, newIndexId); @@ -3458,6 +3472,8 @@ ReindexRelationConcurrently(Oid relationOid, int options) Oid heapId; TransactionId limitXmin; Snapshot snapshot; + Relation newIndexRel; + Oid indexam; StartTransactionCommand(); @@ -3468,8 +3484,6 @@ ReindexRelationConcurrently(Oid relationOid, int options) */ CHECK_FOR_INTERRUPTS(); - heapId = IndexGetRelation(newIndexId, false); - /* * Take the "reference snapshot" that will be used by validate_index() * to filter candidate tuples. @@ -3477,6 +3491,26 @@ ReindexRelationConcurrently(Oid relationOid, int options) snapshot = RegisterSnapshot(GetTransactionSnapshot()); PushActiveSnapshot(snapshot); + /* + * Index relation has been closed by previous commit, so reopen it to + * get its information. + */ + newIndexRel = index_open(newIndexId, ShareUpdateExclusiveLock); + heapId = newIndexRel->rd_index->indrelid; + indexam = newIndexRel->rd_rel->relam; + index_close(newIndexRel, NoLock); + + /* + * Update progress for the index to build, with the correct parent + * table involved. + */ + pgstat_progress_start_command(PROGRESS_COMMAND_CREATE_INDEX, heapId); + pgstat_progress_update_param(PROGRESS_CREATEIDX_COMMAND, + PROGRESS_CREATEIDX_COMMAND_REINDEX_CONCURRENTLY); + pgstat_progress_update_param(PROGRESS_CREATEIDX_INDEX_OID, newIndexId); + pgstat_progress_update_param(PROGRESS_CREATEIDX_ACCESS_METHOD_OID, + indexam); + validate_index(heapId, newIndexId, snapshot); /* @@ -3611,7 +3645,7 @@ ReindexRelationConcurrently(Oid relationOid, int options) */ pgstat_progress_update_param(PROGRESS_CREATEIDX_PHASE, - PROGRESS_CREATEIDX_PHASE_WAIT_4); + PROGRESS_CREATEIDX_PHASE_WAIT_5); WaitForLockersMultiple(lockTags, AccessExclusiveLock, true); PushActiveSnapshot(GetTransactionSnapshot());
signature.asc
Description: PGP signature
