[patch] Concurrent table reindex per-index progress reporting
Hi,
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
Attachments:
0001-Report-the-active-index-for-reindex-table-concurrent.patchtext/x-patch; charset=US-ASCII; name=0001-Report-the-active-index-for-reindex-table-concurrent.patchDownload
From f41da096b1f36118917fe345e2a6fc89530a40c9 Mon Sep 17 00:00:00 2001
From: Matthias van de Meent <boekewurm@gmail.com>
Date: Thu, 24 Sep 2020 20:41:10 +0200
Subject: [PATCH] Report the active index for reindex table concurrently
The pgstat_progress reporting for the multi-index path of reindexing
failed to set the index and relation OIDs correctly for various stages,
which resulted in a pg_stat_progress_create_index view that did not
accurately represent the index that the progress was being reported on.
This commit tags the correct index and relation for each of the concurrent
index creation stages.
Signed-off-by: Matthias van de Meent <boekewurm@gmail.com>
---
src/backend/commands/indexcmds.c | 18 ++++++++++++++++++
1 file changed, 18 insertions(+)
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index f1b5f87e6a..b2f04012a4 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -3431,6 +3431,15 @@ ReindexRelationConcurrently(Oid relationOid, int options)
heapId = indexRel->rd_index->indrelid;
index_close(indexRel, NoLock);
+ /*
+ * 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);
+
/* Perform concurrent build of new index */
index_concurrently_build(heapId, newIndexId);
@@ -3477,6 +3486,15 @@ ReindexRelationConcurrently(Oid relationOid, int options)
snapshot = RegisterSnapshot(GetTransactionSnapshot());
PushActiveSnapshot(snapshot);
+ /*
+ * 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);
/*
--
2.20.1
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
Attachments:
reindex-progress-v2.patchtext/x-diff; charset=us-asciiDownload
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());
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