It looks like we missed this in a6642b3ae. I think it's an odd behavior of pg_stat_progress_create_index to simultaneously show the global progress as well as the progress for the current partition ...
It seems like for partitioned reindex, reindex_index() should set the AM, which is used in the view: src/backend/catalog/system_views.sql- WHEN 2 THEN 'building index' || src/backend/catalog/system_views.sql: COALESCE((': ' || pg_indexam_progress_phasename(S.param9::oid, S.param11)), Maybe it needs a new flag, like: params->options & REINDEXOPT_REPORT_PROGRESS_AM I don't understand why e66bcfb4c added multiple calls to pgstat_progress_start_command(). -- Justin
>From b04d46958b16bc07ecd6c2f07220599ecb304b4d Mon Sep 17 00:00:00 2001 From: Justin Pryzby <pryz...@telsasoft.com> Date: Mon, 15 Feb 2021 14:12:32 -0600 Subject: [PATCH] WIP: progress reporting for CREATE INDEX on partitioned tables See also: a6642b3ae060976b42830b7dc8f29ec190ab05e4 03f9e5cba0ee1633af4abe734504df50af46fbd8 c880096dc1e14b62610aa34bc98db226fa134260 e66bcfb4c66ebc97020b1f7484b1529bd7993f23 --- src/backend/catalog/index.c | 6 +- src/backend/commands/indexcmds.c | 104 ++++++++++++++++++++++--------- 2 files changed, 77 insertions(+), 33 deletions(-) diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c index f5441ce24a..fef9c831ac 100644 --- a/src/backend/catalog/index.c +++ b/src/backend/catalog/index.c @@ -3733,9 +3733,9 @@ reindex_index(Oid indexId, bool skip_constraint_checks, char persistence, */ iRel = index_open(indexId, AccessExclusiveLock); - if (progress) - pgstat_progress_update_param(PROGRESS_CREATEIDX_ACCESS_METHOD_OID, - iRel->rd_rel->relam); + // Do this unconditionally? + pgstat_progress_update_param(PROGRESS_CREATEIDX_ACCESS_METHOD_OID, + iRel->rd_rel->relam); /* * Partitioned indexes should never get processed here, as they have no diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c index 127ba7835d..8fdfeffce5 100644 --- a/src/backend/commands/indexcmds.c +++ b/src/backend/commands/indexcmds.c @@ -98,9 +98,10 @@ static void reindex_error_callback(void *args); static void ReindexPartitions(Oid relid, ReindexParams *params, bool isTopLevel); static void ReindexMultipleInternal(List *relids, - ReindexParams *params); + ReindexParams *params, bool ispartitioned); static bool ReindexRelationConcurrently(Oid relationOid, - ReindexParams *params); + ReindexParams *params, + bool ispartitioned); static void update_relispartition(Oid relationId, bool newval); static inline void set_indexsafe_procflags(void); @@ -2600,7 +2601,7 @@ ReindexIndex(RangeVar *indexRelation, ReindexParams *params, bool isTopLevel) ReindexPartitions(indOid, params, isTopLevel); else if ((params->options & REINDEXOPT_CONCURRENTLY) != 0 && persistence != RELPERSISTENCE_TEMP) - ReindexRelationConcurrently(indOid, params); + ReindexRelationConcurrently(indOid, params, false); else { ReindexParams newparams = *params; @@ -2710,7 +2711,7 @@ ReindexTable(RangeVar *relation, ReindexParams *params, bool isTopLevel) else if ((params->options & REINDEXOPT_CONCURRENTLY) != 0 && get_rel_persistence(heapOid) != RELPERSISTENCE_TEMP) { - result = ReindexRelationConcurrently(heapOid, params); + result = ReindexRelationConcurrently(heapOid, params, false); if (!result) ereport(NOTICE, @@ -2942,7 +2943,7 @@ ReindexMultipleTables(const char *objectName, ReindexObjectType objectKind, * Process each relation listed in a separate transaction. Note that this * commits and then starts a new transaction immediately. */ - ReindexMultipleInternal(relids, params); + ReindexMultipleInternal(relids, params, false); MemoryContextDelete(private_context); } @@ -3046,11 +3047,38 @@ ReindexPartitions(Oid relid, ReindexParams *params, bool isTopLevel) MemoryContextSwitchTo(old_context); } + /* progress reporting for partitioned indexes */ + if (relkind == RELKIND_PARTITIONED_INDEX) + { + const int progress_index[3] = { + PROGRESS_CREATEIDX_COMMAND, + PROGRESS_CREATEIDX_INDEX_OID, + PROGRESS_CREATEIDX_PARTITIONS_TOTAL + }; + + int64 progress_val[3] = { + (params->options & REINDEXOPT_CONCURRENTLY) != 0 && + get_rel_persistence(relid) != RELPERSISTENCE_TEMP ? + PROGRESS_CREATEIDX_COMMAND_REINDEX_CONCURRENTLY : + PROGRESS_CREATEIDX_COMMAND_REINDEX, + relid, + list_length(partitions) + }; + + Oid tableid = IndexGetRelation(relid, false); + pgstat_progress_start_command(PROGRESS_COMMAND_CREATE_INDEX, tableid); + + pgstat_progress_update_multi_param(3, progress_index, progress_val); + } + /* * Process each partition listed in a separate transaction. Note that * this commits and then starts a new transaction immediately. */ - ReindexMultipleInternal(partitions, params); + ReindexMultipleInternal(partitions, params, true); + + if (relkind == RELKIND_PARTITIONED_INDEX) + pgstat_progress_end_command(); /* * Clean up working storage --- note we must do this after @@ -3066,11 +3094,13 @@ ReindexPartitions(Oid relid, ReindexParams *params, bool isTopLevel) * Reindex a list of relations, each one being processed in its own * transaction. This commits the existing transaction immediately, * and starts a new transaction when finished. + * If isparititioned is true, then also handle progress reporting. */ static void -ReindexMultipleInternal(List *relids, ReindexParams *params) +ReindexMultipleInternal(List *relids, ReindexParams *params, bool ispartitioned) { ListCell *l; + off_t childnum = 0; PopActiveSnapshot(); CommitTransactionCommand(); @@ -3128,15 +3158,16 @@ ReindexMultipleInternal(List *relids, ReindexParams *params) ReindexParams newparams = *params; newparams.options |= REINDEXOPT_MISSING_OK; - (void) ReindexRelationConcurrently(relid, &newparams); + (void) ReindexRelationConcurrently(relid, &newparams, ispartitioned); /* ReindexRelationConcurrently() does the verbose output */ } else if (relkind == RELKIND_INDEX) { ReindexParams newparams = *params; - newparams.options |= - REINDEXOPT_REPORT_PROGRESS | REINDEXOPT_MISSING_OK; + /* Do not overwrite progress of partitioned tables */ + newparams.options |= REINDEXOPT_MISSING_OK | + (ispartitioned ? 0 : REINDEXOPT_REPORT_PROGRESS); reindex_index(relid, false, relpersistence, &newparams); PopActiveSnapshot(); /* reindex_index() does the verbose output */ @@ -3162,6 +3193,10 @@ ReindexMultipleInternal(List *relids, ReindexParams *params) PopActiveSnapshot(); } + if (ispartitioned) + pgstat_progress_update_param(PROGRESS_CREATEIDX_PARTITIONS_DONE, + childnum++); + CommitTransactionCommand(); } @@ -3177,7 +3212,8 @@ ReindexMultipleInternal(List *relids, ReindexParams *params) * view. For tables and materialized views, all its indexes will be rebuilt, * excluding invalid indexes and any indexes used in exclusion constraints, * but including its associated toast table indexes. For indexes, the index - * itself will be rebuilt. + * itself will be rebuilt. If "ispartitioned", then avoid overwriting progress + * of concrrently reindex of a partitioned index. * * The locks taken on parent tables and involved indexes are kept until the * transaction is committed, at which point a session lock is taken on each @@ -3193,7 +3229,8 @@ ReindexMultipleInternal(List *relids, ReindexParams *params) * anyway, and a non-concurrent reindex is more efficient. */ static bool -ReindexRelationConcurrently(Oid relationOid, ReindexParams *params) +ReindexRelationConcurrently(Oid relationOid, ReindexParams *params, + bool ispartitioned) { typedef struct ReindexIndexInfo { @@ -3215,13 +3252,13 @@ ReindexRelationConcurrently(Oid relationOid, ReindexParams *params) char *relationName = NULL; char *relationNamespace = NULL; PGRUsage ru0; + int64 progress_vals[4]; const int progress_index[] = { PROGRESS_CREATEIDX_COMMAND, PROGRESS_CREATEIDX_PHASE, - PROGRESS_CREATEIDX_INDEX_OID, - PROGRESS_CREATEIDX_ACCESS_METHOD_OID + PROGRESS_CREATEIDX_ACCESS_METHOD_OID, + PROGRESS_CREATEIDX_INDEX_OID }; - int64 progress_vals[4]; /* * Create a memory context that will survive forced transaction commits we @@ -3545,14 +3582,16 @@ ReindexRelationConcurrently(Oid relationOid, ReindexParams *params) if (indexRel->rd_rel->relpersistence == RELPERSISTENCE_TEMP) elog(ERROR, "cannot reindex a temporary table concurrently"); - pgstat_progress_start_command(PROGRESS_COMMAND_CREATE_INDEX, - idx->tableId); + if (!ispartitioned) + pgstat_progress_start_command(PROGRESS_COMMAND_CREATE_INDEX, + idx->tableId); progress_vals[0] = PROGRESS_CREATEIDX_COMMAND_REINDEX_CONCURRENTLY; progress_vals[1] = 0; /* initializing */ - progress_vals[2] = idx->indexId; - progress_vals[3] = idx->amId; - pgstat_progress_update_multi_param(4, progress_index, progress_vals); + progress_vals[2] = idx->amId; + progress_vals[3] = idx->indexId; + pgstat_progress_update_multi_param(ispartitioned ? 3 : 4, progress_index, + progress_vals); /* Choose a temporary relation name for the new index */ concurrentName = ChooseRelationName(get_rel_name(idx->indexId), @@ -3700,12 +3739,14 @@ ReindexRelationConcurrently(Oid relationOid, ReindexParams *params) * Update progress for the index to build, with the correct parent * table involved. */ - pgstat_progress_start_command(PROGRESS_COMMAND_CREATE_INDEX, newidx->tableId); + if (!ispartitioned) + pgstat_progress_start_command(PROGRESS_COMMAND_CREATE_INDEX, newidx->tableId); progress_vals[0] = PROGRESS_CREATEIDX_COMMAND_REINDEX_CONCURRENTLY; progress_vals[1] = PROGRESS_CREATEIDX_PHASE_BUILD; - progress_vals[2] = newidx->indexId; - progress_vals[3] = newidx->amId; - pgstat_progress_update_multi_param(4, progress_index, progress_vals); + progress_vals[2] = newidx->amId; + progress_vals[3] = newidx->indexId; + pgstat_progress_update_multi_param(ispartitioned ? 3 : 4, progress_index, + progress_vals); /* Perform concurrent build of new index */ index_concurrently_build(newidx->tableId, newidx->indexId); @@ -3764,13 +3805,15 @@ ReindexRelationConcurrently(Oid relationOid, ReindexParams *params) * Update progress for the index to build, with the correct parent * table involved. */ - pgstat_progress_start_command(PROGRESS_COMMAND_CREATE_INDEX, - newidx->tableId); + if (!ispartitioned) + pgstat_progress_start_command(PROGRESS_COMMAND_CREATE_INDEX, + newidx->tableId); progress_vals[0] = PROGRESS_CREATEIDX_COMMAND_REINDEX_CONCURRENTLY; progress_vals[1] = PROGRESS_CREATEIDX_PHASE_VALIDATE_IDXSCAN; - progress_vals[2] = newidx->indexId; - progress_vals[3] = newidx->amId; - pgstat_progress_update_multi_param(4, progress_index, progress_vals); + progress_vals[2] = newidx->amId; + progress_vals[3] = newidx->indexId; + pgstat_progress_update_multi_param(ispartitioned ? 3 : 4, progress_index, + progress_vals); validate_index(newidx->tableId, newidx->indexId, snapshot); @@ -4000,7 +4043,8 @@ ReindexRelationConcurrently(Oid relationOid, ReindexParams *params) MemoryContextDelete(private_context); - pgstat_progress_end_command(); + if (!ispartitioned) + pgstat_progress_end_command(); return true; } -- 2.17.0