progress reporting for partitioned REINDEX
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
Attachments:
0001-WIP-progress-reporting-for-CREATE-INDEX-on-partition.patchtext/x-diff; charset=us-asciiDownload
From b04d46958b16bc07ecd6c2f07220599ecb304b4d Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryzbyj@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
On Tue, 16 Feb 2021, 07:42 Justin Pryzby, <pryzby@telsasoft.com> wrote:
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_AMI don't understand why e66bcfb4c added multiple calls to
pgstat_progress_start_command().
These were added to report the index and table that are currently
being worked on in concurrent reindexes of tables, schemas and
databases. Before that commit, it would only report up to the last
index being prepared in phase 1, leaving the user with no info on
which index is being rebuilt.
Why pgstat_progress_start_command specifically was chosen? That is
because there is no method to update the
beentry->st_progress_command_target other than through
stat_progress_start_command, and according to the docs that field
should contain the tableId of the index that is currently being worked
on. This field needs a pgstat_progress_start_command because CIC / RiC
reindexes all indexes concurrently at the same time (and not grouped
by e.g. table), so we must re-start reporting for each index in each
new phase in which we report data to get the heapId reported correctly
for that index.
With regards,
Matthias van de Meent
On Tue, Feb 16, 2021 at 12:39:08PM +0100, Matthias van de Meent wrote:
These were added to report the index and table that are currently
being worked on in concurrent reindexes of tables, schemas and
databases. Before that commit, it would only report up to the last
index being prepared in phase 1, leaving the user with no info on
which index is being rebuilt.
Nothing much to add on top of what Matthias is saying here. REINDEX
for partitioned relations builds first the full list of partitions to
work on, and then processes each one of them in a separate
transaction. This is consistent with what we do for other commands
that need to handle an object different than a non-partitioned table
or a non-partitioned index. The progress reporting has to report the
index whose storage is manipulated and its parent table.
Why pgstat_progress_start_command specifically was chosen? That is
because there is no method to update the
beentry->st_progress_command_target other than through
stat_progress_start_command, and according to the docs that field
should contain the tableId of the index that is currently being worked
on. This field needs a pgstat_progress_start_command because CIC / RiC
reindexes all indexes concurrently at the same time (and not grouped
by e.g. table), so we must re-start reporting for each index in each
new phase in which we report data to get the heapId reported correctly
for that index.
Using pgstat_progress_start_command() for this purpose is fine IMO.
This provides enough information for the user without complicating
more this API layer.
- 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);
You cannot do that, this would clobber the progress information of any
upper layer that already reports something to the progress infra in
the backend's MyProc. CLUSTER is one example calling reindex_relation()
that does *not* want progress reporting to happen in REINDEX. + /* 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
+ };
This does not make sense in ReindexPartitions() IMO because this
relation is not reindexed as it has no storage, and you would lose the
context of each partition.Something that we may want to study instead is whether we'd like to
report to the user the set of relations a REINDEX command is working
on and on which relation the work is currently done. But I am not
really sure that we need that as long a we have a VERBOSE option that
lets us know via the logs what already happened in a single command.
I see no bug here.
--
Michael
On Wed, Feb 17, 2021 at 02:55:04PM +0900, Michael Paquier wrote:
I see no bug here.
pg_stat_progress_create_index includes partitions_{done,total} for
CREATE INDEX p, so isn't it strange if it wouldn't do likewise for
REINDEX INDEX p ?
--
Justin
On Wed, Feb 17, 2021 at 12:10:43AM -0600, Justin Pryzby wrote:
On Wed, Feb 17, 2021 at 02:55:04PM +0900, Michael Paquier wrote:
I see no bug here.
pg_stat_progress_create_index includes partitions_{done,total} for
CREATE INDEX p, so isn't it strange if it wouldn't do likewise for
REINDEX INDEX p ?
There is always room for improvement. This stuff applies now only
when creating an index in the non-concurrent case because an index
cannot be created on a partitioned table concurrently, and this
behavior is documented as such. If we are going to improve this area,
it seems to me that we may want to consider more cases than just the
case of partitions, as it could also help the monitoring of REINDEX on
schemas and databases.
I don't think that this fits as an open item. That's just a different
feature.
--
Michael
On Wed, Feb 17, 2021 at 03:36:20PM +0900, Michael Paquier wrote:
On Wed, Feb 17, 2021 at 12:10:43AM -0600, Justin Pryzby wrote:
On Wed, Feb 17, 2021 at 02:55:04PM +0900, Michael Paquier wrote:
I see no bug here.
pg_stat_progress_create_index includes partitions_{done,total} for
CREATE INDEX p, so isn't it strange if it wouldn't do likewise for
REINDEX INDEX p ?There is always room for improvement. This stuff applies now only
when creating an index in the non-concurrent case because an index
cannot be created on a partitioned table concurrently, and this
behavior is documented as such. If we are going to improve this area,
it seems to me that we may want to consider more cases than just the
case of partitions, as it could also help the monitoring of REINDEX on
schemas and databases.I don't think that this fits as an open item. That's just a different
feature.
I see it as an omission in the existing feature.
Since v13, pg_stat_progress_create_index does progress reports for CREATE INDEX
(partitioned and nonpartitioned), and REINDEX of nonpartitioned tables.
When we implemented REINDEX of partitioned tables, it should've handled
progress reporting in the fields where that's reported for CREATE INDEX.
Or else we should document that "partitions_total/done are not populated for
REINDEX of a partitioned table as they are for CREATE INDEX".
--
Justin
On Wed, Feb 17, 2021 at 10:24:37AM -0600, Justin Pryzby wrote:
When we implemented REINDEX of partitioned tables, it should've handled
progress reporting in the fields where that's reported for CREATE INDEX.
Or else we should document that "partitions_total/done are not populated for
REINDEX of a partitioned table as they are for CREATE INDEX".
CREATE INDEX and REINDEX are two completely separate commands, with
separate code paths, and mostly separate logics. When it comes to
REINDEX, the information that is currently showed to the user is not
incorrect, but in line with what the progress reporting of ~13 is able
to do: each index gets reported with its parent table one-by-one,
depending on if CONCURRENTLY is used or not, in consistency with what
ReindexMultipleTables() does for all the REINDEX commands working on
multiple objects, processing in one transaction each object listed
previously.
Now, coming back to the ask, I think that if we want to provide some
information in the REINDEX with the list of relations to work on, we
are going to need more fields than what we have now, to report:
1) The total number of indexes on which REINDEX is working on for the
current relation worked on.
2) The n-th index being worked on by REINDEX, as of the number of
indexes in 1).
3) The total number of relations a given command is working on, aka
the number of tables REINDEX SCHEMA, DATABASE, SYSTEM or REINDEX on a
partitioned relation has accumulated.
4) The n-th relation listed in 3) currently worked on.
The current columns partitions_total and partitions_done are partially
able to fill in the roles of 3) and 4), if we'd rename those columns
to relations_done and relations_total, still they could also mean 1)
and 2) in some contexts, like the number of indexes worked on for a
single relation. So the problem is more complex than you make it
sound, and needs to consider a certain number of cases to be
consistent across all the REINDEX commands that exist. In short, this
is not only a problem related to partitioned tables.
I have no issues with documenting more precisely on which commands
partitions_total and partitions_done apply currently, by citing the
commands where these are effective. We do that for index_relid for
instance.
--
Michael
On Thu, Feb 18, 2021 at 02:17:00PM +0900, Michael Paquier wrote:
I have no issues with documenting more precisely on which commands
partitions_total and partitions_done apply currently, by citing the
commands where these are effective. We do that for index_relid for
instance.
Please find attached a patch to do that. Justin, what do you think?
--
Michael
Attachments:
reindex-progress-doc.patchtext/x-diff; charset=us-asciiDownload
diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml
index c602ee4427..3513e127b7 100644
--- a/doc/src/sgml/monitoring.sgml
+++ b/doc/src/sgml/monitoring.sgml
@@ -5716,6 +5716,7 @@ SELECT pg_stat_get_backend_pid(s.backendid) AS pid,
<para>
When creating an index on a partitioned table, this column is set to
the total number of partitions on which the index is to be created.
+ This field is <literal>0</literal> during a <literal>REINDEX</literal>.
</para></entry>
</row>
@@ -5726,6 +5727,7 @@ SELECT pg_stat_get_backend_pid(s.backendid) AS pid,
<para>
When creating an index on a partitioned table, this column is set to
the number of partitions on which the index has been completed.
+ This field is <literal>0</literal> during a <literal>REINDEX</literal>.
</para></entry>
</row>
</tbody>
On Fri, Feb 19, 2021 at 03:06:04PM +0900, Michael Paquier wrote:
On Thu, Feb 18, 2021 at 02:17:00PM +0900, Michael Paquier wrote:
I have no issues with documenting more precisely on which commands
partitions_total and partitions_done apply currently, by citing the
commands where these are effective. We do that for index_relid for
instance.Please find attached a patch to do that. Justin, what do you think?
Looks fine.
I removed this from opened items.
Also, I noticed that vacuum recurses into partition heirarchies since v10, but
pg_stat_progress_vacuum also doesn't show anything about the parent table or
the progress of recursing through the hierarchy.
--
Justin
On Fri, Feb 19, 2021 at 12:12:54AM -0600, Justin Pryzby wrote:
Looks fine.
Thanks, applied then to clarify things.
Also, I noticed that vacuum recurses into partition heirarchies since v10, but
pg_stat_progress_vacuum also doesn't show anything about the parent table or
the progress of recursing through the hierarchy.
Yeah, that's an area where it would be possible to improve the
monitoring, for both autovacuums and manual VACUUMs.
--
Michael
On Sat, Feb 20, 2021 at 10:37:08AM +0900, Michael Paquier wrote:
Also, I noticed that vacuum recurses into partition heirarchies since v10, but
pg_stat_progress_vacuum also doesn't show anything about the parent table or
the progress of recursing through the hierarchy.Yeah, that's an area where it would be possible to improve the
monitoring, for both autovacuums and manual VACUUMs.
I was thinking that instead of reporting partitions_done/partitions_total in
the individual progress views, maybe the progress across partitions should be
reported in a separate pg_stat_progress_partitioned. This would apply to my
CLUSTER patch as well as VACUUM. I haven't though about the implementation,
though.
If the partitions_done/total were *removed* from the create_index view, that
would resolve the odd behavior that a single row simultanously shows 1) the
overall progress of the operation across partitions; and, 2) the detailed
information about the status of the operation on the current leaf partition.
However I guess it's not general enough to support progress reports of
execution of planned (not utility) statements.
--
Justin