REINDEX not updating partition progress
Hi,
While working on CIC for partitioned tables [1]/messages/by-id/55cfae76-2ffa-43ed-a7e7-901bffbebee4@gmail.com, I noticed that REINDEX
for partitioned tables is not tracking keeping progress of partitioned
tables, so I'm creating a separate thread for this fix as suggested.
The way REINDEX for partitioned tables works now ReindexMultipleInternal
treats every partition as an independent table without keeping track of
partition_done/total counts, because this is just generic code for
processing multiple tables. The patch addresses that by passing down the
knowledge a flag to distinguish partitions from independent tables
in ReindexMultipleInternal and its callees. I also noticed that the
partitioned CREATE INDEX progress tracking could also benefit from
progress_index_partition_done function that zeroizes params in addition
to incrementing the counter, so I applied it there as well.
[1]: /messages/by-id/55cfae76-2ffa-43ed-a7e7-901bffbebee4@gmail.com
/messages/by-id/55cfae76-2ffa-43ed-a7e7-901bffbebee4@gmail.com
Attachments:
0001-make-REINDEX-track-partition-progress.patchtext/x-patch; charset=UTF-8; name=0001-make-REINDEX-track-partition-progress.patchDownload
From 18baa028e1cc5c39347b9126ec1a96eb99e8e3e1 Mon Sep 17 00:00:00 2001
From: Ilya Gladyshev <ilya.v.gladyshev@gmail.com>
Date: Tue, 11 Jun 2024 17:48:08 +0100
Subject: [PATCH] make REINDEX track partition progress
---
src/backend/catalog/index.c | 11 ++++--
src/backend/commands/indexcmds.c | 61 +++++++++++++++++++++++++++++---
src/include/catalog/index.h | 1 +
3 files changed, 65 insertions(+), 8 deletions(-)
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index a819b4197c..90488f9140 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -3560,6 +3560,7 @@ reindex_index(const ReindexStmt *stmt, Oid indexId,
volatile bool skipped_constraint = false;
PGRUsage ru0;
bool progress = ((params->options & REINDEXOPT_REPORT_PROGRESS) != 0);
+ bool partition = ((params->options & REINDEXOPT_PARTITION) != 0);
bool set_tablespace = false;
pg_rusage_init(&ru0);
@@ -3605,8 +3606,9 @@ reindex_index(const ReindexStmt *stmt, Oid indexId,
indexId
};
- pgstat_progress_start_command(PROGRESS_COMMAND_CREATE_INDEX,
- heapId);
+ if (!partition)
+ pgstat_progress_start_command(PROGRESS_COMMAND_CREATE_INDEX,
+ heapId);
pgstat_progress_update_multi_param(2, progress_cols, progress_vals);
}
@@ -3846,8 +3848,11 @@ reindex_index(const ReindexStmt *stmt, Oid indexId,
index_close(iRel, NoLock);
table_close(heapRelation, NoLock);
- if (progress)
+ if (progress && !partition)
+ {
+ /* progress for partitions is tracked in the caller */
pgstat_progress_end_command();
+ }
}
/*
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 2caab88aa5..2196279108 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -113,6 +113,7 @@ static bool ReindexRelationConcurrently(const ReindexStmt *stmt,
const ReindexParams *params);
static void update_relispartition(Oid relationId, bool newval);
static inline void set_indexsafe_procflags(void);
+static inline void progress_index_partition_done(void);
/*
* callback argument type for RangeVarCallbackForReindexIndex()
@@ -1569,7 +1570,7 @@ DefineIndex(Oid tableId,
else
{
/* Update progress for an intermediate partitioned index itself */
- pgstat_progress_incr_param(PROGRESS_CREATEIDX_PARTITIONS_DONE, 1);
+ progress_index_partition_done();
}
return address;
@@ -1590,7 +1591,7 @@ DefineIndex(Oid tableId,
if (!OidIsValid(parentIndexId))
pgstat_progress_end_command();
else
- pgstat_progress_incr_param(PROGRESS_CREATEIDX_PARTITIONS_DONE, 1);
+ progress_index_partition_done();
return address;
}
@@ -3213,6 +3214,14 @@ ReindexPartitions(const ReindexStmt *stmt, Oid relid, const ReindexParams *param
ListCell *lc;
ErrorContextCallback errcallback;
ReindexErrorInfo errinfo;
+ ReindexParams newparams;
+ int progress_params[3] = {
+ PROGRESS_CREATEIDX_COMMAND,
+ PROGRESS_CREATEIDX_PHASE,
+ PROGRESS_CREATEIDX_PARTITIONS_TOTAL
+ };
+ int64 progress_values[3];
+ Oid heapId = relid;
Assert(RELKIND_HAS_PARTITIONS(relkind));
@@ -3274,11 +3283,28 @@ ReindexPartitions(const ReindexStmt *stmt, Oid relid, const ReindexParams *param
MemoryContextSwitchTo(old_context);
}
+ if (relkind == RELKIND_PARTITIONED_INDEX)
+ {
+ heapId = IndexGetRelation(relid, true);
+ }
+
+ progress_values[0] = (params->options & REINDEXOPT_CONCURRENTLY) != 0 ?
+ PROGRESS_CREATEIDX_COMMAND_REINDEX_CONCURRENTLY :
+ PROGRESS_CREATEIDX_COMMAND_REINDEX;
+ progress_values[1] = 0;
+ progress_values[2] = list_length(partitions);
+ pgstat_progress_start_command(PROGRESS_COMMAND_CREATE_INDEX, heapId);
+ pgstat_progress_update_multi_param(3, progress_params, progress_values);
+
/*
* Process each partition listed in a separate transaction. Note that
* this commits and then starts a new transaction immediately.
*/
- ReindexMultipleInternal(stmt, partitions, params);
+ newparams = *params;
+ newparams.options |= REINDEXOPT_PARTITION;
+ ReindexMultipleInternal(stmt, partitions, &newparams);
+
+ pgstat_progress_end_command();
/*
* Clean up working storage --- note we must do this after
@@ -3299,6 +3325,7 @@ static void
ReindexMultipleInternal(const ReindexStmt *stmt, const List *relids, const ReindexParams *params)
{
ListCell *l;
+ bool partitions = ((params->options & REINDEXOPT_PARTITION) != 0);
PopActiveSnapshot();
CommitTransactionCommand();
@@ -3392,6 +3419,9 @@ ReindexMultipleInternal(const ReindexStmt *stmt, const List *relids, const Reind
}
CommitTransactionCommand();
+
+ if (partitions)
+ progress_index_partition_done();
}
StartTransactionCommand();
@@ -3444,6 +3474,7 @@ ReindexRelationConcurrently(const ReindexStmt *stmt, Oid relationOid, const Rein
char *relationName = NULL;
char *relationNamespace = NULL;
PGRUsage ru0;
+ bool partition = ((params->options & REINDEXOPT_PARTITION) != 0);
const int progress_index[] = {
PROGRESS_CREATEIDX_COMMAND,
PROGRESS_CREATEIDX_PHASE,
@@ -3787,7 +3818,8 @@ ReindexRelationConcurrently(const ReindexStmt *stmt, Oid relationOid, const Rein
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 (!partition)
+ pgstat_progress_start_command(PROGRESS_COMMAND_CREATE_INDEX, idx->tableId);
progress_vals[0] = PROGRESS_CREATEIDX_COMMAND_REINDEX_CONCURRENTLY;
progress_vals[1] = 0; /* initializing */
@@ -4261,7 +4293,8 @@ ReindexRelationConcurrently(const ReindexStmt *stmt, Oid relationOid, const Rein
MemoryContextDelete(private_context);
- pgstat_progress_end_command();
+ if (!partition)
+ pgstat_progress_end_command();
return true;
}
@@ -4454,3 +4487,21 @@ set_indexsafe_procflags(void)
ProcGlobal->statusFlags[MyProc->pgxactoff] = MyProc->statusFlags;
LWLockRelease(ProcArrayLock);
}
+
+static inline void
+progress_index_partition_done(void)
+{
+ int nparam = 6;
+ const int progress_idx[] = {
+ PROGRESS_CREATEIDX_INDEX_OID,
+ PROGRESS_CREATEIDX_ACCESS_METHOD_OID,
+ PROGRESS_CREATEIDX_PHASE,
+ PROGRESS_CREATEIDX_SUBPHASE,
+ PROGRESS_CREATEIDX_TUPLES_TOTAL,
+ PROGRESS_CREATEIDX_TUPLES_DONE
+ };
+ const int64 progress_vals[] = {0, 0, 0, 0, 0, 0};
+
+ pgstat_progress_update_multi_param(nparam, progress_idx, progress_vals);
+ pgstat_progress_incr_param(PROGRESS_CREATEIDX_PARTITIONS_DONE, 1);
+}
diff --git a/src/include/catalog/index.h b/src/include/catalog/index.h
index 7d434f8e65..ccba65fbbf 100644
--- a/src/include/catalog/index.h
+++ b/src/include/catalog/index.h
@@ -42,6 +42,7 @@ typedef struct ReindexParams
#define REINDEXOPT_REPORT_PROGRESS 0x02 /* report pgstat progress */
#define REINDEXOPT_MISSING_OK 0x04 /* skip missing relations */
#define REINDEXOPT_CONCURRENTLY 0x08 /* concurrent mode */
+#define REINDEXOPT_PARTITION 0x10 /* reindexing is done as part of partitioned table/index reindex */
/* state info for validate_index bulkdelete callback */
typedef struct ValidateIndexState
--
2.43.0
On Fri, Jul 12, 2024 at 11:07:49PM +0100, Ilya Gladyshev wrote:
While working on CIC for partitioned tables [1], I noticed that REINDEX for
partitioned tables is not tracking keeping progress of partitioned tables,
so I'm creating a separate thread for this fix as suggested.
This limitation is not a bug, because we already document that
partitions_total and partitions_done are 0 during a REINDEX. Compared
to CREATE INDEX, a REINDEX is a bit wilder as it would work on all the
indexes for all the partitions, providing this information makes
sense.
Agreed that this could be better, but what's now on HEAD is not wrong
either.
The way REINDEX for partitioned tables works now ReindexMultipleInternal
treats every partition as an independent table without keeping track of
partition_done/total counts, because this is just generic code for
processing multiple tables. The patch addresses that by passing down the
knowledge a flag to distinguish partitions from independent tables
in ReindexMultipleInternal and its callees. I also noticed that the
partitioned CREATE INDEX progress tracking could also benefit from
progress_index_partition_done function that zeroizes params in addition to
incrementing the counter, so I applied it there as well.
Still it does not do it because we know that the fields are going to
be overwritten pretty quickly anyway, no? For now, we could just do
without progress_index_partition_done(), I guess, keeping it to the
incrementation of PROGRESS_CREATEIDX_PARTITIONS_DONE. There's an
argument that this makes the code slightly easier to follow, with less
wrappers around the progress reporting.
+ int progress_params[3] = {
+ PROGRESS_CREATEIDX_COMMAND,
+ PROGRESS_CREATEIDX_PHASE,
+ PROGRESS_CREATEIDX_PARTITIONS_TOTAL
+ };
+ int64 progress_values[3];
+ Oid heapId = relid;Rather than having new code blocks, let's use a style consistent with
DefineIndex() where we have the pgstat_progress_update_multi_param(),
with a single {} block.
Adding the counter increment at the end of the loop in
ReindexMultipleInternal() is a good idea. It considers both the
concurrent and non-concurrent cases.
+ progress_values[2] = list_length(partitions);
+ pgstat_progress_start_command(PROGRESS_COMMAND_CREATE_INDEX, heapId);Hmm. Is setting the relid only once for pg_stat_progress_create_index
the best choice there is? Could it be better to report the partition
OID instead? Let's remember that indexes may have been attached with
names inconsistent with the partitioned table's index. It is a bit
confusing to stick to the relid all the partitioned table all the
time, for all the indexes of all the partitions reindexed. Could it
be better to actually introduce an entirely new field to the progress
table? What I would look for is more information:
1) the partitioned table OID on which the REINDEX runs
2) the partition table being processed
3) the index OID being processed (old and new for concurrent case).
The patch sets 1) to the OID of the partitioned table, lacks 2) and
sets 3) each time an index is rebuilt.
--
Michael
19 июля 2024 г., в 04:17, Michael Paquier <michael@paquier.xyz>
написал(а):On Fri, Jul 12, 2024 at 11:07:49PM +0100, Ilya Gladyshev wrote:
While working on CIC for partitioned tables [1], I noticed that
REINDEX for
partitioned tables is not tracking keeping progress of partitioned
tables,
so I'm creating a separate thread for this fix as suggested.This limitation is not a bug, because we already document that
partitions_total and partitions_done are 0 during a REINDEX. Compared
to CREATE INDEX, a REINDEX is a bit wilder as it would work on all the
indexes for all the partitions, providing this information makes
sense.Agreed that this could be better, but what's now on HEAD is not wrong
either.
Thanks for pointing it out, I didn’t notice it. You’re right, this not a
bug, but an improvement. Will remove the bits from the documentation as
well.
Still it does not do it because we know that the fields are going to
be overwritten pretty quickly anyway, no? For now, we could just do
without progress_index_partition_done(), I guess, keeping it to the
incrementation of PROGRESS_CREATEIDX_PARTITIONS_DONE. There's an
argument that this makes the code slightly easier to follow, with less
wrappers around the progress reporting.
The use-case that I thought this would improve is REINDEX CONCURRENT,
where data from later stages of the previous partitions would linger for
quite a while before it gets to the same stage of the current partition.
I don’t think this is of big importance, so I’m ok with making code
simpler and leaving it out.
+intprogress_params[3] = { +PROGRESS_CREATEIDX_COMMAND, +PROGRESS_CREATEIDX_PHASE, +PROGRESS_CREATEIDX_PARTITIONS_TOTAL +}; +int64progress_values[3]; +OidheapId = relid;Rather than having new code blocks, let's use a style consistent with
DefineIndex() where we have the pgstat_progress_update_multi_param(),
with a single {} block.
Fixed.
Adding the counter increment at the end of the loop in
ReindexMultipleInternal() is a good idea. It considers both the
concurrent and non-concurrent cases.
Another reason to do so is that reindex_relation calls itself
recursively, so it'd require some additional mechanism so that it
wouldn't increment multiple times per partition.
+ progress_values[2] = list_length(partitions); + pgstat_progress_start_command(PROGRESS_COMMAND_CREATE_INDEX, heapId);Hmm. Is setting the relid only once for pg_stat_progress_create_index
the best choice there is? Could it be better to report the partition
OID instead?
Technically, we could do this, but it looks like no other commands do it
and currently there’s no API to do it without erasing the rest of
progress information.
Let's remember that indexes may have been attached with
names inconsistent with the partitioned table's index. It is a bit
confusing to stick to the relid all the partitioned table all the
time, for all the indexes of all the partitions reindexed.
I agree that it’s a bit confusing especially because documentation gives
no hints about it. Judging by documentation, I would expect relid and
index_relid to correspond to the same table. However, if we say that
this field represents the oid of the relation on which the command was
invoked, then I think it does make sense. Edited documentation to say
that in the new patch.
Could it
be better to actually introduce an entirely new field to the progress
table? What I would look for is more information:
1) the partitioned table OID on which the REINDEX runs
2) the partition table being processed
3) the index OID being processed (old and new for concurrent case).The patch sets 1) to the OID of the partitioned table, lacks 2) and
sets 3) each time an index is rebuilt.
—
Michael
I like this approach more, but I’m not sure whether adding another field
for partition oid is worth it, since we already have index_relid that we
can use to join with pg_index to get that. On the other hand,
index_relid is missing during regular CREATE INDEX, so this new field
could be useful to indicate which table is being indexed in this case.
I'm on the fence about this, attached this as a separate patch, if you
think it's a good idea.
Thank you for the review!
Attachments:
v2-0002-partition_relid-column-for-create-index-progress.patchtext/x-patch; charset=UTF-8; name=v2-0002-partition_relid-column-for-create-index-progress.patchDownload
From 61e82b773e1c21dcb50a298bb205449ff17c90dc Mon Sep 17 00:00:00 2001
From: Ilya Gladyshev <ilya.v.gladyshev@gmail.com>
Date: Sun, 21 Jul 2024 01:40:15 +0100
Subject: [PATCH v2 2/2] partition_relid column for create index progress
---
doc/src/sgml/monitoring.sgml | 9 +++++++++
src/backend/catalog/system_views.sql | 3 ++-
src/backend/commands/indexcmds.c | 18 +++++++++++++-----
src/include/commands/progress.h | 1 +
src/test/regress/expected/rules.out | 3 ++-
5 files changed, 27 insertions(+), 7 deletions(-)
diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml
index 00bb423288..595f19a8f8 100644
--- a/doc/src/sgml/monitoring.sgml
+++ b/doc/src/sgml/monitoring.sgml
@@ -6006,6 +6006,15 @@ FROM pg_stat_get_backend_idset() AS backendid;
<literal>0</literal> when the index is not partitioned.
</para></entry>
</row>
+ <row>
+ <entry role="catalog_table_entry"><para role="column_definition">
+ <structfield>partition_relid</structfield> <type>bigint</type>
+ </para>
+ <para>
+ OID of the partition on which the index is currently being created.
+ <literal>0</literal> when the index is not partitioned.
+ </para></entry>
+ </row>
</tbody>
</tgroup>
</table>
diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql
index 19cabc9a47..00b7f3bda8 100644
--- a/src/backend/catalog/system_views.sql
+++ b/src/backend/catalog/system_views.sql
@@ -1280,7 +1280,8 @@ CREATE VIEW pg_stat_progress_create_index AS
S.param12 AS tuples_total,
S.param13 AS tuples_done,
S.param14 AS partitions_total,
- S.param15 AS partitions_done
+ S.param15 AS partitions_done,
+ S.param18 AS partition_relid
FROM pg_stat_get_progress_info('CREATE INDEX') AS S
LEFT JOIN pg_database D ON S.datid = D.oid;
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index d4c1811f50..284f7a919a 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -617,6 +617,10 @@ DefineIndex(Oid tableId,
PROGRESS_CREATEIDX_COMMAND_CREATE_CONCURRENTLY :
PROGRESS_CREATEIDX_COMMAND_CREATE);
}
+ else
+ {
+ pgstat_progress_update_param(PROGRESS_CREATEIDX_PARTITION_RELID, tableId);
+ }
/*
* No index OID to report yet
@@ -3486,9 +3490,10 @@ ReindexRelationConcurrently(const ReindexStmt *stmt, Oid relationOid, const Rein
PROGRESS_CREATEIDX_COMMAND,
PROGRESS_CREATEIDX_PHASE,
PROGRESS_CREATEIDX_INDEX_OID,
- PROGRESS_CREATEIDX_ACCESS_METHOD_OID
+ PROGRESS_CREATEIDX_ACCESS_METHOD_OID,
+ PROGRESS_CREATEIDX_PARTITION_RELID
};
- int64 progress_vals[4];
+ int64 progress_vals[5];
/*
* Create a memory context that will survive forced transaction commits we
@@ -3832,7 +3837,8 @@ ReindexRelationConcurrently(const ReindexStmt *stmt, Oid relationOid, const Rein
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[4] = partition ? idx->tableId : 0;
+ pgstat_progress_update_multi_param(5, progress_index, progress_vals);
/* Choose a temporary relation name for the new index */
concurrentName = ChooseRelationName(get_rel_name(idx->indexId),
@@ -4007,7 +4013,8 @@ ReindexRelationConcurrently(const ReindexStmt *stmt, Oid relationOid, const Rein
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[4] = partition ? newidx->tableId : 0;
+ pgstat_progress_update_multi_param(5, progress_index, progress_vals);
/* Perform concurrent build of new index */
index_concurrently_build(newidx->tableId, newidx->indexId);
@@ -4072,7 +4079,8 @@ ReindexRelationConcurrently(const ReindexStmt *stmt, Oid relationOid, const Rein
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[4] = partition ? newidx->tableId : 0;
+ pgstat_progress_update_multi_param(5, progress_index, progress_vals);
validate_index(newidx->tableId, newidx->indexId, snapshot);
diff --git a/src/include/commands/progress.h b/src/include/commands/progress.h
index 5616d64523..6cd8b41a4c 100644
--- a/src/include/commands/progress.h
+++ b/src/include/commands/progress.h
@@ -89,6 +89,7 @@
#define PROGRESS_CREATEIDX_PARTITIONS_TOTAL 13
#define PROGRESS_CREATEIDX_PARTITIONS_DONE 14
/* 15 and 16 reserved for "block number" metrics */
+#define PROGRESS_CREATEIDX_PARTITION_RELID 17
/* Phases of CREATE INDEX (as advertised via PROGRESS_CREATEIDX_PHASE) */
#define PROGRESS_CREATEIDX_PHASE_WAIT_1 1
diff --git a/src/test/regress/expected/rules.out b/src/test/regress/expected/rules.out
index 4c789279e5..2bcb72c014 100644
--- a/src/test/regress/expected/rules.out
+++ b/src/test/regress/expected/rules.out
@@ -2027,7 +2027,8 @@ pg_stat_progress_create_index| SELECT s.pid,
s.param12 AS tuples_total,
s.param13 AS tuples_done,
s.param14 AS partitions_total,
- s.param15 AS partitions_done
+ s.param15 AS partitions_done,
+ s.param18 AS partition_relid
FROM (pg_stat_get_progress_info('CREATE INDEX'::text) s(pid, datid, relid, param1, param2, param3, param4, param5, param6, param7, param8, param9, param10, param11, param12, param13, param14, param15, param16, param17, param18, param19, param20)
LEFT JOIN pg_database d ON ((s.datid = d.oid)));
pg_stat_progress_vacuum| SELECT s.pid,
--
2.43.0
v2-0001-make-REINDEX-track-partition-progress.patchtext/x-patch; charset=UTF-8; name=v2-0001-make-REINDEX-track-partition-progress.patchDownload
From 5f3d4c5b4877b31bb95caa6086d1939aef4f3d12 Mon Sep 17 00:00:00 2001
From: Ilya Gladyshev <ilya.v.gladyshev@gmail.com>
Date: Sat, 20 Jul 2024 14:08:33 +0100
Subject: [PATCH v2 1/2] make REINDEX track partition progress
---
doc/src/sgml/monitoring.sgml | 8 ++----
src/backend/catalog/index.c | 11 ++++++--
src/backend/commands/indexcmds.c | 48 ++++++++++++++++++++++++++++----
src/include/catalog/index.h | 1 +
4 files changed, 55 insertions(+), 13 deletions(-)
diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml
index 55417a6fa9..00bb423288 100644
--- a/doc/src/sgml/monitoring.sgml
+++ b/doc/src/sgml/monitoring.sgml
@@ -5888,7 +5888,7 @@ FROM pg_stat_get_backend_idset() AS backendid;
<structfield>relid</structfield> <type>oid</type>
</para>
<para>
- OID of the table on which the index is being created.
+ OID of the table on which the command was run.
</para></entry>
</row>
@@ -5992,8 +5992,7 @@ FROM pg_stat_get_backend_idset() AS backendid;
<para>
Total number of partitions on which the index is to be created
or attached, including both direct and indirect partitions.
- <literal>0</literal> during a <literal>REINDEX</literal>, or when
- the index is not partitioned.
+ <literal>0</literal> when the index is not partitioned.
</para></entry>
</row>
@@ -6004,8 +6003,7 @@ FROM pg_stat_get_backend_idset() AS backendid;
<para>
Number of partitions on which the index has already been created
or attached, including both direct and indirect partitions.
- <literal>0</literal> during a <literal>REINDEX</literal>, or when
- the index is not partitioned.
+ <literal>0</literal> when the index is not partitioned.
</para></entry>
</row>
</tbody>
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index a819b4197c..90488f9140 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -3560,6 +3560,7 @@ reindex_index(const ReindexStmt *stmt, Oid indexId,
volatile bool skipped_constraint = false;
PGRUsage ru0;
bool progress = ((params->options & REINDEXOPT_REPORT_PROGRESS) != 0);
+ bool partition = ((params->options & REINDEXOPT_PARTITION) != 0);
bool set_tablespace = false;
pg_rusage_init(&ru0);
@@ -3605,8 +3606,9 @@ reindex_index(const ReindexStmt *stmt, Oid indexId,
indexId
};
- pgstat_progress_start_command(PROGRESS_COMMAND_CREATE_INDEX,
- heapId);
+ if (!partition)
+ pgstat_progress_start_command(PROGRESS_COMMAND_CREATE_INDEX,
+ heapId);
pgstat_progress_update_multi_param(2, progress_cols, progress_vals);
}
@@ -3846,8 +3848,11 @@ reindex_index(const ReindexStmt *stmt, Oid indexId,
index_close(iRel, NoLock);
table_close(heapRelation, NoLock);
- if (progress)
+ if (progress && !partition)
+ {
+ /* progress for partitions is tracked in the caller */
pgstat_progress_end_command();
+ }
}
/*
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index c5a56c75f6..d4c1811f50 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -3217,9 +3217,16 @@ ReindexPartitions(const ReindexStmt *stmt, Oid relid, const ReindexParams *param
ListCell *lc;
ErrorContextCallback errcallback;
ReindexErrorInfo errinfo;
+ ReindexParams newparams;
+ Oid heapId = relid;
Assert(RELKIND_HAS_PARTITIONS(relkind));
+ if (relkind == RELKIND_PARTITIONED_INDEX)
+ {
+ heapId = IndexGetRelation(relid, true);
+ }
+
/*
* Check if this runs in a transaction block, with an error callback to
* provide more context under which a problem happens.
@@ -3278,11 +3285,33 @@ ReindexPartitions(const ReindexStmt *stmt, Oid relid, const ReindexParams *param
MemoryContextSwitchTo(old_context);
}
+
+ {
+ int progress_params[3] = {
+ PROGRESS_CREATEIDX_COMMAND,
+ PROGRESS_CREATEIDX_PHASE,
+ PROGRESS_CREATEIDX_PARTITIONS_TOTAL
+ };
+ int64 progress_values[3];
+
+ progress_values[0] = (params->options & REINDEXOPT_CONCURRENTLY) != 0 ?
+ PROGRESS_CREATEIDX_COMMAND_REINDEX_CONCURRENTLY :
+ PROGRESS_CREATEIDX_COMMAND_REINDEX;
+ progress_values[1] = 0;
+ progress_values[2] = list_length(partitions);
+ pgstat_progress_start_command(PROGRESS_COMMAND_CREATE_INDEX, heapId);
+ pgstat_progress_update_multi_param(3, progress_params, progress_values);
+ }
+
/*
* Process each partition listed in a separate transaction. Note that
* this commits and then starts a new transaction immediately.
*/
- ReindexMultipleInternal(stmt, partitions, params);
+ newparams = *params;
+ newparams.options |= REINDEXOPT_PARTITION;
+ ReindexMultipleInternal(stmt, partitions, &newparams);
+
+ pgstat_progress_end_command();
/*
* Clean up working storage --- note we must do this after
@@ -3303,6 +3332,7 @@ static void
ReindexMultipleInternal(const ReindexStmt *stmt, const List *relids, const ReindexParams *params)
{
ListCell *l;
+ bool partitions = ((params->options & REINDEXOPT_PARTITION) != 0);
PopActiveSnapshot();
CommitTransactionCommand();
@@ -3396,6 +3426,9 @@ ReindexMultipleInternal(const ReindexStmt *stmt, const List *relids, const Reind
}
CommitTransactionCommand();
+
+ if (partitions)
+ pgstat_progress_incr_param(PROGRESS_CREATEIDX_PARTITIONS_DONE, 1);
}
StartTransactionCommand();
@@ -3448,6 +3481,7 @@ ReindexRelationConcurrently(const ReindexStmt *stmt, Oid relationOid, const Rein
char *relationName = NULL;
char *relationNamespace = NULL;
PGRUsage ru0;
+ bool partition = ((params->options & REINDEXOPT_PARTITION) != 0);
const int progress_index[] = {
PROGRESS_CREATEIDX_COMMAND,
PROGRESS_CREATEIDX_PHASE,
@@ -3791,7 +3825,8 @@ ReindexRelationConcurrently(const ReindexStmt *stmt, Oid relationOid, const Rein
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 (!partition)
+ pgstat_progress_start_command(PROGRESS_COMMAND_CREATE_INDEX, idx->tableId);
progress_vals[0] = PROGRESS_CREATEIDX_COMMAND_REINDEX_CONCURRENTLY;
progress_vals[1] = 0; /* initializing */
@@ -3966,7 +4001,8 @@ ReindexRelationConcurrently(const ReindexStmt *stmt, Oid relationOid, const Rein
* Update progress for the index to build, with the correct parent
* table involved.
*/
- pgstat_progress_start_command(PROGRESS_COMMAND_CREATE_INDEX, newidx->tableId);
+ if (!partition)
+ 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;
@@ -4030,7 +4066,8 @@ ReindexRelationConcurrently(const ReindexStmt *stmt, Oid relationOid, const Rein
* Update progress for the index to build, with the correct parent
* table involved.
*/
- pgstat_progress_start_command(PROGRESS_COMMAND_CREATE_INDEX, newidx->tableId);
+ if (!partition)
+ 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;
@@ -4265,7 +4302,8 @@ ReindexRelationConcurrently(const ReindexStmt *stmt, Oid relationOid, const Rein
MemoryContextDelete(private_context);
- pgstat_progress_end_command();
+ if (!partition)
+ pgstat_progress_end_command();
return true;
}
diff --git a/src/include/catalog/index.h b/src/include/catalog/index.h
index 7d434f8e65..ccba65fbbf 100644
--- a/src/include/catalog/index.h
+++ b/src/include/catalog/index.h
@@ -42,6 +42,7 @@ typedef struct ReindexParams
#define REINDEXOPT_REPORT_PROGRESS 0x02 /* report pgstat progress */
#define REINDEXOPT_MISSING_OK 0x04 /* skip missing relations */
#define REINDEXOPT_CONCURRENTLY 0x08 /* concurrent mode */
+#define REINDEXOPT_PARTITION 0x10 /* reindexing is done as part of partitioned table/index reindex */
/* state info for validate_index bulkdelete callback */
typedef struct ValidateIndexState
--
2.43.0
Forgot to update partition_relid in reindex_index in the second patch. Fixed in attachment.

Show quoted text
21 июля 2024 г., в 01:49, Ilya Gladyshev <ilya.v.gladyshev@gmail.com> написал(а):
19 июля 2024 г., в 04:17, Michael Paquier <michael@paquier.xyz> <mailto:michael@paquier.xyz> написал(а):
On Fri, Jul 12, 2024 at 11:07:49PM +0100, Ilya Gladyshev wrote:
While working on CIC for partitioned tables [1], I noticed that REINDEX for
partitioned tables is not tracking keeping progress of partitioned tables,
so I'm creating a separate thread for this fix as suggested.This limitation is not a bug, because we already document that
partitions_total and partitions_done are 0 during a REINDEX. Compared
to CREATE INDEX, a REINDEX is a bit wilder as it would work on all the
indexes for all the partitions, providing this information makes
sense.Agreed that this could be better, but what's now on HEAD is not wrong
either.Thanks for pointing it out, I didn’t notice it. You’re right, this not a bug, but an improvement. Will remove the bits from the documentation as well.
Still it does not do it because we know that the fields are going to
be overwritten pretty quickly anyway, no? For now, we could just do
without progress_index_partition_done(), I guess, keeping it to the
incrementation of PROGRESS_CREATEIDX_PARTITIONS_DONE. There's an
argument that this makes the code slightly easier to follow, with less
wrappers around the progress reporting.The use-case that I thought this would improve is REINDEX CONCURRENT, where data from later stages of the previous partitions would linger for quite a while before it gets to the same stage of the current partition. I don’t think this is of big importance, so I’m ok with making code simpler and leaving it out.
+ int progress_params[3] = { + PROGRESS_CREATEIDX_COMMAND, + PROGRESS_CREATEIDX_PHASE, + PROGRESS_CREATEIDX_PARTITIONS_TOTAL + }; + int64 progress_values[3]; + Oid heapId = relid;Rather than having new code blocks, let's use a style consistent with
DefineIndex() where we have the pgstat_progress_update_multi_param(),
with a single {} block.Fixed.
Adding the counter increment at the end of the loop in
ReindexMultipleInternal() is a good idea. It considers both the
concurrent and non-concurrent cases.Another reason to do so is that reindex_relation calls itself recursively, so it'd require some additional mechanism so that it wouldn't increment multiple times per partition.
+ progress_values[2] = list_length(partitions); + pgstat_progress_start_command(PROGRESS_COMMAND_CREATE_INDEX, heapId);Hmm. Is setting the relid only once for pg_stat_progress_create_index
the best choice there is? Could it be better to report the partition
OID instead?Technically, we could do this, but it looks like no other commands do it and currently there’s no API to do it without erasing the rest of progress information.
Let's remember that indexes may have been attached with
names inconsistent with the partitioned table's index. It is a bit
confusing to stick to the relid all the partitioned table all the
time, for all the indexes of all the partitions reindexed.I agree that it’s a bit confusing especially because documentation gives no hints about it. Judging by documentation, I would expect relid and index_relid to correspond to the same table. However, if we say that this field represents the oid of the relation on which the command was invoked, then I think it does make sense. Edited documentation to say that in the new patch.
Could it
be better to actually introduce an entirely new field to the progress
table? What I would look for is more information:
1) the partitioned table OID on which the REINDEX runs
2) the partition table being processed
3) the index OID being processed (old and new for concurrent case).The patch sets 1) to the OID of the partitioned table, lacks 2) and
sets 3) each time an index is rebuilt.
—
MichaelI like this approach more, but I’m not sure whether adding another field for partition oid is worth it, since we already have index_relid that we can use to join with pg_index to get that. On the other hand, index_relid is missing during regular CREATE INDEX, so this new field could be useful to indicate which table is being indexed in this case. I'm on the fence about this, attached this as a separate patch, if you think it's a good idea.
Thank you for the review!
<v2-0002-partition_relid-column-for-create-index-progress.patch><v2-0001-make-REINDEX-track-partition-progress.patch>
Attachments:
v2-0001-make-REINDEX-track-partition-progress.patchapplication/octet-stream; name=v2-0001-make-REINDEX-track-partition-progress.patch; x-unix-mode=0644Download
From 5f3d4c5b4877b31bb95caa6086d1939aef4f3d12 Mon Sep 17 00:00:00 2001
From: Ilya Gladyshev <ilya.v.gladyshev@gmail.com>
Date: Sat, 20 Jul 2024 14:08:33 +0100
Subject: [PATCH v2 1/2] make REINDEX track partition progress
---
doc/src/sgml/monitoring.sgml | 8 ++----
src/backend/catalog/index.c | 11 ++++++--
src/backend/commands/indexcmds.c | 48 ++++++++++++++++++++++++++++----
src/include/catalog/index.h | 1 +
4 files changed, 55 insertions(+), 13 deletions(-)
diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml
index 55417a6fa9..00bb423288 100644
--- a/doc/src/sgml/monitoring.sgml
+++ b/doc/src/sgml/monitoring.sgml
@@ -5888,7 +5888,7 @@ FROM pg_stat_get_backend_idset() AS backendid;
<structfield>relid</structfield> <type>oid</type>
</para>
<para>
- OID of the table on which the index is being created.
+ OID of the table on which the command was run.
</para></entry>
</row>
@@ -5992,8 +5992,7 @@ FROM pg_stat_get_backend_idset() AS backendid;
<para>
Total number of partitions on which the index is to be created
or attached, including both direct and indirect partitions.
- <literal>0</literal> during a <literal>REINDEX</literal>, or when
- the index is not partitioned.
+ <literal>0</literal> when the index is not partitioned.
</para></entry>
</row>
@@ -6004,8 +6003,7 @@ FROM pg_stat_get_backend_idset() AS backendid;
<para>
Number of partitions on which the index has already been created
or attached, including both direct and indirect partitions.
- <literal>0</literal> during a <literal>REINDEX</literal>, or when
- the index is not partitioned.
+ <literal>0</literal> when the index is not partitioned.
</para></entry>
</row>
</tbody>
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index a819b4197c..90488f9140 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -3560,6 +3560,7 @@ reindex_index(const ReindexStmt *stmt, Oid indexId,
volatile bool skipped_constraint = false;
PGRUsage ru0;
bool progress = ((params->options & REINDEXOPT_REPORT_PROGRESS) != 0);
+ bool partition = ((params->options & REINDEXOPT_PARTITION) != 0);
bool set_tablespace = false;
pg_rusage_init(&ru0);
@@ -3605,8 +3606,9 @@ reindex_index(const ReindexStmt *stmt, Oid indexId,
indexId
};
- pgstat_progress_start_command(PROGRESS_COMMAND_CREATE_INDEX,
- heapId);
+ if (!partition)
+ pgstat_progress_start_command(PROGRESS_COMMAND_CREATE_INDEX,
+ heapId);
pgstat_progress_update_multi_param(2, progress_cols, progress_vals);
}
@@ -3846,8 +3848,11 @@ reindex_index(const ReindexStmt *stmt, Oid indexId,
index_close(iRel, NoLock);
table_close(heapRelation, NoLock);
- if (progress)
+ if (progress && !partition)
+ {
+ /* progress for partitions is tracked in the caller */
pgstat_progress_end_command();
+ }
}
/*
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index c5a56c75f6..d4c1811f50 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -3217,9 +3217,16 @@ ReindexPartitions(const ReindexStmt *stmt, Oid relid, const ReindexParams *param
ListCell *lc;
ErrorContextCallback errcallback;
ReindexErrorInfo errinfo;
+ ReindexParams newparams;
+ Oid heapId = relid;
Assert(RELKIND_HAS_PARTITIONS(relkind));
+ if (relkind == RELKIND_PARTITIONED_INDEX)
+ {
+ heapId = IndexGetRelation(relid, true);
+ }
+
/*
* Check if this runs in a transaction block, with an error callback to
* provide more context under which a problem happens.
@@ -3278,11 +3285,33 @@ ReindexPartitions(const ReindexStmt *stmt, Oid relid, const ReindexParams *param
MemoryContextSwitchTo(old_context);
}
+
+ {
+ int progress_params[3] = {
+ PROGRESS_CREATEIDX_COMMAND,
+ PROGRESS_CREATEIDX_PHASE,
+ PROGRESS_CREATEIDX_PARTITIONS_TOTAL
+ };
+ int64 progress_values[3];
+
+ progress_values[0] = (params->options & REINDEXOPT_CONCURRENTLY) != 0 ?
+ PROGRESS_CREATEIDX_COMMAND_REINDEX_CONCURRENTLY :
+ PROGRESS_CREATEIDX_COMMAND_REINDEX;
+ progress_values[1] = 0;
+ progress_values[2] = list_length(partitions);
+ pgstat_progress_start_command(PROGRESS_COMMAND_CREATE_INDEX, heapId);
+ pgstat_progress_update_multi_param(3, progress_params, progress_values);
+ }
+
/*
* Process each partition listed in a separate transaction. Note that
* this commits and then starts a new transaction immediately.
*/
- ReindexMultipleInternal(stmt, partitions, params);
+ newparams = *params;
+ newparams.options |= REINDEXOPT_PARTITION;
+ ReindexMultipleInternal(stmt, partitions, &newparams);
+
+ pgstat_progress_end_command();
/*
* Clean up working storage --- note we must do this after
@@ -3303,6 +3332,7 @@ static void
ReindexMultipleInternal(const ReindexStmt *stmt, const List *relids, const ReindexParams *params)
{
ListCell *l;
+ bool partitions = ((params->options & REINDEXOPT_PARTITION) != 0);
PopActiveSnapshot();
CommitTransactionCommand();
@@ -3396,6 +3426,9 @@ ReindexMultipleInternal(const ReindexStmt *stmt, const List *relids, const Reind
}
CommitTransactionCommand();
+
+ if (partitions)
+ pgstat_progress_incr_param(PROGRESS_CREATEIDX_PARTITIONS_DONE, 1);
}
StartTransactionCommand();
@@ -3448,6 +3481,7 @@ ReindexRelationConcurrently(const ReindexStmt *stmt, Oid relationOid, const Rein
char *relationName = NULL;
char *relationNamespace = NULL;
PGRUsage ru0;
+ bool partition = ((params->options & REINDEXOPT_PARTITION) != 0);
const int progress_index[] = {
PROGRESS_CREATEIDX_COMMAND,
PROGRESS_CREATEIDX_PHASE,
@@ -3791,7 +3825,8 @@ ReindexRelationConcurrently(const ReindexStmt *stmt, Oid relationOid, const Rein
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 (!partition)
+ pgstat_progress_start_command(PROGRESS_COMMAND_CREATE_INDEX, idx->tableId);
progress_vals[0] = PROGRESS_CREATEIDX_COMMAND_REINDEX_CONCURRENTLY;
progress_vals[1] = 0; /* initializing */
@@ -3966,7 +4001,8 @@ ReindexRelationConcurrently(const ReindexStmt *stmt, Oid relationOid, const Rein
* Update progress for the index to build, with the correct parent
* table involved.
*/
- pgstat_progress_start_command(PROGRESS_COMMAND_CREATE_INDEX, newidx->tableId);
+ if (!partition)
+ 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;
@@ -4030,7 +4066,8 @@ ReindexRelationConcurrently(const ReindexStmt *stmt, Oid relationOid, const Rein
* Update progress for the index to build, with the correct parent
* table involved.
*/
- pgstat_progress_start_command(PROGRESS_COMMAND_CREATE_INDEX, newidx->tableId);
+ if (!partition)
+ 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;
@@ -4265,7 +4302,8 @@ ReindexRelationConcurrently(const ReindexStmt *stmt, Oid relationOid, const Rein
MemoryContextDelete(private_context);
- pgstat_progress_end_command();
+ if (!partition)
+ pgstat_progress_end_command();
return true;
}
diff --git a/src/include/catalog/index.h b/src/include/catalog/index.h
index 7d434f8e65..ccba65fbbf 100644
--- a/src/include/catalog/index.h
+++ b/src/include/catalog/index.h
@@ -42,6 +42,7 @@ typedef struct ReindexParams
#define REINDEXOPT_REPORT_PROGRESS 0x02 /* report pgstat progress */
#define REINDEXOPT_MISSING_OK 0x04 /* skip missing relations */
#define REINDEXOPT_CONCURRENTLY 0x08 /* concurrent mode */
+#define REINDEXOPT_PARTITION 0x10 /* reindexing is done as part of partitioned table/index reindex */
/* state info for validate_index bulkdelete callback */
typedef struct ValidateIndexState
--
2.39.3 (Apple Git-146)
v2-0002-partition_relid-column-for-create-index-progress.patchapplication/octet-stream; name=v2-0002-partition_relid-column-for-create-index-progress.patch; x-unix-mode=0644Download
From de67cad872046ae5bae6e3a0a6033594b9d1ee86 Mon Sep 17 00:00:00 2001
From: Ilya Gladyshev <ilya.v.gladyshev@gmail.com>
Date: Sun, 21 Jul 2024 01:40:15 +0100
Subject: [PATCH v2 2/2] partition_relid column for create index progress
---
doc/src/sgml/monitoring.sgml | 9 +++++++++
src/backend/catalog/index.c | 8 +++++---
src/backend/catalog/system_views.sql | 3 ++-
src/backend/commands/indexcmds.c | 18 +++++++++++++-----
src/include/commands/progress.h | 1 +
src/test/regress/expected/rules.out | 3 ++-
6 files changed, 32 insertions(+), 10 deletions(-)
diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml
index 00bb423288..595f19a8f8 100644
--- a/doc/src/sgml/monitoring.sgml
+++ b/doc/src/sgml/monitoring.sgml
@@ -6006,6 +6006,15 @@ FROM pg_stat_get_backend_idset() AS backendid;
<literal>0</literal> when the index is not partitioned.
</para></entry>
</row>
+ <row>
+ <entry role="catalog_table_entry"><para role="column_definition">
+ <structfield>partition_relid</structfield> <type>bigint</type>
+ </para>
+ <para>
+ OID of the partition on which the index is currently being created.
+ <literal>0</literal> when the index is not partitioned.
+ </para></entry>
+ </row>
</tbody>
</tgroup>
</table>
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index 90488f9140..6789e61f98 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -3599,17 +3599,19 @@ reindex_index(const ReindexStmt *stmt, Oid indexId,
{
const int progress_cols[] = {
PROGRESS_CREATEIDX_COMMAND,
- PROGRESS_CREATEIDX_INDEX_OID
+ PROGRESS_CREATEIDX_INDEX_OID,
+ PROGRESS_CREATEIDX_PARTITION_RELID
};
const int64 progress_vals[] = {
PROGRESS_CREATEIDX_COMMAND_REINDEX,
- indexId
+ indexId,
+ partition ? heapId : 0
};
if (!partition)
pgstat_progress_start_command(PROGRESS_COMMAND_CREATE_INDEX,
heapId);
- pgstat_progress_update_multi_param(2, progress_cols, progress_vals);
+ pgstat_progress_update_multi_param(3, progress_cols, progress_vals);
}
/*
diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql
index 19cabc9a47..00b7f3bda8 100644
--- a/src/backend/catalog/system_views.sql
+++ b/src/backend/catalog/system_views.sql
@@ -1280,7 +1280,8 @@ CREATE VIEW pg_stat_progress_create_index AS
S.param12 AS tuples_total,
S.param13 AS tuples_done,
S.param14 AS partitions_total,
- S.param15 AS partitions_done
+ S.param15 AS partitions_done,
+ S.param18 AS partition_relid
FROM pg_stat_get_progress_info('CREATE INDEX') AS S
LEFT JOIN pg_database D ON S.datid = D.oid;
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index d4c1811f50..284f7a919a 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -617,6 +617,10 @@ DefineIndex(Oid tableId,
PROGRESS_CREATEIDX_COMMAND_CREATE_CONCURRENTLY :
PROGRESS_CREATEIDX_COMMAND_CREATE);
}
+ else
+ {
+ pgstat_progress_update_param(PROGRESS_CREATEIDX_PARTITION_RELID, tableId);
+ }
/*
* No index OID to report yet
@@ -3486,9 +3490,10 @@ ReindexRelationConcurrently(const ReindexStmt *stmt, Oid relationOid, const Rein
PROGRESS_CREATEIDX_COMMAND,
PROGRESS_CREATEIDX_PHASE,
PROGRESS_CREATEIDX_INDEX_OID,
- PROGRESS_CREATEIDX_ACCESS_METHOD_OID
+ PROGRESS_CREATEIDX_ACCESS_METHOD_OID,
+ PROGRESS_CREATEIDX_PARTITION_RELID
};
- int64 progress_vals[4];
+ int64 progress_vals[5];
/*
* Create a memory context that will survive forced transaction commits we
@@ -3832,7 +3837,8 @@ ReindexRelationConcurrently(const ReindexStmt *stmt, Oid relationOid, const Rein
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[4] = partition ? idx->tableId : 0;
+ pgstat_progress_update_multi_param(5, progress_index, progress_vals);
/* Choose a temporary relation name for the new index */
concurrentName = ChooseRelationName(get_rel_name(idx->indexId),
@@ -4007,7 +4013,8 @@ ReindexRelationConcurrently(const ReindexStmt *stmt, Oid relationOid, const Rein
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[4] = partition ? newidx->tableId : 0;
+ pgstat_progress_update_multi_param(5, progress_index, progress_vals);
/* Perform concurrent build of new index */
index_concurrently_build(newidx->tableId, newidx->indexId);
@@ -4072,7 +4079,8 @@ ReindexRelationConcurrently(const ReindexStmt *stmt, Oid relationOid, const Rein
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[4] = partition ? newidx->tableId : 0;
+ pgstat_progress_update_multi_param(5, progress_index, progress_vals);
validate_index(newidx->tableId, newidx->indexId, snapshot);
diff --git a/src/include/commands/progress.h b/src/include/commands/progress.h
index 5616d64523..6cd8b41a4c 100644
--- a/src/include/commands/progress.h
+++ b/src/include/commands/progress.h
@@ -89,6 +89,7 @@
#define PROGRESS_CREATEIDX_PARTITIONS_TOTAL 13
#define PROGRESS_CREATEIDX_PARTITIONS_DONE 14
/* 15 and 16 reserved for "block number" metrics */
+#define PROGRESS_CREATEIDX_PARTITION_RELID 17
/* Phases of CREATE INDEX (as advertised via PROGRESS_CREATEIDX_PHASE) */
#define PROGRESS_CREATEIDX_PHASE_WAIT_1 1
diff --git a/src/test/regress/expected/rules.out b/src/test/regress/expected/rules.out
index 4c789279e5..2bcb72c014 100644
--- a/src/test/regress/expected/rules.out
+++ b/src/test/regress/expected/rules.out
@@ -2027,7 +2027,8 @@ pg_stat_progress_create_index| SELECT s.pid,
s.param12 AS tuples_total,
s.param13 AS tuples_done,
s.param14 AS partitions_total,
- s.param15 AS partitions_done
+ s.param15 AS partitions_done,
+ s.param18 AS partition_relid
FROM (pg_stat_get_progress_info('CREATE INDEX'::text) s(pid, datid, relid, param1, param2, param3, param4, param5, param6, param7, param8, param9, param10, param11, param12, param13, param14, param15, param16, param17, param18, param19, param20)
LEFT JOIN pg_database d ON ((s.datid = d.oid)));
pg_stat_progress_vacuum| SELECT s.pid,
--
2.39.3 (Apple Git-146)
On Sun, Jul 21, 2024 at 11:41:43AM +0100, Ilya Gladyshev wrote:
Forgot to update partition_relid in reindex_index in the second patch. Fixed in attachment.
<structfield>relid</structfield> <type>oid</type>
</para>
<para>
- OID of the table on which the index is being created.
+ OID of the table on which the command was run.
</para></entry>Hmm. I am not sure if we really need to change the definition of this
field, because it can have the same meaning when using a REINDEX on a
partitioned table, pointing to the parent table (the partition) of the
index currently rebuilt.
Hence, rather than a partition_relid, could a partitioned_relid
reflect better the situation, set only when issuing a REINDEX on a
partitioned relation?
+ if (relkind == RELKIND_PARTITIONED_INDEX)
+ {
+ heapId = IndexGetRelation(relid, true);
+ }
Shouldn't we report the partitioned index OID rather than its parent
table when running a REINDEX on a partitioned index?
--
Michael25 июля 2024 г., в 09:55, Michael Paquier <michael@paquier.xyz>
написал(а):On Sun, Jul 21, 2024 at 11:41:43AM +0100, Ilya Gladyshev wrote:
Forgot to update partition_relid in reindex_index in the second
patch. Fixed in attachment.<structfield>relid</structfield> <type>oid</type> </para> <para> - OID of the table on which the index is being created. + OID of the table on which the command was run. </para></entry>Hmm. I am not sure if we really need to change the definition of this
field, because it can have the same meaning when using a REINDEX on a
partitioned table, pointing to the parent table (the partition) of the
index currently rebuilt.Hence, rather than a partition_relid, could a partitioned_relid
reflect better the situation, set only when issuing a REINDEX on a
partitioned relation?
I'm not quite happy with the documentation update, but I think the
approach for partitioned tables in this patch makes sense. I checked
what other commands, that deal with partitions, (CREATE INDEX and
ANALYZE) do, and they put a root partitioned table in "relid". ANALYZE
has a separate column for the id of partition named
current_child_table_relid, so I think it makes sense to have REINDEX do
the same.
In addition, the current API for progress tracking doesn't have a way of
updating "relid" without wiping out all other fields (that's what
pgstat_progress_start_command does). This can definitely be changed, but
that's another thing that made me not think in this direction.
+ if (relkind == RELKIND_PARTITIONED_INDEX) + { + heapId = IndexGetRelation(relid, true); + } Shouldn't we report the partitioned index OID rather than its parent table when running a REINDEX on a partitioned index? — Michael
It’s used to update the "relid" field of the progress report. It’s the
one that’s described in docs currently as "OID of the table on which the
index is being created.", so I think it’s correct.