Re: CREATE INDEX CONCURRENTLY on partitioned index
On Fri, Jul 12, 2024 at 11:17:25PM +0100, Ilya Gladyshev wrote: > Sure, created a separate thread [1]. Please disregard the second patch in > this thread. Duplicating the last version of the relevant patch here to > avoid any confusion. > > [1] > https://www.postgresql.org/message-id/b72f2d89-820a-4fa2-9058-b155cf646f4f%40gmail.com Thanks, will check that. -- Michael signature.asc Description: PGP signature
Re: CREATE INDEX CONCURRENTLY on partitioned index
On 12.07.2024 01:01, Michael Paquier wrote: Please let's move this point to its own thread and deal with it with an independent patch. Hiding that in a thread that's already quite long is not a good idea. This needs proper review, and a separate thread with a good subject to describe the problem will attract a better audience to deal with the problem you are seeing. I was not paying much attention, until you've mentioned that this was an issue with HEAD. -- Michael Sure, created a separate thread [1]. Please disregard the second patch in this thread. Duplicating the last version of the relevant patch here to avoid any confusion. [1] https://www.postgresql.org/message-id/b72f2d89-820a-4fa2-9058-b155cf646f4f%40gmail.com From acf5cf5d4a984c0f8635a25e03c23409601c0c93 Mon Sep 17 00:00:00 2001 From: Ilya Gladyshev Date: Thu, 23 May 2024 18:13:41 +0100 Subject: [PATCH v5 1/2] Allow CREATE INDEX CONCURRENTLY on partitioned table --- doc/src/sgml/ddl.sgml | 10 +- doc/src/sgml/ref/create_index.sgml| 14 +- src/backend/commands/indexcmds.c | 228 +- .../isolation/expected/partitioned-cic.out| 135 +++ src/test/isolation/isolation_schedule | 1 + src/test/isolation/specs/partitioned-cic.spec | 57 + src/test/regress/expected/indexing.out| 127 +- src/test/regress/sql/indexing.sql | 26 +- 8 files changed, 520 insertions(+), 78 deletions(-) create mode 100644 src/test/isolation/expected/partitioned-cic.out create mode 100644 src/test/isolation/specs/partitioned-cic.spec diff --git a/doc/src/sgml/ddl.sgml b/doc/src/sgml/ddl.sgml index 6aab79e901..904978c6e5 100644 --- a/doc/src/sgml/ddl.sgml +++ b/doc/src/sgml/ddl.sgml @@ -4314,14 +4314,12 @@ ALTER TABLE measurement ATTACH PARTITION measurement_y2008m02 As mentioned earlier, it is possible to create indexes on partitioned tables so that they are applied automatically to the entire hierarchy. This can be very convenient as not only will all existing partitions be - indexed, but any future partitions will be as well. However, one - limitation when creating new indexes on partitioned tables is that it - is not possible to use the CONCURRENTLY - qualifier, which could lead to long lock times. To avoid this, you can - use CREATE INDEX ON ONLY the partitioned table, which + indexed, but any future partitions will be as well. For more control over + locking of the partitions you can use CREATE INDEX ON ONLY + on the partitioned table, which creates the new index marked as invalid, preventing automatic application to existing partitions. Instead, indexes can then be created individually - on each partition using CONCURRENTLY and + on each partition and attached to the partitioned index on the parent using ALTER INDEX ... ATTACH PARTITION. Once indexes for all the partitions are attached to the parent index, the parent index will diff --git a/doc/src/sgml/ref/create_index.sgml b/doc/src/sgml/ref/create_index.sgml index 621bc0e253..2366cfd9b5 100644 --- a/doc/src/sgml/ref/create_index.sgml +++ b/doc/src/sgml/ref/create_index.sgml @@ -645,7 +645,10 @@ CREATE [ UNIQUE ] INDEX [ CONCURRENTLY ] [ [ IF NOT EXISTS ] If a problem arises while scanning the table, such as a deadlock or a uniqueness violation in a unique index, the CREATE INDEX -command will fail but leave behind an invalid index. This index +command will fail but leave behind an invalid index. +If this happens while build an index concurrently on a partitioned +table, the command can also leave behind valid or +invalid indexes on table partitions. The invalid index will be ignored for querying purposes because it might be incomplete; however it will still consume update overhead. The psql \d command will report such an index as INVALID: @@ -692,15 +695,6 @@ Indexes: cannot. - -Concurrent builds for indexes on partitioned tables are currently not -supported. However, you may concurrently build the index on each -partition individually and then finally create the partitioned index -non-concurrently in order to reduce the time where writes to the -partitioned table will be locked out. In this case, building the -partitioned index is a metadata only operation. - - diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c index 309389e20d..dcb4ea89e9 100644 --- a/src/backend/commands/indexcmds.c +++ b/src/backend/commands/indexcmds.c @@ -95,6 +95,11 @@ static char *ChooseIndexName(const char *tabname, Oid namespaceId, bool primary, bool isconstraint); static char *ChooseIndexNameAddition(const List *colnames); static List *ChooseIndexColumnNames(const List *indexElems); +static void DefineIndexConcurrentInternal(Oid relationId, + Oid indexRelationId, +
Re: CREATE INDEX CONCURRENTLY on partitioned index
On Thu, Jul 11, 2024 at 09:35:24PM +0100, Ilya Gladyshev wrote: > It is broken in master, I just didn’t want to create a separate > thread, but it can be fixed independently. As I remember, the > problem is that progress is tracked for each table in the hierarchy > as if the table is processed separately, without ever setting > partitions_total and partitions_done counters. Please let's move this point to its own thread and deal with it with an independent patch. Hiding that in a thread that's already quite long is not a good idea. This needs proper review, and a separate thread with a good subject to describe the problem will attract a better audience to deal with the problem you are seeing. I was not paying much attention, until you've mentioned that this was an issue with HEAD. -- Michael signature.asc Description: PGP signature
Re: CREATE INDEX CONCURRENTLY on partitioned index
It is broken in master, I just didn’t want to create a separate thread, but it can be fixed independently. As I remember, the problem is that progress is tracked for each table in the hierarchy as if the table is processed separately, without ever setting partitions_total and partitions_done counters. > 11 июля 2024 г., в 13:31, Justin Pryzby написал(а): > > On Sat, Jun 15, 2024 at 07:56:38PM +0100, Ilya Gladyshev wrote: >> In addition, I noticed that progress tracking is once again broken for >> partitioned tables, while looking at REINDEX implementation, attaching the >> second patch to fix it. > > Thanks for the fixes, I started reviewing them but need some more time > to digest. > > Do you mean that progress reporting is broken in master, for REINDEX, or > just with this patch ? > > -- > Justin
Re: CREATE INDEX CONCURRENTLY on partitioned index
On Sat, Jun 15, 2024 at 07:56:38PM +0100, Ilya Gladyshev wrote: > In addition, I noticed that progress tracking is once again broken for > partitioned tables, while looking at REINDEX implementation, attaching the > second patch to fix it. Thanks for the fixes, I started reviewing them but need some more time to digest. Do you mean that progress reporting is broken in master, for REINDEX, or just with this patch ? -- Justin
Re: CREATE INDEX CONCURRENTLY on partitioned index
On 15.06.2024 20:40, Justin Pryzby wrote:
On Thu, May 23, 2024 at 10:14:57PM +0100, Ilya Gladyshev wrote:
Hi,
I think it's well worth the effort to revive the patch, so I rebased it on
master, updated it and will return it back to the commitfest. Alexander,
Justin feel free to add yourselves as authors
Thanks -- I was intending to write about this.
I realized that the patch will need some isolation tests to exercise its
concurrent behavior.
Thanks for the suggestion, added an isolation test that verifies
behaviour of partitioned CIC with simultaneous partition drop/detach
going on. Also fixed some issues in the new patch that I found while
writing the test.
From 45f2ec9ee57a5337b77b66db3c8c5092f305a176 Mon Sep 17 00:00:00 2001
From: Ilya Gladyshev
Date: Tue, 11 Jun 2024 17:48:08 +0100
Subject: [PATCH v5 2/2] Fix progress report for partitioned REINDEX
---
src/backend/catalog/index.c | 11 --
src/backend/commands/indexcmds.c | 63 +---
src/include/catalog/index.h | 1 +
3 files changed, 66 insertions(+), 9 deletions(-)
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index 55fdde4b24..c5bc72b350 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -3559,6 +3559,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);
@@ -3604,8 +3605,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);
}
@@ -3845,8 +3847,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 dcb4ea89e9..6abe1f017c 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -118,6 +118,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()
@@ -1550,7 +1551,7 @@ DefineIndex(Oid tableId,
* Update progress for an intermediate partitioned index
* itself
*/
-pgstat_progress_incr_param(PROGRESS_CREATEIDX_PARTITIONS_DONE, 1);
+progress_index_partition_done();
}
return address;
@@ -1577,7 +1578,7 @@ DefineIndex(Oid tableId,
if (!OidIsValid(parentIndexId))
pgstat_progress_end_command();
else if (!concurrent)
- pgstat_progress_incr_param(PROGRESS_CREATEIDX_PARTITIONS_DONE, 1);
+ progress_index_partition_done();
return address;
}
@@ -1686,7 +1687,7 @@ DefineIndex(Oid tableId,
heaplocktag, heaprelid);
PushActiveSnapshot(GetTransactionSnapshot());
- pgstat_progress_incr_param(PROGRESS_CREATEIDX_PARTITIONS_DONE, 1);
+ progress_index_partition_done();
}
/* Set as valid all partitioned indexes, including the parent */
@@ -3331,6 +3332,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));
@@ -3392,11 +3401,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 = *
Re: CREATE INDEX CONCURRENTLY on partitioned index
On Thu, May 23, 2024 at 10:14:57PM +0100, Ilya Gladyshev wrote: > Hi, > > I think it's well worth the effort to revive the patch, so I rebased it on > master, updated it and will return it back to the commitfest. Alexander, > Justin feel free to add yourselves as authors Thanks -- I was intending to write about this. I realized that the patch will need some isolation tests to exercise its concurrent behavior. -- Justin
Re: CREATE INDEX CONCURRENTLY on partitioned index
On 28.05.2024 07:05, Alexander Pyhalov wrote:
Ilya Gladyshev писал(а) 2024-05-28 02:52:
Also I'd like to note that in new patch version there's a strange
wording in documentation:
"This can be very convenient as not only will all existing
partitions be
indexed, but any future partitions will be as well.
CREATE INDEX ... CONCURRENTLY can incur long
lock times
on huge partitioned tables, to avoid that you can
use CREATE INDEX ON ONLY the partitioned table,
which
creates the new index marked as invalid, preventing automatic
application
to existing partitions."
All the point of CIC is to avoid long lock times. So it seems this
paragraph should be rewritten in the following way:
"To avoid long lock times, you can use CREATE INDEX CONCURRENTLY or
CREATE INDEX ON ONLY the partitioned table..."
True, the current wording doesn't look right. Right now CREATE INDEX
ON ONLY is described as a workaround for the missing CIC. I think it
rather makes sense to say that it gives more fine-grained control of
partition locking than both CIC and ordinary CREATE INDEX. See the
updated patch.
Hi.
Not sure if it's worth removing mentioning of CIC in
creates the new index marked as invalid, preventing automatic
application
to existing partitions. Instead, indexes can then be created
individually
- on each partition using CONCURRENTLY and
+ on each partition and
attached to the partitioned index on the
parent
using ALTER INDEX ... ATTACH PARTITION. Once
indexes for
all the partitions are attached to the parent index, the parent
index will
but at least now it looks better.
The current patch version locks all the partitions in the first
transaction up until each of them is built, which makes for long lock
times for partitions that are built last. Having looked at the
implementation of REINDEX CONCURRENTLY for partitioned tables, I think
we can improve this by using the same approach of just skipping the
relations that we find out are dropped when trying to lock them.
Incidentally, this implementation in the new patch version is also simpler.
In addition, I noticed that progress tracking is once again broken for
partitioned tables, while looking at REINDEX implementation, attaching
the second patch to fix it.
From 884be03aaeabee5c6eeb5a3f639ac9afe712c24b Mon Sep 17 00:00:00 2001
From: Ilya Gladyshev
Date: Tue, 11 Jun 2024 17:48:08 +0100
Subject: [PATCH v4 2/2] Fix progress report for partitioned REINDEX
---
src/backend/catalog/index.c | 11 --
src/backend/commands/indexcmds.c | 63 +---
src/include/catalog/index.h | 1 +
3 files changed, 66 insertions(+), 9 deletions(-)
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index 55fdde4b24..c5bc72b350 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -3559,6 +3559,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);
@@ -3604,8 +3605,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);
}
@@ -3845,8 +3847,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 5da2df2d3b..17b30ad6aa 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -118,6 +118,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()
@@ -1550,7 +1551,7 @@ DefineIndex(Oid tableId,
* Update progress for an intermediate partitioned index
* itself
*/
-pgstat_progress_incr_param(PROGRESS_CREATEIDX_PARTITIONS_DONE, 1);
+progress_index_partition_done();
}
return address;
@@ -1577,7 +1578,7 @@ DefineIndex(Oid tableId,
if (!OidIsValid(parentIndexId))
pgstat_progress_end_command();
else if (!concurrent)
- pgstat_progress_incr_param(PROGRESS_CREATEIDX_PARTITIONS_DONE, 1);
+ progress_index_partition_done();
retu
Re: CREATE INDEX CONCURRENTLY on partitioned index
Ilya Gladyshev писал(а) 2024-05-28 02:52: Also I'd like to note that in new patch version there's a strange wording in documentation: "This can be very convenient as not only will all existing partitions be indexed, but any future partitions will be as well. CREATE INDEX ... CONCURRENTLY can incur long lock times on huge partitioned tables, to avoid that you can use CREATE INDEX ON ONLY the partitioned table, which creates the new index marked as invalid, preventing automatic application to existing partitions." All the point of CIC is to avoid long lock times. So it seems this paragraph should be rewritten in the following way: "To avoid long lock times, you can use CREATE INDEX CONCURRENTLY or CREATE INDEX ON ONLY the partitioned table..." True, the current wording doesn't look right. Right now CREATE INDEX ON ONLY is described as a workaround for the missing CIC. I think it rather makes sense to say that it gives more fine-grained control of partition locking than both CIC and ordinary CREATE INDEX. See the updated patch. Hi. Not sure if it's worth removing mentioning of CIC in creates the new index marked as invalid, preventing automatic application to existing partitions. Instead, indexes can then be created individually - on each partition using CONCURRENTLY and + on each partition and attached to the partitioned index on the parent using ALTER INDEX ... ATTACH PARTITION. Once indexes for all the partitions are attached to the parent index, the parent index will but at least now it looks better. -- Best regards, Alexander Pyhalov, Postgres Professional
Re: CREATE INDEX CONCURRENTLY on partitioned index
On 24.05.2024 10:04, Alexander Pyhalov wrote: Ilya Gladyshev писал(а) 2024-05-24 00:14: Hi, Hi. I think it's well worth the effort to revive the patch, so I rebased it on master, updated it and will return it back to the commitfest. Alexander, Justin feel free to add yourselves as authors On 29.01.2024 12:43, Alexander Pyhalov wrote: Hi. I've rebased patch on master and it'seems to me there's one more issue - when we call DefineIndexConcurrentInternal() in partitioned case, it waits for transactions, locking tableId, not tabrelid - heaprelid LockRelId is constructed for parent index relation, not for child index relation. Attaching fixed version. Also I'm not sure what to do with locking of child relations. If we don't do anything, you can drop one of the partitioned table childs while CIC is in progress, and get error ERROR: cache lookup failed for index 16399 I agree that we need to do something about it, in particular, I think we should lock all the partitions inside the transaction that builds the catalog entries. Fixed this in the new version. If you try to lock all child tables in CIC session, you'll get deadlocks. Do you mean the deadlock between the transaction that drops a partition and the transaction doing CIC? I think this is unavoidable and can be reproduced even without partitioning. Yes, it seems we trade this error for possible deadlock between transaction, dropping a partition, and CIC. Also not sure why a list of children relation was obtained with ShareLock that CIC is supposed to avoid not to block writes, changed that to ShareUpdateExclusive. I expect that it wasn't an issue due to the fact that it's held for a brief period until DefineIndexConcurrentInternal() commits for the first time. But it seems, it's more correct to use ShareUpdateExclusive lock here. Also I'd like to note that in new patch version there's a strange wording in documentation: "This can be very convenient as not only will all existing partitions be indexed, but any future partitions will be as well. CREATE INDEX ... CONCURRENTLY can incur long lock times on huge partitioned tables, to avoid that you can use CREATE INDEX ON ONLY the partitioned table, which creates the new index marked as invalid, preventing automatic application to existing partitions." All the point of CIC is to avoid long lock times. So it seems this paragraph should be rewritten in the following way: "To avoid long lock times, you can use CREATE INDEX CONCURRENTLY or CREATE INDEX ON ONLY the partitioned table..." True, the current wording doesn't look right. Right now CREATE INDEX ON ONLY is described as a workaround for the missing CIC. I think it rather makes sense to say that it gives more fine-grained control of partition locking than both CIC and ordinary CREATE INDEX. See the updated patch. From 7687b4c3ba10fc1df5ca4c8f50198dfb269be8bb Mon Sep 17 00:00:00 2001 From: Ilya Gladyshev Date: Thu, 23 May 2024 18:13:41 +0100 Subject: [PATCH v3] Allow CREATE INDEX CONCURRENTLY on partitioned table --- doc/src/sgml/ddl.sgml | 10 +- doc/src/sgml/ref/create_index.sgml | 14 +- src/backend/commands/indexcmds.c | 293 ++--- src/test/regress/expected/indexing.out | 127 ++- src/test/regress/sql/indexing.sql | 26 ++- 5 files changed, 368 insertions(+), 102 deletions(-) diff --git a/doc/src/sgml/ddl.sgml b/doc/src/sgml/ddl.sgml index 6aab79e901..904978c6e5 100644 --- a/doc/src/sgml/ddl.sgml +++ b/doc/src/sgml/ddl.sgml @@ -4314,14 +4314,12 @@ ALTER TABLE measurement ATTACH PARTITION measurement_y2008m02 As mentioned earlier, it is possible to create indexes on partitioned tables so that they are applied automatically to the entire hierarchy. This can be very convenient as not only will all existing partitions be - indexed, but any future partitions will be as well. However, one - limitation when creating new indexes on partitioned tables is that it - is not possible to use the CONCURRENTLY - qualifier, which could lead to long lock times. To avoid this, you can - use CREATE INDEX ON ONLY the partitioned table, which + indexed, but any future partitions will be as well. For more control over + locking of the partitions you can use CREATE INDEX ON ONLY + on the partitioned table, which creates the new index marked as invalid, preventing automatic application to existing partitions. Instead, indexes can then be created individually - on each partition using CONCURRENTLY and + on each partition and attached to the partitioned index on the parent using ALTER INDEX ... ATTACH PARTITION. Once indexes for all the partitions are attached to the parent index, the parent index will diff --git a/doc/src/sgml/ref/create_index.sgml b/doc/src/sgml/ref/create_index.sgml index 621bc0e253..2366cfd9b5 100644 --- a/doc/src/sgml/ref/create_
Re: CREATE INDEX CONCURRENTLY on partitioned index
Ilya Gladyshev писал(а) 2024-05-24 00:14: Hi, Hi. I think it's well worth the effort to revive the patch, so I rebased it on master, updated it and will return it back to the commitfest. Alexander, Justin feel free to add yourselves as authors On 29.01.2024 12:43, Alexander Pyhalov wrote: Hi. I've rebased patch on master and it'seems to me there's one more issue - when we call DefineIndexConcurrentInternal() in partitioned case, it waits for transactions, locking tableId, not tabrelid - heaprelid LockRelId is constructed for parent index relation, not for child index relation. Attaching fixed version. Also I'm not sure what to do with locking of child relations. If we don't do anything, you can drop one of the partitioned table childs while CIC is in progress, and get error ERROR: cache lookup failed for index 16399 I agree that we need to do something about it, in particular, I think we should lock all the partitions inside the transaction that builds the catalog entries. Fixed this in the new version. If you try to lock all child tables in CIC session, you'll get deadlocks. Do you mean the deadlock between the transaction that drops a partition and the transaction doing CIC? I think this is unavoidable and can be reproduced even without partitioning. Yes, it seems we trade this error for possible deadlock between transaction, dropping a partition, and CIC. Also not sure why a list of children relation was obtained with ShareLock that CIC is supposed to avoid not to block writes, changed that to ShareUpdateExclusive. I expect that it wasn't an issue due to the fact that it's held for a brief period until DefineIndexConcurrentInternal() commits for the first time. But it seems, it's more correct to use ShareUpdateExclusive lock here. Also I'd like to note that in new patch version there's a strange wording in documentation: "This can be very convenient as not only will all existing partitions be indexed, but any future partitions will be as well. CREATE INDEX ... CONCURRENTLY can incur long lock times on huge partitioned tables, to avoid that you can use CREATE INDEX ON ONLY the partitioned table, which creates the new index marked as invalid, preventing automatic application to existing partitions." All the point of CIC is to avoid long lock times. So it seems this paragraph should be rewritten in the following way: "To avoid long lock times, you can use CREATE INDEX CONCURRENTLY or CREATE INDEX ON ONLY the partitioned table..." -- Best regards, Alexander Pyhalov, Postgres Professional
Re: CREATE INDEX CONCURRENTLY on partitioned index
Hi, I think it's well worth the effort to revive the patch, so I rebased it on master, updated it and will return it back to the commitfest. Alexander, Justin feel free to add yourselves as authors On 29.01.2024 12:43, Alexander Pyhalov wrote: Hi. I've rebased patch on master and it'seems to me there's one more issue - when we call DefineIndexConcurrentInternal() in partitioned case, it waits for transactions, locking tableId, not tabrelid - heaprelid LockRelId is constructed for parent index relation, not for child index relation. Attaching fixed version. Also I'm not sure what to do with locking of child relations. If we don't do anything, you can drop one of the partitioned table childs while CIC is in progress, and get error ERROR: cache lookup failed for index 16399 I agree that we need to do something about it, in particular, I think we should lock all the partitions inside the transaction that builds the catalog entries. Fixed this in the new version. If you try to lock all child tables in CIC session, you'll get deadlocks. Do you mean the deadlock between the transaction that drops a partition and the transaction doing CIC? I think this is unavoidable and can be reproduced even without partitioning. Also not sure why a list of children relation was obtained with ShareLock that CIC is supposed to avoid not to block writes, changed that to ShareUpdateExclusive. Regards, Ilya From 1cb76c8c19b1c0549fbba70febc32017bc04c0a2 Mon Sep 17 00:00:00 2001 From: Ilya Gladyshev Date: Thu, 23 May 2024 18:13:41 +0100 Subject: [PATCH v2] Allow CREATE INDEX CONCURRENTLY on partitioned table --- doc/src/sgml/ddl.sgml | 7 +- doc/src/sgml/ref/create_index.sgml | 14 +- src/backend/commands/indexcmds.c | 293 ++--- src/test/regress/expected/indexing.out | 127 ++- src/test/regress/sql/indexing.sql | 26 ++- 5 files changed, 367 insertions(+), 100 deletions(-) diff --git a/doc/src/sgml/ddl.sgml b/doc/src/sgml/ddl.sgml index 6aab79e901..f1d4a59a99 100644 --- a/doc/src/sgml/ddl.sgml +++ b/doc/src/sgml/ddl.sgml @@ -4314,10 +4314,9 @@ ALTER TABLE measurement ATTACH PARTITION measurement_y2008m02 As mentioned earlier, it is possible to create indexes on partitioned tables so that they are applied automatically to the entire hierarchy. This can be very convenient as not only will all existing partitions be - indexed, but any future partitions will be as well. However, one - limitation when creating new indexes on partitioned tables is that it - is not possible to use the CONCURRENTLY - qualifier, which could lead to long lock times. To avoid this, you can + indexed, but any future partitions will be as well. + CREATE INDEX ... CONCURRENTLY can incur long lock times + on huge partitioned tables, to avoid that you can use CREATE INDEX ON ONLY the partitioned table, which creates the new index marked as invalid, preventing automatic application to existing partitions. Instead, indexes can then be created individually diff --git a/doc/src/sgml/ref/create_index.sgml b/doc/src/sgml/ref/create_index.sgml index 621bc0e253..2366cfd9b5 100644 --- a/doc/src/sgml/ref/create_index.sgml +++ b/doc/src/sgml/ref/create_index.sgml @@ -645,7 +645,10 @@ CREATE [ UNIQUE ] INDEX [ CONCURRENTLY ] [ [ IF NOT EXISTS ] If a problem arises while scanning the table, such as a deadlock or a uniqueness violation in a unique index, the CREATE INDEX -command will fail but leave behind an invalid index. This index +command will fail but leave behind an invalid index. +If this happens while build an index concurrently on a partitioned +table, the command can also leave behind valid or +invalid indexes on table partitions. The invalid index will be ignored for querying purposes because it might be incomplete; however it will still consume update overhead. The psql \d command will report such an index as INVALID: @@ -692,15 +695,6 @@ Indexes: cannot. - -Concurrent builds for indexes on partitioned tables are currently not -supported. However, you may concurrently build the index on each -partition individually and then finally create the partitioned index -non-concurrently in order to reduce the time where writes to the -partitioned table will be locked out. In this case, building the -partitioned index is a metadata only operation. - - diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c index 309389e20d..5806eeb8ef 100644 --- a/src/backend/commands/indexcmds.c +++ b/src/backend/commands/indexcmds.c @@ -95,6 +95,11 @@ static char *ChooseIndexName(const char *tabname, Oid namespaceId, bool primary, bool isconstraint); static char *ChooseIndexNameAddition(const List *colnames); static List *ChooseIndexColumnNames(const List *indexElems); +static void DefineInd
Re: CREATE INDEX CONCURRENTLY on partitioned index
Hi. I've rebased patch on master and it'seems to me there's one more issue - when we call DefineIndexConcurrentInternal() in partitioned case, it waits for transactions, locking tableId, not tabrelid - heaprelid LockRelId is constructed for parent index relation, not for child index relation. Attaching fixed version. Also I'm not sure what to do with locking of child relations. If we don't do anything, you can drop one of the partitioned table childs while CIC is in progress, and get error ERROR: cache lookup failed for index 16399 If you try to lock all child tables in CIC session, you'll get deadlocks. -- Best regards, Alexander Pyhalov, Postgres ProfessionalFrom 37a13b7fa1c3277b9d038b7a0c75399ff05b28a7 Mon Sep 17 00:00:00 2001 From: Justin Pryzby Date: Mon, 29 Jan 2024 10:41:01 +0300 Subject: [PATCH] Allow CREATE INDEX CONCURRENTLY on partitioned table --- doc/src/sgml/ddl.sgml | 4 +- doc/src/sgml/ref/create_index.sgml | 14 +- src/backend/commands/indexcmds.c | 200 ++--- src/test/regress/expected/indexing.out | 127 +++- src/test/regress/sql/indexing.sql | 26 +++- 5 files changed, 296 insertions(+), 75 deletions(-) diff --git a/doc/src/sgml/ddl.sgml b/doc/src/sgml/ddl.sgml index 075ff329912..8ee80c40e3b 100644 --- a/doc/src/sgml/ddl.sgml +++ b/doc/src/sgml/ddl.sgml @@ -4194,9 +4194,7 @@ ALTER TABLE measurement ATTACH PARTITION measurement_y2008m02 so that they are applied automatically to the entire hierarchy. This is very convenient, as not only will the existing partitions become indexed, but - also any partitions that are created in the future will. One limitation is - that it's not possible to use the CONCURRENTLY - qualifier when creating such a partitioned index. To avoid long lock + also any partitions that are created in the future will. To avoid long lock times, it is possible to use CREATE INDEX ON ONLY the partitioned table; such an index is marked invalid, and the partitions do not get the index applied automatically. The indexes on partitions can diff --git a/doc/src/sgml/ref/create_index.sgml b/doc/src/sgml/ref/create_index.sgml index 40986aa502f..b05102efdaf 100644 --- a/doc/src/sgml/ref/create_index.sgml +++ b/doc/src/sgml/ref/create_index.sgml @@ -645,7 +645,10 @@ CREATE [ UNIQUE ] INDEX [ CONCURRENTLY ] [ [ IF NOT EXISTS ] If a problem arises while scanning the table, such as a deadlock or a uniqueness violation in a unique index, the CREATE INDEX -command will fail but leave behind an invalid index. This index +command will fail but leave behind an invalid index. +If this happens while build an index concurrently on a partitioned +table, the command can also leave behind valid or +invalid indexes on table partitions. The invalid index will be ignored for querying purposes because it might be incomplete; however it will still consume update overhead. The psql \d command will report such an index as INVALID: @@ -692,15 +695,6 @@ Indexes: cannot. - -Concurrent builds for indexes on partitioned tables are currently not -supported. However, you may concurrently build the index on each -partition individually and then finally create the partitioned index -non-concurrently in order to reduce the time where writes to the -partitioned table will be locked out. In this case, building the -partitioned index is a metadata only operation. - - diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c index ab8b81b3020..65477aeb3a8 100644 --- a/src/backend/commands/indexcmds.c +++ b/src/backend/commands/indexcmds.c @@ -93,6 +93,11 @@ static char *ChooseIndexName(const char *tabname, Oid namespaceId, bool primary, bool isconstraint); static char *ChooseIndexNameAddition(const List *colnames); static List *ChooseIndexColumnNames(const List *indexElems); +static void DefineIndexConcurrentInternal(Oid relationId, + Oid indexRelationId, + IndexInfo *indexInfo, + LOCKTAG heaplocktag, + LockRelId heaprelid); static void ReindexIndex(const RangeVar *indexRelation, const ReindexParams *params, bool isTopLevel); static void RangeVarCallbackForReindexIndex(const RangeVar *relation, @@ -554,7 +559,6 @@ DefineIndex(Oid tableId, bool amissummarizing; amoptions_function amoptions; bool partitioned; - bool safe_index; Datum reloptions; int16 *coloptions; IndexInfo *indexInfo; @@ -562,12 +566,10 @@ DefineIndex(Oid tableId, bits16 constr_flags; int numberOfAttributes; int numberOfKeyAttributes; - TransactionId limitXmin; ObjectAddress address; LockRelId heaprelid; LOCKTAG heaplocktag; LOCKMODE lockmode; - Snapshot snapshot; Oid root_save_userid; int root_save_sec_context; int root_save_nestlevel; @@ -697,20 +699,6 @@ DefineIndex(Oid tableId, *
Re: CREATE INDEX CONCURRENTLY on partitioned index
Justin Pryzby писал 2023-07-13 05:27: On Mon, Mar 27, 2023 at 01:28:24PM +0300, Alexander Pyhalov wrote: Justin Pryzby писал 2023-03-26 17:51: > On Sun, Dec 04, 2022 at 01:09:35PM -0600, Justin Pryzby wrote: > > This currently handles partitions with a loop around the whole CIC > > implementation, which means that things like WaitForLockers() happen > > once for each index, the same as REINDEX CONCURRENTLY on a partitioned > > table. Contrast that with ReindexRelationConcurrently(), which handles > > all the indexes on a table in one pass by looping around indexes within > > each phase. > > Rebased over the progress reporting fix (27f5c712b). > > I added a list of (intermediate) partitioned tables, rather than looping > over the list of inheritors again, to save calling rel_get_relkind(). > > I think this patch is done. Overall looks good to me. However, I think that using 'partitioned' as list of partitioned index oids in DefineIndex() is a bit misleading - we've just used it as boolean, specifying if we are dealing with a partitioned relation. Right. This is also rebased on 8c852ba9a4 (Allow some exclusion constraints on partitions). Hi. I have some more question. In the following code (indexcmds.c:1640 and later) 1640 rel = table_open(relationId, ShareUpdateExclusiveLock); 1641 heaprelid = rel->rd_lockInfo.lockRelId; 1642 table_close(rel, ShareUpdateExclusiveLock); 1643 SET_LOCKTAG_RELATION(heaplocktag, heaprelid.dbId, heaprelid.relId); should we release ShareUpdateExclusiveLock before getting session lock in DefineIndexConcurrentInternal()? Also we unlock parent table there between reindexing childs in the end of DefineIndexConcurrentInternal(): 1875 /* 1876 * Last thing to do is release the session-level lock on the parent table. 1877 */ 1878 UnlockRelationIdForSession(&heaprelid, ShareUpdateExclusiveLock); 1879 } Is it safe? Shouldn't we hold session lock on the parent table while rebuilding child indexes? -- Best regards, Alexander Pyhalov, Postgres Professional
Re: CREATE INDEX CONCURRENTLY on partitioned index
On Mon, Mar 27, 2023 at 01:28:24PM +0300, Alexander Pyhalov wrote: > Justin Pryzby писал 2023-03-26 17:51: > > On Sun, Dec 04, 2022 at 01:09:35PM -0600, Justin Pryzby wrote: > > > This currently handles partitions with a loop around the whole CIC > > > implementation, which means that things like WaitForLockers() happen > > > once for each index, the same as REINDEX CONCURRENTLY on a partitioned > > > table. Contrast that with ReindexRelationConcurrently(), which handles > > > all the indexes on a table in one pass by looping around indexes within > > > each phase. > > > > Rebased over the progress reporting fix (27f5c712b). > > > > I added a list of (intermediate) partitioned tables, rather than looping > > over the list of inheritors again, to save calling rel_get_relkind(). > > > > I think this patch is done. > > Overall looks good to me. However, I think that using 'partitioned' as list > of partitioned index oids in DefineIndex() is a bit misleading - we've just > used it as boolean, specifying if we are dealing with a partitioned > relation. Right. This is also rebased on 8c852ba9a4 (Allow some exclusion constraints on partitions). -- Justin >From 3f60cbdd12b67115f86854ff60a4009028b8b99f Mon Sep 17 00:00:00 2001 From: Justin Pryzby Date: Sat, 6 Jun 2020 17:42:23 -0500 Subject: [PATCH] Allow CREATE INDEX CONCURRENTLY on partitioned table https://www.postgresql.org/message-id/flat/20201031063117.gf3...@telsasoft.com --- doc/src/sgml/ddl.sgml | 4 +- doc/src/sgml/ref/create_index.sgml | 14 +- src/backend/commands/indexcmds.c | 201 ++--- src/test/regress/expected/indexing.out | 127 +++- src/test/regress/sql/indexing.sql | 26 +++- 5 files changed, 297 insertions(+), 75 deletions(-) diff --git a/doc/src/sgml/ddl.sgml b/doc/src/sgml/ddl.sgml index 58aaa691c6a..afa982154a8 100644 --- a/doc/src/sgml/ddl.sgml +++ b/doc/src/sgml/ddl.sgml @@ -4161,9 +4161,7 @@ ALTER TABLE measurement ATTACH PARTITION measurement_y2008m02 so that they are applied automatically to the entire hierarchy. This is very convenient, as not only will the existing partitions become indexed, but - also any partitions that are created in the future will. One limitation is - that it's not possible to use the CONCURRENTLY - qualifier when creating such a partitioned index. To avoid long lock + also any partitions that are created in the future will. To avoid long lock times, it is possible to use CREATE INDEX ON ONLY the partitioned table; such an index is marked invalid, and the partitions do not get the index applied automatically. The indexes on partitions can diff --git a/doc/src/sgml/ref/create_index.sgml b/doc/src/sgml/ref/create_index.sgml index 40986aa502f..b05102efdaf 100644 --- a/doc/src/sgml/ref/create_index.sgml +++ b/doc/src/sgml/ref/create_index.sgml @@ -645,7 +645,10 @@ CREATE [ UNIQUE ] INDEX [ CONCURRENTLY ] [ [ IF NOT EXISTS ] If a problem arises while scanning the table, such as a deadlock or a uniqueness violation in a unique index, the CREATE INDEX -command will fail but leave behind an invalid index. This index +command will fail but leave behind an invalid index. +If this happens while build an index concurrently on a partitioned +table, the command can also leave behind valid or +invalid indexes on table partitions. The invalid index will be ignored for querying purposes because it might be incomplete; however it will still consume update overhead. The psql \d command will report such an index as INVALID: @@ -692,15 +695,6 @@ Indexes: cannot. - -Concurrent builds for indexes on partitioned tables are currently not -supported. However, you may concurrently build the index on each -partition individually and then finally create the partitioned index -non-concurrently in order to reduce the time where writes to the -partitioned table will be locked out. In this case, building the -partitioned index is a metadata only operation. - - diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c index baf3e6e57a5..dfe64052b81 100644 --- a/src/backend/commands/indexcmds.c +++ b/src/backend/commands/indexcmds.c @@ -92,6 +92,11 @@ static char *ChooseIndexName(const char *tabname, Oid namespaceId, bool primary, bool isconstraint); static char *ChooseIndexNameAddition(List *colnames); static List *ChooseIndexColumnNames(List *indexElems); +static void DefineIndexConcurrentInternal(Oid relationId, + Oid indexRelationId, + IndexInfo *indexInfo, + LOCKTAG heaplocktag, + LockRelId heaprelid); static void ReindexIndex(RangeVar *indexRelation, ReindexParams *params, bool isTopLevel); static void RangeVarCallbackForReindexIndex(const RangeVar *relation, @@ -555,7 +560,6 @@ DefineIndex(Oid relationId, bool amissumma
Re: CREATE INDEX CONCURRENTLY on partitioned index
Justin Pryzby писал 2023-03-26 17:51: On Sun, Dec 04, 2022 at 01:09:35PM -0600, Justin Pryzby wrote: This currently handles partitions with a loop around the whole CIC implementation, which means that things like WaitForLockers() happen once for each index, the same as REINDEX CONCURRENTLY on a partitioned table. Contrast that with ReindexRelationConcurrently(), which handles all the indexes on a table in one pass by looping around indexes within each phase. Rebased over the progress reporting fix (27f5c712b). I added a list of (intermediate) partitioned tables, rather than looping over the list of inheritors again, to save calling rel_get_relkind(). I think this patch is done. Hi. Overall looks good to me. However, I think that using 'partitioned' as list of partitioned index oids in DefineIndex() is a bit misleading - we've just used it as boolean, specifying if we are dealing with a partitioned relation. -- Best regards, Alexander Pyhalov, Postgres Professional
Re: CREATE INDEX CONCURRENTLY on partitioned index
On Sun, Dec 04, 2022 at 01:09:35PM -0600, Justin Pryzby wrote: > This currently handles partitions with a loop around the whole CIC > implementation, which means that things like WaitForLockers() happen > once for each index, the same as REINDEX CONCURRENTLY on a partitioned > table. Contrast that with ReindexRelationConcurrently(), which handles > all the indexes on a table in one pass by looping around indexes within > each phase. Rebased over the progress reporting fix (27f5c712b). I added a list of (intermediate) partitioned tables, rather than looping over the list of inheritors again, to save calling rel_get_relkind(). I think this patch is done. -- Justin >From 941f7f930fc18563e2da42143015b6573d5447b1 Mon Sep 17 00:00:00 2001 From: Justin Pryzby Date: Sat, 6 Jun 2020 17:42:23 -0500 Subject: [PATCH] Allow CREATE INDEX CONCURRENTLY on partitioned table https://www.postgresql.org/message-id/flat/20201031063117.gf3...@telsasoft.com --- doc/src/sgml/ddl.sgml | 4 +- doc/src/sgml/ref/create_index.sgml | 14 +- src/backend/commands/indexcmds.c | 200 ++--- src/test/regress/expected/indexing.out | 127 +++- src/test/regress/sql/indexing.sql | 26 +++- 5 files changed, 297 insertions(+), 74 deletions(-) diff --git a/doc/src/sgml/ddl.sgml b/doc/src/sgml/ddl.sgml index 91c036d1cbe..64efdf1e879 100644 --- a/doc/src/sgml/ddl.sgml +++ b/doc/src/sgml/ddl.sgml @@ -4178,9 +4178,7 @@ ALTER TABLE measurement ATTACH PARTITION measurement_y2008m02 so that they are applied automatically to the entire hierarchy. This is very convenient, as not only will the existing partitions become indexed, but - also any partitions that are created in the future will. One limitation is - that it's not possible to use the CONCURRENTLY - qualifier when creating such a partitioned index. To avoid long lock + also any partitions that are created in the future will. To avoid long lock times, it is possible to use CREATE INDEX ON ONLY the partitioned table; such an index is marked invalid, and the partitions do not get the index applied automatically. The indexes on partitions can diff --git a/doc/src/sgml/ref/create_index.sgml b/doc/src/sgml/ref/create_index.sgml index 40986aa502f..b05102efdaf 100644 --- a/doc/src/sgml/ref/create_index.sgml +++ b/doc/src/sgml/ref/create_index.sgml @@ -645,7 +645,10 @@ CREATE [ UNIQUE ] INDEX [ CONCURRENTLY ] [ [ IF NOT EXISTS ] If a problem arises while scanning the table, such as a deadlock or a uniqueness violation in a unique index, the CREATE INDEX -command will fail but leave behind an invalid index. This index +command will fail but leave behind an invalid index. +If this happens while build an index concurrently on a partitioned +table, the command can also leave behind valid or +invalid indexes on table partitions. The invalid index will be ignored for querying purposes because it might be incomplete; however it will still consume update overhead. The psql \d command will report such an index as INVALID: @@ -692,15 +695,6 @@ Indexes: cannot. - -Concurrent builds for indexes on partitioned tables are currently not -supported. However, you may concurrently build the index on each -partition individually and then finally create the partitioned index -non-concurrently in order to reduce the time where writes to the -partitioned table will be locked out. In this case, building the -partitioned index is a metadata only operation. - - diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c index 3ec8b5cca6c..daba8b67dbe 100644 --- a/src/backend/commands/indexcmds.c +++ b/src/backend/commands/indexcmds.c @@ -93,6 +93,11 @@ static char *ChooseIndexName(const char *tabname, Oid namespaceId, bool primary, bool isconstraint); static char *ChooseIndexNameAddition(List *colnames); static List *ChooseIndexColumnNames(List *indexElems); +static void DefineIndexConcurrentInternal(Oid relationId, + Oid indexRelationId, + IndexInfo *indexInfo, + LOCKTAG heaplocktag, + LockRelId heaprelid); static void ReindexIndex(RangeVar *indexRelation, ReindexParams *params, bool isTopLevel); static void RangeVarCallbackForReindexIndex(const RangeVar *relation, @@ -559,7 +564,6 @@ DefineIndex(Oid relationId, bool amissummarizing; amoptions_function amoptions; bool partitioned; - bool safe_index; Datum reloptions; int16 *coloptions; IndexInfo *indexInfo; @@ -567,12 +571,10 @@ DefineIndex(Oid relationId, bits16 constr_flags; int numberOfAttributes; int numberOfKeyAttributes; - TransactionId limitXmin; ObjectAddress address; LockRelId heaprelid; LOCKTAG heaplocktag; LOCKMODE lockmode; - Snapshot snapshot; Oid root_save_userid; int root_save_sec_context; int root_sav
Re: CREATE INDEX CONCURRENTLY on partitioned index
On Sun, 2022-12-04 at 13:09 -0600, Justin Pryzby wrote:
>
> This beavior is fixed. I re-factored and re-implented to use
> DefineIndex() for building indexes concurrently rather than
> reindexing.
> That makes the patch smaller, actually, and has the added benefit of
> splitting off the "Concurrently" part of DefineIndex() into a
> separate
> function.
Nice, I think it turned out pretty concise. I played around with the
patch quite a bit, didn't find any major problems, the only minor thing
that I can note is that we should skip the top parent index itself in
the loop not to increment the pg_stat counter, something like this:
diff --git a/src/backend/commands/indexcmds.c
b/src/backend/commands/indexcmds.c
index cfab45b999..9049540b5b 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -1515,6 +1515,9 @@ DefineIndex(Oid relationId,
Oid indrelid =
lfirst_oid(lc);
Oid tabrelid =
IndexGetRelation(indrelid, false);
+ if (indrelid == indexRelationId)
+ continue;
+
if
(RELKIND_HAS_STORAGE(get_rel_relkind(indrelid)) &&
!get_index_isvalid(indrelid))
{
>
> BTW, it causes the patch to fail to apply in cfbot when you send an
> additional (002) supplementary patch without including the original
> (001) patch. You can name it *.txt to avoid the issue.
>
> https://wiki.postgresql.org/wiki/Cfbot#Which_attachments_are_considered_to_be_patches.3F
>
> Thanks for looking.
>
My bad, didn't know about this, thanks for the link.
On a side note, I noticed that reindex behaviour is strange on
partitioned tables, it doesn't mark partitioned tables as valid after
reindexing children, as I could understand from the code and mailing
lists, this is the intended behaviour, but I can't quite understand the
rationale for it, do you know why it is done this way?
Re: CREATE INDEX CONCURRENTLY on partitioned index
On Sat, Dec 03, 2022 at 07:13:30PM +0400, Ilya Gladyshev wrote: > Hi, > > Thank you Justin and Alexander for working on this, I have reviewed and > tested the latest patch, it works well, the problems mentioned > previously are all fixed. I like the idea of sharing code of reindex > and index, but I have noticed some peculiarities as a user. > > The reporting is somewhat confusing as it switches to reporting for > reindex concurrently while building child indexes, this should be fixed > with the simple patch I have attached. Another thing that I have > noticed is that REINDEX, which is used under the hood, creates new > indexes with suffix _ccnew, and if the index building fails, the > indexes that could not be build will have the name with _ccnew suffix. > This can actually be seen in your test: > > ERROR: could not create unique index "idxpart2_a_idx2_ccnew" > I find it quite confusing and I don't think that this the expected > behavior (if it is, I think it should be documented, like it is for > REINDEX). As an example of problems that it might entail, DROP INDEX > will not drop all the invalid indexes in the inheritance tree, because > it will leave _ccnew indexes in place, which is ok for reindex > concurrently, but that's not how C-I-C works now. I think that fixing > this problem requires some heavy code rewrite and I'm not quite sure This beavior is fixed. I re-factored and re-implented to use DefineIndex() for building indexes concurrently rather than reindexing. That makes the patch smaller, actually, and has the added benefit of splitting off the "Concurrently" part of DefineIndex() into a separate function. This currently handles partitions with a loop around the whole CIC implementation, which means that things like WaitForLockers() happen once for each index, the same as REINDEX CONCURRENTLY on a partitioned table. Contrast that with ReindexRelationConcurrently(), which handles all the indexes on a table in one pass by looping around indexes within each phase. BTW, it causes the patch to fail to apply in cfbot when you send an additional (002) supplementary patch without including the original (001) patch. You can name it *.txt to avoid the issue. https://wiki.postgresql.org/wiki/Cfbot#Which_attachments_are_considered_to_be_patches.3F Thanks for looking. -- Justin >From e25b15173f4ce939efa54426e369b6996129ff59 Mon Sep 17 00:00:00 2001 From: Justin Pryzby Date: Sat, 6 Jun 2020 17:42:23 -0500 Subject: [PATCH] Allow CREATE INDEX CONCURRENTLY on partitioned table --- doc/src/sgml/ddl.sgml | 4 +- doc/src/sgml/ref/create_index.sgml | 9 -- src/backend/commands/indexcmds.c | 172 + src/test/regress/expected/indexing.out | 127 +- src/test/regress/sql/indexing.sql | 26 +++- 5 files changed, 268 insertions(+), 70 deletions(-) diff --git a/doc/src/sgml/ddl.sgml b/doc/src/sgml/ddl.sgml index 38618de01c5..cd72b455447 100644 --- a/doc/src/sgml/ddl.sgml +++ b/doc/src/sgml/ddl.sgml @@ -4163,9 +4163,7 @@ ALTER TABLE measurement ATTACH PARTITION measurement_y2008m02 so that they are applied automatically to the entire hierarchy. This is very convenient, as not only will the existing partitions become indexed, but - also any partitions that are created in the future will. One limitation is - that it's not possible to use the CONCURRENTLY - qualifier when creating such a partitioned index. To avoid long lock + also any partitions that are created in the future will. To avoid long lock times, it is possible to use CREATE INDEX ON ONLY the partitioned table; such an index is marked invalid, and the partitions do not get the index applied automatically. The indexes on partitions can diff --git a/doc/src/sgml/ref/create_index.sgml b/doc/src/sgml/ref/create_index.sgml index 40986aa502f..fc8cda655f0 100644 --- a/doc/src/sgml/ref/create_index.sgml +++ b/doc/src/sgml/ref/create_index.sgml @@ -692,15 +692,6 @@ Indexes: cannot. - -Concurrent builds for indexes on partitioned tables are currently not -supported. However, you may concurrently build the index on each -partition individually and then finally create the partitioned index -non-concurrently in order to reduce the time where writes to the -partitioned table will be locked out. In this case, building the -partitioned index is a metadata only operation. - - diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c index b5b860c3abf..cfab45b9992 100644 --- a/src/backend/commands/indexcmds.c +++ b/src/backend/commands/indexcmds.c @@ -92,6 +92,11 @@ static char *ChooseIndexName(const char *tabname, Oid namespaceId, bool primary, bool isconstraint); static char *ChooseIndexNameAddition(List *colnames); static List *ChooseIndexColumnNames(List *indexElems); +static void DefineIndexConcurrentInternal(Oid relationId, + Oid
Re: CREATE INDEX CONCURRENTLY on partitioned index
Hi,
Thank you Justin and Alexander for working on this, I have reviewed and
tested the latest patch, it works well, the problems mentioned
previously are all fixed. I like the idea of sharing code of reindex
and index, but I have noticed some peculiarities as a user.
The reporting is somewhat confusing as it switches to reporting for
reindex concurrently while building child indexes, this should be fixed
with the simple patch I have attached. Another thing that I have
noticed is that REINDEX, which is used under the hood, creates new
indexes with suffix _ccnew, and if the index building fails, the
indexes that could not be build will have the name with _ccnew suffix.
This can actually be seen in your test:
ERROR: could not create unique index "idxpart2_a_idx2_ccnew"
I find it quite confusing and I don't think that this the expected
behavior (if it is, I think it should be documented, like it is for
REINDEX). As an example of problems that it might entail, DROP INDEX
will not drop all the invalid indexes in the inheritance tree, because
it will leave _ccnew indexes in place, which is ok for reindex
concurrently, but that's not how C-I-C works now. I think that fixing
this problem requires some heavy code rewrite and I'm not quite sure
how to go about it, if you have any ideas, I will be happy to try them
out.
Thanks,
Ilya
From 8eb9fd7ce7d34c5c323c47b60a7f883f360ef090 Mon Sep 17 00:00:00 2001
From: Ilya Gladyshev
Date: Sat, 3 Dec 2022 18:20:03 +0400
Subject: [PATCH] turn off reindex reporting for create
---
src/backend/commands/indexcmds.c | 36 +++-
1 file changed, 21 insertions(+), 15 deletions(-)
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index a2775931e2..b3c713037f 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -3804,14 +3804,16 @@ ReindexRelationConcurrently(Oid relationOid, ReindexParams *params)
/* Don't overwrite CREATE INDEX command */
if (!(params->options & REINDEXOPT_REPORT_CREATE_PART))
+ {
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[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);
+ }
/* Choose a temporary relation name for the new index */
concurrentName = ChooseRelationName(get_rel_name(idx->indexId),
@@ -3967,13 +3969,15 @@ ReindexRelationConcurrently(Oid relationOid, ReindexParams *params)
* table involved. Don't overwrite CREATE INDEX command.
*/
if (!(params->options & REINDEXOPT_REPORT_CREATE_PART))
+ {
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[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);
+ }
/* Perform concurrent build of new index */
index_concurrently_build(newidx->tableId, newidx->indexId);
@@ -4033,14 +4037,16 @@ ReindexRelationConcurrently(Oid relationOid, ReindexParams *params)
* table involved. Don't overwrite CREATE INDEX command.
*/
if (!(params->options & REINDEXOPT_REPORT_CREATE_PART))
+ {
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[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);
+ }
validate_index(newidx->tableId, newidx->indexId, snapshot);
--
2.30.2
Re: CREATE INDEX CONCURRENTLY on partitioned index
Justin Pryzby писал 2022-11-21 06:00: I finally found time to digest and integrate your changes into my local branch. This fixes the three issues you reported: FORCE_RELEASE, issue with INVALID partitions issue (for which I adapted your patch into an earlier patch in my series), and progress reporting. And rebased. Hi. Thank you for the effort. I've looked through and tested new patch a bit. Overall it looks good to me. The question I have is whether we should update pg_stat_progress_create_index in reindex_invalid_child_indexes(), when we skip valid indexes? -- Best regards, Alexander Pyhalov, Postgres Professional
Re: CREATE INDEX CONCURRENTLY on partitioned index
I finally found time to digest and integrate your changes into my local
branch. This fixes the three issues you reported: FORCE_RELEASE, issue
with INVALID partitions issue (for which I adapted your patch into an
earlier patch in my series), and progress reporting. And rebased.
--
Justin
>From 4ba360eaaac5e1ac169d41c26cf6213b0c6a2432 Mon Sep 17 00:00:00 2001
From: Justin Pryzby
Date: Sat, 6 Jun 2020 17:42:23 -0500
Subject: [PATCH] Allow CREATE INDEX CONCURRENTLY on partitioned table
Note, this effectively reverts 050098b14, so take care to not reintroduce the
bug it fixed.
---
doc/src/sgml/ddl.sgml | 4 +-
doc/src/sgml/ref/create_index.sgml | 9 --
src/backend/commands/indexcmds.c | 214 +++--
src/include/catalog/index.h| 1 +
src/test/regress/expected/indexing.out | 136 +++-
src/test/regress/sql/indexing.sql | 26 ++-
6 files changed, 320 insertions(+), 70 deletions(-)
diff --git a/doc/src/sgml/ddl.sgml b/doc/src/sgml/ddl.sgml
index 03c01937094..fd56e21ef49 100644
--- a/doc/src/sgml/ddl.sgml
+++ b/doc/src/sgml/ddl.sgml
@@ -4131,9 +4131,7 @@ ALTER TABLE measurement ATTACH PARTITION measurement_y2008m02
so that they are applied automatically to the entire hierarchy.
This is very
convenient, as not only will the existing partitions become indexed, but
- also any partitions that are created in the future will. One limitation is
- that it's not possible to use the CONCURRENTLY
- qualifier when creating such a partitioned index. To avoid long lock
+ also any partitions that are created in the future will. To avoid long lock
times, it is possible to use CREATE INDEX ON ONLY
the partitioned table; such an index is marked invalid, and the partitions
do not get the index applied automatically. The indexes on partitions can
diff --git a/doc/src/sgml/ref/create_index.sgml b/doc/src/sgml/ref/create_index.sgml
index 40986aa502f..fc8cda655f0 100644
--- a/doc/src/sgml/ref/create_index.sgml
+++ b/doc/src/sgml/ref/create_index.sgml
@@ -692,15 +692,6 @@ Indexes:
cannot.
-
-Concurrent builds for indexes on partitioned tables are currently not
-supported. However, you may concurrently build the index on each
-partition individually and then finally create the partitioned index
-non-concurrently in order to reduce the time where writes to the
-partitioned table will be locked out. In this case, building the
-partitioned index is a metadata only operation.
-
-
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 91cee27743d..bb98e745267 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -71,6 +71,7 @@
/* non-export function prototypes */
+static void reindex_invalid_child_indexes(Oid indexRelationId);
static bool CompareOpclassOptions(Datum *opts1, Datum *opts2, int natts);
static void CheckPredicate(Expr *predicate);
static void ComputeIndexAttrs(IndexInfo *indexInfo,
@@ -104,7 +105,9 @@ static void reindex_error_callback(void *arg);
static void ReindexPartitions(Oid relid, ReindexParams *params,
bool isTopLevel);
static void ReindexMultipleInternal(List *relids,
- ReindexParams *params);
+ ReindexParams *params,
+ Oid parent,
+ int npart);
static bool ReindexRelationConcurrently(Oid relationOid,
ReindexParams *params);
static void update_relispartition(Oid relationId, bool newval);
@@ -697,17 +700,6 @@ DefineIndex(Oid relationId,
partitioned = rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE;
if (partitioned)
{
- /*
- * Note: we check 'stmt->concurrent' rather than 'concurrent', so that
- * the error is thrown also for temporary tables. Seems better to be
- * consistent, even though we could do it on temporary table because
- * we're not actually doing it concurrently.
- */
- if (stmt->concurrent)
- ereport(ERROR,
- (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("cannot create index on partitioned table \"%s\" concurrently",
- RelationGetRelationName(rel;
if (stmt->excludeOpNames)
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
@@ -1147,6 +1139,11 @@ DefineIndex(Oid relationId,
if (pd->nparts != 0)
flags |= INDEX_CREATE_INVALID;
}
+ else if (concurrent && OidIsValid(parentIndexId))
+ {
+ /* If concurrent, initially build index partitions as "invalid" */
+ flags |= INDEX_CREATE_INVALID;
+ }
if (stmt->deferrable)
constr_flags |= INDEX_CONSTR_CREATE_DEFERRABLE;
@@ -1212,17 +1209,30 @@ DefineIndex(Oid relationId,
partdesc = RelationGetPartitionDesc(rel, true);
if ((!stmt->relation || stmt->relation->inh) && partdesc->nparts > 0)
{
+ /*
+ * Need to close the relation before recursing into children, so
+ * copy needed data into a longlived context.
+ */
+
+ MemoryContext ind_context = AllocSetConte
Re: CREATE INDEX CONCURRENTLY on partitioned index
Justin Pryzby писал 2022-06-28 21:33:
Hi,
On Thu, Feb 10, 2022 at 06:07:08PM +0300, Alexander Pyhalov wrote:
I've rebased patches and tried to fix issues I've seen. I've fixed
reference
after table_close() in the first patch (can be seen while building
with
CPPFLAGS='-DRELCACHE_FORCE_RELEASE').
Rebased patches on the current master.
They still require proper review.
--
Best regards,
Alexander Pyhalov,
Postgres ProfessionalFrom 5c11849ceb2a1feb0e44dbdf30cc27de0282a659 Mon Sep 17 00:00:00 2001
From: Alexander Pyhalov
Date: Mon, 28 Feb 2022 10:50:58 +0300
Subject: [PATCH 5/5] Mark intermediate partitioned indexes as valid
---
src/backend/commands/indexcmds.c | 33 ++-
src/test/regress/expected/indexing.out | 80 +-
src/test/regress/sql/indexing.sql | 8 +++
3 files changed, 118 insertions(+), 3 deletions(-)
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index d09f0390413..d3ced6265b6 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -3139,6 +3139,7 @@ static void
ReindexPartitions(Oid relid, ReindexParams *params, bool isTopLevel)
{
List *partitions = NIL;
+ List *inhpartindexes = NIL;
char relkind = get_rel_relkind(relid);
char *relname = get_rel_name(relid);
char *relnamespace = get_namespace_name(get_rel_namespace(relid));
@@ -3193,6 +3194,17 @@ ReindexPartitions(Oid relid, ReindexParams *params, bool isTopLevel)
char partkind = get_rel_relkind(partoid);
MemoryContext old_context;
+ /* Create a list of invalid inherited partitioned indexes */
+ if (partkind == RELKIND_PARTITIONED_INDEX)
+ {
+ if (partoid == relid || get_index_isvalid(partoid))
+continue;
+
+ old_context = MemoryContextSwitchTo(reindex_context);
+ inhpartindexes = lappend_oid(inhpartindexes, partoid);
+ MemoryContextSwitchTo(old_context);
+ }
+
/*
* This discards partitioned tables, partitioned indexes and foreign
* tables.
@@ -3237,9 +3249,28 @@ ReindexPartitions(Oid relid, ReindexParams *params, bool isTopLevel)
Oid tableoid = IndexGetRelation(relid, false);
List *child_tables = find_all_inheritors(tableoid, ShareLock, NULL);
- /* Both lists include their parent relation as well as any intermediate partitioned rels */
+ /*
+ * Both lists include their parent relation as well as any
+ * intermediate partitioned rels
+ */
if (list_length(inhoids) == list_length(child_tables))
+ {
index_set_state_flags(relid, INDEX_CREATE_SET_VALID);
+
+ /* Mark any intermediate partitioned index as valid */
+ foreach(lc, inhpartindexes)
+ {
+Oid partoid = lfirst_oid(lc);
+
+Assert(get_rel_relkind(partoid) == RELKIND_PARTITIONED_INDEX);
+Assert(!get_index_isvalid(partoid));
+
+/* Can't mark an index valid without marking it ready */
+index_set_state_flags(partoid, INDEX_CREATE_SET_READY);
+CommandCounterIncrement();
+index_set_state_flags(partoid, INDEX_CREATE_SET_VALID);
+ }
+ }
}
/*
diff --git a/src/test/regress/expected/indexing.out b/src/test/regress/expected/indexing.out
index a4ccae50de3..b4f1aea6fca 100644
--- a/src/test/regress/expected/indexing.out
+++ b/src/test/regress/expected/indexing.out
@@ -57,6 +57,8 @@ create table idxpart11 partition of idxpart1 for values from (0) to (10) partiti
create table idxpart111 partition of idxpart11 default partition by range(a);
create table idxpart partition of idxpart111 default partition by range(a);
create table idxpart2 partition of idxpart for values from (10) to (20);
+create table idxpart3 partition of idxpart for values from (30) to (40) partition by range(a);
+create table idxpart31 partition of idxpart3 default;
insert into idxpart2 values(10),(10); -- not unique
create index concurrently on idxpart (a); -- partitioned
create index concurrently on idxpart1 (a); -- partitioned and partition
@@ -76,7 +78,7 @@ Partition key: RANGE (a)
Indexes:
"idxpart_a_idx" btree (a)
"idxpart_a_idx1" UNIQUE, btree (a) INVALID
-Number of partitions: 2 (Use \d+ to list them.)
+Number of partitions: 3 (Use \d+ to list them.)
\d idxpart1
Partitioned table "public.idxpart1"
@@ -88,11 +90,59 @@ Number of partitions: 2 (Use \d+ to list them.)
Partition of: idxpart FOR VALUES FROM (0) TO (10)
Partition key: RANGE (a)
Indexes:
-"idxpart1_a_idx" btree (a) INVALID
+"idxpart1_a_idx" btree (a)
"idxpart1_a_idx1" btree (a)
"idxpart1_a_idx2" UNIQUE, btree (a) INVALID
Number of partitions: 1 (Use \d+ to list them.)
+\d idxpart11
+ Partitioned table "public.idxpart11"
+ Column | Type | Collation | Nullable | Default
++-+---+--+-
+ a | integer | | |
+ b | integer | | |
+ c | text| | |
+Partition of: idxpart1 FOR VALUES FROM (0) TO (10)
+Partition key: RANGE (a)
+Indexes:
+"idxpart11_a_idx" b
Re: CREATE INDEX CONCURRENTLY on partitioned index
Justin Pryzby писал 2022-06-28 21:33: Hi, On Thu, Feb 10, 2022 at 06:07:08PM +0300, Alexander Pyhalov wrote: I've rebased patches and tried to fix issues I've seen. I've fixed reference after table_close() in the first patch (can be seen while building with CPPFLAGS='-DRELCACHE_FORCE_RELEASE'). Thanks for finding that. The patches other than 0001 are more experimental, and need someone to check if it's even a good approach to use, so I kept them separate from the essential patch. Your latest 0005 patch (mark intermediate partitioned indexes as valid) is probably fixing a bug in my SKIPVALID patch, right ? I'm not sure whether the SKIPVALID patch should be merged into 0001, and I've been awaiting feedback on the main patch before handling progress reporting. Hi. I think it's more about fixing ReindexPartitions-to-set-indisvalid patch, as we also should mark intermediate indexes as valid when reindex succeeds. Sorry for not responding sooner. The patch saw no activity for ~11 months so I wasn't prepared to pick it up in March, at least not without guidance from a committer. Would you want to take over this patch ? I wrote it following someone's question, but don't expect that I'd use the feature myself. I can help review it or try to clarify the organization of my existing patches (but still haven't managed to work my way through your amendments to my patches). Yes, I'm glad to work on the patches, as this for us this is a very important feature. -- Best regards, Alexander Pyhalov, Postgres Professional
Re: CREATE INDEX CONCURRENTLY on partitioned index
Hi, On Thu, Feb 10, 2022 at 06:07:08PM +0300, Alexander Pyhalov wrote: > I've rebased patches and tried to fix issues I've seen. I've fixed reference > after table_close() in the first patch (can be seen while building with > CPPFLAGS='-DRELCACHE_FORCE_RELEASE'). Thanks for finding that. The patches other than 0001 are more experimental, and need someone to check if it's even a good approach to use, so I kept them separate from the essential patch. Your latest 0005 patch (mark intermediate partitioned indexes as valid) is probably fixing a bug in my SKIPVALID patch, right ? I'm not sure whether the SKIPVALID patch should be merged into 0001, and I've been awaiting feedback on the main patch before handling progress reporting. Sorry for not responding sooner. The patch saw no activity for ~11 months so I wasn't prepared to pick it up in March, at least not without guidance from a committer. Would you want to take over this patch ? I wrote it following someone's question, but don't expect that I'd use the feature myself. I can help review it or try to clarify the organization of my existing patches (but still haven't managed to work my way through your amendments to my patches). Thanks for caring about partitioned DDL ;) -- Justin
Re: CREATE INDEX CONCURRENTLY on partitioned index
On Fri, Mar 25, 2022 at 01:05:49AM -0400, Greg Stark wrote: > This patch is marked "waiting on author" in the CF. However the most > recent emails have patches and it's not clear to me what's left from > previous reviews that might not be addressed yet. Should this patch be > marked "Needs Review"? > > Anastasia and Alexander are marked as reviewers. Are you still able to > review it or are there still pending issues that need to be resolved > from previous reviews? I still haven't responded to Alexander's feedback, so I need to do that. (Sorry). However, since the patch attracted no attention for 50 some weeks last year, so now is a weird time to shift attention to it. As such, I will move it to the next CF. https://www.postgresql.org/message-id/flat/20210226182019.gu20...@telsasoft.com#da169a0a518bf8121604437d9ab053b3 -- Justin
Re: CREATE INDEX CONCURRENTLY on partitioned index
This patch is marked "waiting on author" in the CF. However the most recent emails have patches and it's not clear to me what's left from previous reviews that might not be addressed yet. Should this patch be marked "Needs Review"? Anastasia and Alexander are marked as reviewers. Are you still able to review it or are there still pending issues that need to be resolved from previous reviews?
Re: CREATE INDEX CONCURRENTLY on partitioned index
Hi.
I've added 0005-Mark-intermediate-partitioned-indexes-as-valid.patch
which fixed the following issues - when partitioned index is created,
indexes on intermediate partitioned tables were preserved in invalid
state. Also added some more tests.
--
Best regards,
Alexander Pyhalov,
Postgres ProfessionalFrom 18fa3c27a3311294a7abfdc0674ef6143c65423b Mon Sep 17 00:00:00 2001
From: Justin Pryzby
Date: Mon, 7 Feb 2022 10:28:42 +0300
Subject: [PATCH 1/5] Allow CREATE INDEX CONCURRENTLY on partitioned table
0001-Allow-CREATE-INDEX-CONCURRENTLY-on-partitioned-table.patch from
https://www.postgresql.org/message-id/20210226182019.gu20...@telsasoft.com
Fixes:
- rel was used after table_close();
- it seems childidxs shouldn't live in ind_context;
- updated doc.
---
doc/src/sgml/ref/create_index.sgml | 14 +--
src/backend/commands/indexcmds.c | 151 ++---
src/test/regress/expected/indexing.out | 60 +-
src/test/regress/sql/indexing.sql | 18 ++-
4 files changed, 186 insertions(+), 57 deletions(-)
diff --git a/doc/src/sgml/ref/create_index.sgml b/doc/src/sgml/ref/create_index.sgml
index 91eaaabc90f..ffa98692430 100644
--- a/doc/src/sgml/ref/create_index.sgml
+++ b/doc/src/sgml/ref/create_index.sgml
@@ -641,7 +641,10 @@ CREATE [ UNIQUE ] INDEX [ CONCURRENTLY ] [ [ IF NOT EXISTS ]
If a problem arises while scanning the table, such as a deadlock or a
uniqueness violation in a unique index, the CREATE INDEX
-command will fail but leave behind an invalid index. This index
+command will fail but leave behind an invalid index.
+If this happens while creating index concurrently on a partitioned
+table, the command can also leave behind valid or
+invalid indexes on table partitions. The invalid index
will be ignored for querying purposes because it might be incomplete;
however it will still consume update overhead. The psql
\d command will report such an index as INVALID:
@@ -688,15 +691,6 @@ Indexes:
cannot.
-
-Concurrent builds for indexes on partitioned tables are currently not
-supported. However, you may concurrently build the index on each
-partition individually and then finally create the partitioned index
-non-concurrently in order to reduce the time where writes to the
-partitioned table will be locked out. In this case, building the
-partitioned index is a metadata only operation.
-
-
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index cd30f15eba6..a34a1b133a0 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -68,6 +68,7 @@
/* non-export function prototypes */
+static void reindex_invalid_child_indexes(Oid indexRelationId);
static bool CompareOpclassOptions(Datum *opts1, Datum *opts2, int natts);
static void CheckPredicate(Expr *predicate);
static void ComputeIndexAttrs(IndexInfo *indexInfo,
@@ -670,17 +671,6 @@ DefineIndex(Oid relationId,
partitioned = rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE;
if (partitioned)
{
- /*
- * Note: we check 'stmt->concurrent' rather than 'concurrent', so that
- * the error is thrown also for temporary tables. Seems better to be
- * consistent, even though we could do it on temporary table because
- * we're not actually doing it concurrently.
- */
- if (stmt->concurrent)
- ereport(ERROR,
- (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("cannot create index on partitioned table \"%s\" concurrently",
- RelationGetRelationName(rel;
if (stmt->excludeOpNames)
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
@@ -1119,6 +1109,11 @@ DefineIndex(Oid relationId,
if (pd->nparts != 0)
flags |= INDEX_CREATE_INVALID;
}
+ else if (concurrent && OidIsValid(parentIndexId))
+ {
+ /* If concurrent, initially build index partitions as "invalid" */
+ flags |= INDEX_CREATE_INVALID;
+ }
if (stmt->deferrable)
constr_flags |= INDEX_CONSTR_CREATE_DEFERRABLE;
@@ -1174,18 +1169,30 @@ DefineIndex(Oid relationId,
partdesc = RelationGetPartitionDesc(rel, true);
if ((!stmt->relation || stmt->relation->inh) && partdesc->nparts > 0)
{
+ /*
+ * Need to close the relation before recursing into children, so
+ * copy needed data into a longlived context.
+ */
+
+ MemoryContext ind_context = AllocSetContextCreate(PortalContext, "CREATE INDEX",
+ ALLOCSET_DEFAULT_SIZES);
+ MemoryContext oldcontext = MemoryContextSwitchTo(ind_context);
int nparts = partdesc->nparts;
Oid *part_oids = palloc(sizeof(Oid) * nparts);
bool invalidate_parent = false;
TupleDesc parentDesc;
Oid *opfamOids;
+ char *relname;
pgstat_progress_update_param(PROGRESS_CREATEIDX_PARTITIONS_TOTAL,
nparts);
memcpy(part_oids, partdesc->oids, sizeof(Oid) * nparts);
+ parentDesc = CreateTupleDescCopy(RelationGetDescr(rel));
+ relname = pstrdup(RelationG
Re: CREATE INDEX CONCURRENTLY on partitioned index
Justin Pryzby писал 2021-02-26 21:20:
On Mon, Feb 15, 2021 at 10:07:05PM +0300, Anastasia Lubennikova wrote:
5) Speaking of documentation, I think we need to add a paragraph about
CIC
on partitioned indexes which will explain that invalid indexes may
appear
and what user should do to fix them.
I'm not sure about that - it's already documented in general, for
nonpartitioned indexes.
Hi.
I've rebased patches and tried to fix issues I've seen. I've fixed
reference after table_close() in the first patch (can be seen while
building with CPPFLAGS='-DRELCACHE_FORCE_RELEASE'). It seems childidxs
shouldn't live in ind_context, so I moved it out of it. Updated
documentation to state that CIC can leave invalid or valid indexes on
partitions if it's not succeeded. Also merged old
0002-f-progress-reporting.patch and
0003-WIP-Add-SKIPVALID-flag-for-more-integration.patch. It seems the
first one didn't really fixed issue with progress report (as
ReindexRelationConcurrently() uses pgstat_progress_start_command(),
which seems to mess up the effect of this command in DefineIndex()).
Note, that third patch completely removes attempts to report create
index progress correctly (reindex reports about individual commands, not
the whole CREATE INDEX).
So I've added 0003-Try-to-fix-create-index-progress-report.patch, which
tries to fix the mess with create index progress report. It introduces
new flag REINDEXOPT_REPORT_CREATE_PART to ReindexParams->options. Given
this flag, ReindexRelationConcurrently() will not report about
individual operations start/stop, but ReindexMultipleInternal() will
report about reindexed partitions. To make the issue worse, some
partitions can be handled in ReindexPartitions() and
ReindexMultipleInternal() should know how many to correctly update
PROGRESS_CREATEIDX_PARTITIONS_DONE counter. Also it needs IndexOid to
correctly generate pg_stat_progress_create_index record, so we pass
these parameters to it.
--
Best regards,
Alexander Pyhalov,
Postgres ProfessionalFrom eaad7c3ed2fda93fdb91aea60294f60489444bf7 Mon Sep 17 00:00:00 2001
From: Justin Pryzby
Date: Mon, 7 Feb 2022 10:28:42 +0300
Subject: [PATCH 1/4] Allow CREATE INDEX CONCURRENTLY on partitioned table
0001-Allow-CREATE-INDEX-CONCURRENTLY-on-partitioned-table.patch from
https://www.postgresql.org/message-id/20210226182019.gu20...@telsasoft.com
Fixes:
- rel was used after table_close();
- it seems childidxs shouldn't live in ind_context;
- updated doc.
---
doc/src/sgml/ref/create_index.sgml | 14 +--
src/backend/commands/indexcmds.c | 151 ++---
src/test/regress/expected/indexing.out | 60 +-
src/test/regress/sql/indexing.sql | 18 ++-
4 files changed, 186 insertions(+), 57 deletions(-)
diff --git a/doc/src/sgml/ref/create_index.sgml b/doc/src/sgml/ref/create_index.sgml
index 91eaaabc90f..ffa98692430 100644
--- a/doc/src/sgml/ref/create_index.sgml
+++ b/doc/src/sgml/ref/create_index.sgml
@@ -641,7 +641,10 @@ CREATE [ UNIQUE ] INDEX [ CONCURRENTLY ] [ [ IF NOT EXISTS ]
If a problem arises while scanning the table, such as a deadlock or a
uniqueness violation in a unique index, the CREATE INDEX
-command will fail but leave behind an invalid index. This index
+command will fail but leave behind an invalid index.
+If this happens while creating index concurrently on a partitioned
+table, the command can also leave behind valid or
+invalid indexes on table partitions. The invalid index
will be ignored for querying purposes because it might be incomplete;
however it will still consume update overhead. The psql
\d command will report such an index as INVALID:
@@ -688,15 +691,6 @@ Indexes:
cannot.
-
-Concurrent builds for indexes on partitioned tables are currently not
-supported. However, you may concurrently build the index on each
-partition individually and then finally create the partitioned index
-non-concurrently in order to reduce the time where writes to the
-partitioned table will be locked out. In this case, building the
-partitioned index is a metadata only operation.
-
-
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 560dcc87a2c..666ced8e1d7 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -68,6 +68,7 @@
/* non-export function prototypes */
+static void reindex_invalid_child_indexes(Oid indexRelationId);
static bool CompareOpclassOptions(Datum *opts1, Datum *opts2, int natts);
static void CheckPredicate(Expr *predicate);
static void ComputeIndexAttrs(IndexInfo *indexInfo,
@@ -670,17 +671,6 @@ DefineIndex(Oid relationId,
partitioned = rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE;
if (partitioned)
{
- /*
- * Note: we check 'stmt->concurrent' rather than 'concurrent', so that
- * the error is thrown also for temporary tables. Seems better to be
- * con
Re: CREATE INDEX CONCURRENTLY on partitioned index
On Mon, Feb 15, 2021 at 10:07:05PM +0300, Anastasia Lubennikova wrote:
> 5) Speaking of documentation, I think we need to add a paragraph about CIC
> on partitioned indexes which will explain that invalid indexes may appear
> and what user should do to fix them.
I'm not sure about that - it's already documented in general, for
nonpartitioned indexes.
--
Justin
>From fb60da3c0fac8f1699a6caeea57476770c66576d Mon Sep 17 00:00:00 2001
From: Justin Pryzby
Date: Sat, 6 Jun 2020 17:42:23 -0500
Subject: [PATCH 1/5] Allow CREATE INDEX CONCURRENTLY on partitioned table
Note, this effectively reverts 050098b14, so take care to not reintroduce the
bug it fixed.
---
doc/src/sgml/ref/create_index.sgml | 9 --
src/backend/commands/indexcmds.c | 143 ++---
src/test/regress/expected/indexing.out | 60 ++-
src/test/regress/sql/indexing.sql | 18 +++-
4 files changed, 176 insertions(+), 54 deletions(-)
diff --git a/doc/src/sgml/ref/create_index.sgml b/doc/src/sgml/ref/create_index.sgml
index 965dcf472c..7c75119d78 100644
--- a/doc/src/sgml/ref/create_index.sgml
+++ b/doc/src/sgml/ref/create_index.sgml
@@ -686,15 +686,6 @@ Indexes:
cannot.
-
-Concurrent builds for indexes on partitioned tables are currently not
-supported. However, you may concurrently build the index on each
-partition individually and then finally create the partitioned index
-non-concurrently in order to reduce the time where writes to the
-partitioned table will be locked out. In this case, building the
-partitioned index is a metadata only operation.
-
-
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 8bc652ecd3..9ab1a66971 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -68,6 +68,7 @@
/* non-export function prototypes */
+static void reindex_invalid_child_indexes(Oid indexRelationId);
static bool CompareOpclassOptions(Datum *opts1, Datum *opts2, int natts);
static void CheckPredicate(Expr *predicate);
static void ComputeIndexAttrs(IndexInfo *indexInfo,
@@ -680,17 +681,6 @@ DefineIndex(Oid relationId,
partitioned = rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE;
if (partitioned)
{
- /*
- * Note: we check 'stmt->concurrent' rather than 'concurrent', so that
- * the error is thrown also for temporary tables. Seems better to be
- * consistent, even though we could do it on temporary table because
- * we're not actually doing it concurrently.
- */
- if (stmt->concurrent)
- ereport(ERROR,
- (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("cannot create index on partitioned table \"%s\" concurrently",
- RelationGetRelationName(rel;
if (stmt->excludeOpNames)
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
@@ -1128,6 +1118,11 @@ DefineIndex(Oid relationId,
if (pd->nparts != 0)
flags |= INDEX_CREATE_INVALID;
}
+ else if (concurrent && OidIsValid(parentIndexId))
+ {
+ /* If concurrent, initially build index partitions as "invalid" */
+ flags |= INDEX_CREATE_INVALID;
+ }
if (stmt->deferrable)
constr_flags |= INDEX_CONSTR_CREATE_DEFERRABLE;
@@ -1183,6 +1178,14 @@ DefineIndex(Oid relationId,
partdesc = RelationGetPartitionDesc(rel);
if ((!stmt->relation || stmt->relation->inh) && partdesc->nparts > 0)
{
+ /*
+ * Need to close the relation before recursing into children, so
+ * copy needed data into a longlived context.
+ */
+
+ MemoryContext ind_context = AllocSetContextCreate(PortalContext, "CREATE INDEX",
+ ALLOCSET_DEFAULT_SIZES);
+ MemoryContext oldcontext = MemoryContextSwitchTo(ind_context);
int nparts = partdesc->nparts;
Oid *part_oids = palloc(sizeof(Oid) * nparts);
bool invalidate_parent = false;
@@ -1193,8 +1196,10 @@ DefineIndex(Oid relationId,
nparts);
memcpy(part_oids, partdesc->oids, sizeof(Oid) * nparts);
+ parentDesc = CreateTupleDescCopy(RelationGetDescr(rel));
+ table_close(rel, NoLock);
+ MemoryContextSwitchTo(oldcontext);
- parentDesc = RelationGetDescr(rel);
opfamOids = palloc(sizeof(Oid) * numberOfKeyAttributes);
for (i = 0; i < numberOfKeyAttributes; i++)
opfamOids[i] = get_opclass_family(classObjectId[i]);
@@ -1237,10 +1242,12 @@ DefineIndex(Oid relationId,
continue;
}
+oldcontext = MemoryContextSwitchTo(ind_context);
childidxs = RelationGetIndexList(childrel);
attmap =
build_attrmap_by_name(RelationGetDescr(childrel),
parentDesc);
+MemoryContextSwitchTo(oldcontext);
foreach(cell, childidxs)
{
@@ -1311,10 +1318,14 @@ DefineIndex(Oid relationId,
*/
if (!found)
{
- IndexStmt *childStmt = copyObject(stmt);
+ IndexStmt *childStmt;
bool found_whole_row;
ListCell *lc;
+ oldcontext = MemoryContextSwitchTo(ind_context);
+ childStmt = copyObject(stmt);
+ MemoryContextSwitchTo(ol
Re: CREATE INDEX CONCURRENTLY on partitioned index
Hi, For v13-0006-More-refactoring.patch : + /* It's not a shared catalog, so refuse to move it to shared tablespace */ + if (params->tablespaceOid == GLOBALTABLESPACE_OID && false) + ereport(ERROR, Do you intend to remove the ineffective check ? + else + heapRelation = table_open(heapId, + ShareUpdateExclusiveLock); + table_close(heapRelation, NoLock); The table_open() seems to be unnecessary since there is no check after the open. + // heapRelationIds = list_make1_oid(heapId); If the code is not needed, you can remove the above. For v13-0005-Refactor-to-allow-reindexing-all-index-partition.patch : + /* Skip invalid indexes, if requested */ + if ((options & REINDEXOPT_SKIPVALID) != 0 && + get_index_isvalid(partoid)) The comment seems to diverge from the name of the flag (which says skip valid index). Cheers On Mon, Feb 15, 2021 at 11:34 AM Justin Pryzby wrote: > On Mon, Feb 15, 2021 at 10:06:47PM +0300, Anastasia Lubennikova wrote: > > On 28.01.2021 17:30, Justin Pryzby wrote: > > > On Thu, Jan 28, 2021 at 09:51:51PM +0900, Masahiko Sawada wrote: > > > > On Mon, Nov 30, 2020 at 5:22 AM Justin Pryzby > wrote: > > > > > On Sat, Oct 31, 2020 at 01:31:17AM -0500, Justin Pryzby wrote: > > > > > > Forking this thread, since the existing CFs have been closed. > > > > > > > https://www.postgresql.org/message-id/flat/20200914143102.GX18552%40telsasoft.com#58b1056488451f8594b0f0ba40996afd > > > > > > > > > > > > The strategy is to create catalog entries for all tables with > indisvalid=false, > > > > > > and then process them like REINDEX CONCURRENTLY. If it's > interrupted, it > > > > > > leaves INVALID indexes, which can be cleaned up with DROP or > REINDEX, same as > > > > > > CIC on a plain table. > > > > > > > > > > > > On Sat, Aug 08, 2020 at 01:37:44AM -0500, Justin Pryzby wrote: > > > > > > > On Mon, Jun 15, 2020 at 09:37:42PM +0900, Michael Paquier > wrote: > > > > > > > Note that the mentioned problem wasn't serious: there was > missing index on > > > > > > > child table, therefor the parent index was invalid, as > intended. However I > > > > > > > agree that it's not nice that the command can fail so easily > and leave behind > > > > > > > some indexes created successfully and some failed some not > created at all. > > > > > > > > > > > > > > But I took your advice initially creating invalid inds. > > > > > > ... > > > > > > > That gave me the idea to layer CIC on top of Reindex, since I > think it does > > > > > > > exactly what's needed. > > > > > > On Sat, Sep 26, 2020 at 02:56:55PM -0500, Justin Pryzby wrote: > > > > > > > On Thu, Sep 24, 2020 at 05:11:03PM +0900, Michael Paquier > wrote: > > > > > > > > It would be good also to check if > > > > > > > > we have a partition index tree that maps partially with a > partition > > > > > > > > table tree (aka no all table partitions have a partition > index), where > > > > > > > > these don't get clustered because there is no index to work > on. > > > > > > > This should not happen, since a incomplete partitioned index > is "invalid". > > > > > > > > > I had been waiting to rebase since there hasn't been any > review comments and I > > > > > > > expected additional, future conflicts. > > > > > > > > > > > I attempted to review this feature, but the last patch conflicts with the > > recent refactoring, so I wasn't able to test it properly. > > Could you please send a new version? > > I rebased this yesterday, so here's my latest. > > > 2) Here we access relation field after closing the relation. Is it safe? > > /* save lockrelid and locktag for below */ > > heaprelid = rel->rd_lockInfo.lockRelId; > > Thanks, fixed this just now. > > > 3) leaf_partitions() function only handles indexes, so I suggest to name > it > > more specifically and add a comment about meaning of 'options' parameter. > > > > 4) I don't quite understand the idea of the regression test. Why do we > > expect to see invalid indexes there? > > +"idxpart_a_idx1" UNIQUE, btree (a) INVALID > > Because of the unique failure: > +create unique index concurrently on idxpart (a); -- partitioned, unique > failure > +ERROR: could not create unique index "idxpart2_a_idx2_ccnew" > +DETAIL: Key (a)=(10) is duplicated. > +\d idxpart > > This shows that CIC first creates catalog-only INVALID indexes, and then > reindexes them to "validate". > > -- > Justin >
Re: CREATE INDEX CONCURRENTLY on partitioned index
On Mon, Feb 15, 2021 at 10:06:47PM +0300, Anastasia Lubennikova wrote: > On 28.01.2021 17:30, Justin Pryzby wrote: > > On Thu, Jan 28, 2021 at 09:51:51PM +0900, Masahiko Sawada wrote: > > > On Mon, Nov 30, 2020 at 5:22 AM Justin Pryzby > > > wrote: > > > > On Sat, Oct 31, 2020 at 01:31:17AM -0500, Justin Pryzby wrote: > > > > > Forking this thread, since the existing CFs have been closed. > > > > > https://www.postgresql.org/message-id/flat/20200914143102.GX18552%40telsasoft.com#58b1056488451f8594b0f0ba40996afd > > > > > > > > > > The strategy is to create catalog entries for all tables with > > > > > indisvalid=false, > > > > > and then process them like REINDEX CONCURRENTLY. If it's > > > > > interrupted, it > > > > > leaves INVALID indexes, which can be cleaned up with DROP or REINDEX, > > > > > same as > > > > > CIC on a plain table. > > > > > > > > > > On Sat, Aug 08, 2020 at 01:37:44AM -0500, Justin Pryzby wrote: > > > > > > On Mon, Jun 15, 2020 at 09:37:42PM +0900, Michael Paquier wrote: > > > > > > Note that the mentioned problem wasn't serious: there was missing > > > > > > index on > > > > > > child table, therefor the parent index was invalid, as intended. > > > > > > However I > > > > > > agree that it's not nice that the command can fail so easily and > > > > > > leave behind > > > > > > some indexes created successfully and some failed some not created > > > > > > at all. > > > > > > > > > > > > But I took your advice initially creating invalid inds. > > > > > ... > > > > > > That gave me the idea to layer CIC on top of Reindex, since I think > > > > > > it does > > > > > > exactly what's needed. > > > > > On Sat, Sep 26, 2020 at 02:56:55PM -0500, Justin Pryzby wrote: > > > > > > On Thu, Sep 24, 2020 at 05:11:03PM +0900, Michael Paquier wrote: > > > > > > > It would be good also to check if > > > > > > > we have a partition index tree that maps partially with a > > > > > > > partition > > > > > > > table tree (aka no all table partitions have a partition index), > > > > > > > where > > > > > > > these don't get clustered because there is no index to work on. > > > > > > This should not happen, since a incomplete partitioned index is > > > > > > "invalid". > > > > > > > I had been waiting to rebase since there hasn't been any review > > > > > > comments and I > > > > > > expected additional, future conflicts. > > > > > > > > I attempted to review this feature, but the last patch conflicts with the > recent refactoring, so I wasn't able to test it properly. > Could you please send a new version? I rebased this yesterday, so here's my latest. > 2) Here we access relation field after closing the relation. Is it safe? > /* save lockrelid and locktag for below */ > heaprelid = rel->rd_lockInfo.lockRelId; Thanks, fixed this just now. > 3) leaf_partitions() function only handles indexes, so I suggest to name it > more specifically and add a comment about meaning of 'options' parameter. > > 4) I don't quite understand the idea of the regression test. Why do we > expect to see invalid indexes there? > + "idxpart_a_idx1" UNIQUE, btree (a) INVALID Because of the unique failure: +create unique index concurrently on idxpart (a); -- partitioned, unique failure +ERROR: could not create unique index "idxpart2_a_idx2_ccnew" +DETAIL: Key (a)=(10) is duplicated. +\d idxpart This shows that CIC first creates catalog-only INVALID indexes, and then reindexes them to "validate". -- Justin >From c846ddfc287bfeddb9b389de1869aadf7173c068 Mon Sep 17 00:00:00 2001 From: Justin Pryzby Date: Sat, 6 Jun 2020 17:42:23 -0500 Subject: [PATCH v13 01/18] Allow CREATE INDEX CONCURRENTLY on partitioned table Note, this effectively reverts 050098b14, so take care to not reintroduce the bug it fixed. --- doc/src/sgml/ref/create_index.sgml | 9 -- src/backend/commands/indexcmds.c | 143 ++--- src/test/regress/expected/indexing.out | 60 ++- src/test/regress/sql/indexing.sql | 18 +++- 4 files changed, 176 insertions(+), 54 deletions(-) diff --git a/doc/src/sgml/ref/create_index.sgml b/doc/src/sgml/ref/create_index.sgml index a5271a9f8f..6869a18968 100644 --- a/doc/src/sgml/ref/create_index.sgml +++ b/doc/src/sgml/ref/create_index.sgml @@ -686,15 +686,6 @@ Indexes: cannot. - -Concurrent builds for indexes on partitioned tables are currently not -supported. However, you may concurrently build the index on each -partition individually and then finally create the partitioned index -non-concurrently in order to reduce the time where writes to the -partitioned table will be locked out. In this case, building the -partitioned index is a metadata only operation. - - diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c index 127ba7835d..4ac1dacd7d 100644 --- a/src/backend/commands/indexcmds.c +++ b/src/backend/commands/indexcmds.c @@ -68,6 +68,7 @@ /* non-export f
Re: CREATE INDEX CONCURRENTLY on partitioned index
On 28.01.2021 17:30, Justin Pryzby wrote: On Thu, Jan 28, 2021 at 09:51:51PM +0900, Masahiko Sawada wrote: On Mon, Nov 30, 2020 at 5:22 AM Justin Pryzby wrote: On Sat, Oct 31, 2020 at 01:31:17AM -0500, Justin Pryzby wrote: Forking this thread, since the existing CFs have been closed. https://www.postgresql.org/message-id/flat/20200914143102.GX18552%40telsasoft.com#58b1056488451f8594b0f0ba40996afd The strategy is to create catalog entries for all tables with indisvalid=false, and then process them like REINDEX CONCURRENTLY. If it's interrupted, it leaves INVALID indexes, which can be cleaned up with DROP or REINDEX, same as CIC on a plain table. On Sat, Aug 08, 2020 at 01:37:44AM -0500, Justin Pryzby wrote: On Mon, Jun 15, 2020 at 09:37:42PM +0900, Michael Paquier wrote: Note that the mentioned problem wasn't serious: there was missing index on child table, therefor the parent index was invalid, as intended. However I agree that it's not nice that the command can fail so easily and leave behind some indexes created successfully and some failed some not created at all. But I took your advice initially creating invalid inds. ... That gave me the idea to layer CIC on top of Reindex, since I think it does exactly what's needed. On Sat, Sep 26, 2020 at 02:56:55PM -0500, Justin Pryzby wrote: On Thu, Sep 24, 2020 at 05:11:03PM +0900, Michael Paquier wrote: It would be good also to check if we have a partition index tree that maps partially with a partition table tree (aka no all table partitions have a partition index), where these don't get clustered because there is no index to work on. This should not happen, since a incomplete partitioned index is "invalid". I had been waiting to rebase since there hasn't been any review comments and I expected additional, future conflicts. I attempted to review this feature, but the last patch conflicts with the recent refactoring, so I wasn't able to test it properly. Could you please send a new version? Meanwhile, here are my questions about the patch: 1) I don't see a reason to change the logic here. We don't skip counting existing indexes when create parent index. Why should we skip them in CONCURRENTLY mode? // If concurrent, maybe this should be done after excluding indexes which already exist ? pgstat_progress_update_param(PROGRESS_CREATEIDX_PARTITIONS_TOTAL, nparts); 2) Here we access relation field after closing the relation. Is it safe? /* save lockrelid and locktag for below */ heaprelid = rel->rd_lockInfo.lockRelId; 3) leaf_partitions() function only handles indexes, so I suggest to name it more specifically and add a comment about meaning of 'options' parameter. 4) I don't quite understand the idea of the regression test. Why do we expect to see invalid indexes there? + "idxpart_a_idx1" UNIQUE, btree (a) INVALID 5) Speaking of documentation, I think we need to add a paragraph about CIC on partitioned indexes which will explain that invalid indexes may appear and what user should do to fix them. 6) ReindexIndexesConcurrently() needs some code cleanup. -- Anastasia Lubennikova Postgres Professional: http://www.postgrespro.com The Russian Postgres Company
Re: CREATE INDEX CONCURRENTLY on partitioned index
On 28.01.2021 17:30, Justin Pryzby wrote: On Thu, Jan 28, 2021 at 09:51:51PM +0900, Masahiko Sawada wrote: On Mon, Nov 30, 2020 at 5:22 AM Justin Pryzby wrote: On Sat, Oct 31, 2020 at 01:31:17AM -0500, Justin Pryzby wrote: Forking this thread, since the existing CFs have been closed. https://www.postgresql.org/message-id/flat/20200914143102.GX18552%40telsasoft.com#58b1056488451f8594b0f0ba40996afd The strategy is to create catalog entries for all tables with indisvalid=false, and then process them like REINDEX CONCURRENTLY. If it's interrupted, it leaves INVALID indexes, which can be cleaned up with DROP or REINDEX, same as CIC on a plain table. On Sat, Aug 08, 2020 at 01:37:44AM -0500, Justin Pryzby wrote: On Mon, Jun 15, 2020 at 09:37:42PM +0900, Michael Paquier wrote: Note that the mentioned problem wasn't serious: there was missing index on child table, therefor the parent index was invalid, as intended. However I agree that it's not nice that the command can fail so easily and leave behind some indexes created successfully and some failed some not created at all. But I took your advice initially creating invalid inds. ... That gave me the idea to layer CIC on top of Reindex, since I think it does exactly what's needed. On Sat, Sep 26, 2020 at 02:56:55PM -0500, Justin Pryzby wrote: On Thu, Sep 24, 2020 at 05:11:03PM +0900, Michael Paquier wrote: It would be good also to check if we have a partition index tree that maps partially with a partition table tree (aka no all table partitions have a partition index), where these don't get clustered because there is no index to work on. This should not happen, since a incomplete partitioned index is "invalid". I had been waiting to rebase since there hasn't been any review comments and I expected additional, future conflicts. I attempted to review this feature, but the last patch conflicts with the recent refactoring, so I wasn't able to test it properly. Could you please send a new version? Meanwhile, here are my questions about the patch: 1) I don't see a reason to change the logic here. We don't skip counting existing indexes when create parent index. Why should we skip them in CONCURRENTLY mode? // If concurrent, maybe this should be done after excluding indexes which already exist ? pgstat_progress_update_param(PROGRESS_CREATEIDX_PARTITIONS_TOTAL, nparts); 2) Here we access relation field after closing the relation. Is it safe? /* save lockrelid and locktag for below */ heaprelid = rel->rd_lockInfo.lockRelId; 3) leaf_partitions() function only handles indexes, so I suggest to name it more specifically and add a comment about meaning of 'options' parameter. 4) I don't quite understand the idea of the regression test. Why do we expect to see invalid indexes there? + "idxpart_a_idx1" UNIQUE, btree (a) INVALID 5) Speaking of documentation, I think we need to add a paragraph about CIC on partitioned indexes which will explain that invalid indexes may appear and what user should do to fix them. 6) ReindexIndexesConcurrently() needs some code cleanup. -- Anastasia Lubennikova Postgres Professional: http://www.postgrespro.com The Russian Postgres Company
Re: CREATE INDEX CONCURRENTLY on partitioned index
On Thu, Jan 28, 2021 at 09:51:51PM +0900, Masahiko Sawada wrote:
> On Mon, Nov 30, 2020 at 5:22 AM Justin Pryzby wrote:
> > On Sat, Oct 31, 2020 at 01:31:17AM -0500, Justin Pryzby wrote:
> > > Forking this thread, since the existing CFs have been closed.
> > > https://www.postgresql.org/message-id/flat/20200914143102.GX18552%40telsasoft.com#58b1056488451f8594b0f0ba40996afd
> > >
> > > The strategy is to create catalog entries for all tables with
> > > indisvalid=false,
> > > and then process them like REINDEX CONCURRENTLY. If it's interrupted, it
> > > leaves INVALID indexes, which can be cleaned up with DROP or REINDEX,
> > > same as
> > > CIC on a plain table.
> > >
> > > On Sat, Aug 08, 2020 at 01:37:44AM -0500, Justin Pryzby wrote:
> > > > On Mon, Jun 15, 2020 at 09:37:42PM +0900, Michael Paquier wrote:
> > > > Note that the mentioned problem wasn't serious: there was missing index
> > > > on
> > > > child table, therefor the parent index was invalid, as intended.
> > > > However I
> > > > agree that it's not nice that the command can fail so easily and leave
> > > > behind
> > > > some indexes created successfully and some failed some not created at
> > > > all.
> > > >
> > > > But I took your advice initially creating invalid inds.
> > > ...
> > > > That gave me the idea to layer CIC on top of Reindex, since I think it
> > > > does
> > > > exactly what's needed.
> > >
> > > On Sat, Sep 26, 2020 at 02:56:55PM -0500, Justin Pryzby wrote:
> > > > On Thu, Sep 24, 2020 at 05:11:03PM +0900, Michael Paquier wrote:
> > > > > It would be good also to check if
> > > > > we have a partition index tree that maps partially with a partition
> > > > > table tree (aka no all table partitions have a partition index), where
> > > > > these don't get clustered because there is no index to work on.
> > > >
> > > > This should not happen, since a incomplete partitioned index is
> > > > "invalid".
> >
> > @cfbot: rebased over recent changes to indexcmds.c
>
> Status update for a commitfest entry.
>
> This patch has not been updated and "Waiting on Author" status since
> Nov 30. Are you still planning to work on this, Justin? If no, I'm
> going to set this entry to "Returned with Feedback" barring
> objections.
I had been waiting to rebase since there hasn't been any review comments and I
expected additional, future conflicts.
--
Justin
>From 2840a6d355961ea6fdd29c2851b9c333c17c849f Mon Sep 17 00:00:00 2001
From: Justin Pryzby
Date: Sat, 6 Jun 2020 17:42:23 -0500
Subject: [PATCH v12 1/5] Allow CREATE INDEX CONCURRENTLY on partitioned table
Note, this effectively reverts 050098b14, so take care to not reintroduce the
bug it fixed.
XXX: does pgstat_progress_update_param() break other commands progress ?
---
doc/src/sgml/ref/create_index.sgml | 9 --
src/backend/commands/indexcmds.c | 142 ++---
src/test/regress/expected/indexing.out | 60 ++-
src/test/regress/sql/indexing.sql | 18 +++-
4 files changed, 173 insertions(+), 56 deletions(-)
diff --git a/doc/src/sgml/ref/create_index.sgml b/doc/src/sgml/ref/create_index.sgml
index a5271a9f8f..6869a18968 100644
--- a/doc/src/sgml/ref/create_index.sgml
+++ b/doc/src/sgml/ref/create_index.sgml
@@ -686,15 +686,6 @@ Indexes:
cannot.
-
-Concurrent builds for indexes on partitioned tables are currently not
-supported. However, you may concurrently build the index on each
-partition individually and then finally create the partitioned index
-non-concurrently in order to reduce the time where writes to the
-partitioned table will be locked out. In this case, building the
-partitioned index is a metadata only operation.
-
-
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index f9f3ff3b62..c513e8a6bd 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -68,6 +68,7 @@
/* non-export function prototypes */
+static void reindex_invalid_child_indexes(Oid indexRelationId);
static bool CompareOpclassOptions(Datum *opts1, Datum *opts2, int natts);
static void CheckPredicate(Expr *predicate);
static void ComputeIndexAttrs(IndexInfo *indexInfo,
@@ -680,17 +681,6 @@ DefineIndex(Oid relationId,
partitioned = rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE;
if (partitioned)
{
- /*
- * Note: we check 'stmt->concurrent' rather than 'concurrent', so that
- * the error is thrown also for temporary tables. Seems better to be
- * consistent, even though we could do it on temporary table because
- * we're not actually doing it concurrently.
- */
- if (stmt->concurrent)
- ereport(ERROR,
- (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("cannot create index on partitioned table \"%s\" concurrently",
- RelationGetRelationName(rel;
if (stmt->excludeOpNames)
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
@@ -1128,6 +1118,11 @@ DefineIndex(Oid relationId,
Re: CREATE INDEX CONCURRENTLY on partitioned index
On Mon, Nov 30, 2020 at 5:22 AM Justin Pryzby wrote: > > On Sat, Oct 31, 2020 at 01:31:17AM -0500, Justin Pryzby wrote: > > Forking this thread, since the existing CFs have been closed. > > https://www.postgresql.org/message-id/flat/20200914143102.GX18552%40telsasoft.com#58b1056488451f8594b0f0ba40996afd > > > > The strategy is to create catalog entries for all tables with > > indisvalid=false, > > and then process them like REINDEX CONCURRENTLY. If it's interrupted, it > > leaves INVALID indexes, which can be cleaned up with DROP or REINDEX, same > > as > > CIC on a plain table. > > > > On Sat, Aug 08, 2020 at 01:37:44AM -0500, Justin Pryzby wrote: > > > On Mon, Jun 15, 2020 at 09:37:42PM +0900, Michael Paquier wrote: > > > > On Mon, Jun 15, 2020 at 08:15:05PM +0800, 李杰(慎追) wrote: > > > > > As shown above, an error occurred while creating an index in the > > > > > second partition. > > > > > It can be clearly seen that the index of the partitioned table is > > > > > invalid > > > > > and the index of the first partition is normal, the second partition > > > > > is invalid, > > > > > and the Third Partition index does not exist at all. > > > > > > > > That's a problem. I really think that we should make the steps of the > > > > concurrent operation consistent across all relations, meaning that all > > > > the indexes should be created as invalid for all the parts of the > > > > partition tree, including partitioned tables as well as their > > > > partitions, in the same transaction. Then a second new transaction > > > > gets used for the index build, followed by a third one for the > > > > validation that switches the indexes to become valid. > > > > > > Note that the mentioned problem wasn't serious: there was missing index on > > > child table, therefor the parent index was invalid, as intended. However > > > I > > > agree that it's not nice that the command can fail so easily and leave > > > behind > > > some indexes created successfully and some failed some not created at all. > > > > > > But I took your advice initially creating invalid inds. > > ... > > > That gave me the idea to layer CIC on top of Reindex, since I think it > > > does > > > exactly what's needed. > > > > On Sat, Sep 26, 2020 at 02:56:55PM -0500, Justin Pryzby wrote: > > > On Thu, Sep 24, 2020 at 05:11:03PM +0900, Michael Paquier wrote: > > > > It would be good also to check if > > > > we have a partition index tree that maps partially with a partition > > > > table tree (aka no all table partitions have a partition index), where > > > > these don't get clustered because there is no index to work on. > > > > > > This should not happen, since a incomplete partitioned index is "invalid". > > @cfbot: rebased over recent changes to indexcmds.c Status update for a commitfest entry. This patch has not been updated and "Waiting on Author" status since Nov 30. Are you still planning to work on this, Justin? If no, I'm going to set this entry to "Returned with Feedback" barring objections. Regards, -- Masahiko Sawada EDB: https://www.enterprisedb.com/
Re: CREATE INDEX CONCURRENTLY on partitioned index
On Sat, Oct 31, 2020 at 01:31:17AM -0500, Justin Pryzby wrote:
> Forking this thread, since the existing CFs have been closed.
> https://www.postgresql.org/message-id/flat/20200914143102.GX18552%40telsasoft.com#58b1056488451f8594b0f0ba40996afd
>
> The strategy is to create catalog entries for all tables with
> indisvalid=false,
> and then process them like REINDEX CONCURRENTLY. If it's interrupted, it
> leaves INVALID indexes, which can be cleaned up with DROP or REINDEX, same as
> CIC on a plain table.
>
> On Sat, Aug 08, 2020 at 01:37:44AM -0500, Justin Pryzby wrote:
> > On Mon, Jun 15, 2020 at 09:37:42PM +0900, Michael Paquier wrote:
> > > On Mon, Jun 15, 2020 at 08:15:05PM +0800, 李杰(慎追) wrote:
> > > > As shown above, an error occurred while creating an index in the second
> > > > partition.
> > > > It can be clearly seen that the index of the partitioned table is
> > > > invalid
> > > > and the index of the first partition is normal, the second partition is
> > > > invalid,
> > > > and the Third Partition index does not exist at all.
> > >
> > > That's a problem. I really think that we should make the steps of the
> > > concurrent operation consistent across all relations, meaning that all
> > > the indexes should be created as invalid for all the parts of the
> > > partition tree, including partitioned tables as well as their
> > > partitions, in the same transaction. Then a second new transaction
> > > gets used for the index build, followed by a third one for the
> > > validation that switches the indexes to become valid.
> >
> > Note that the mentioned problem wasn't serious: there was missing index on
> > child table, therefor the parent index was invalid, as intended. However I
> > agree that it's not nice that the command can fail so easily and leave
> > behind
> > some indexes created successfully and some failed some not created at all.
> >
> > But I took your advice initially creating invalid inds.
> ...
> > That gave me the idea to layer CIC on top of Reindex, since I think it does
> > exactly what's needed.
>
> On Sat, Sep 26, 2020 at 02:56:55PM -0500, Justin Pryzby wrote:
> > On Thu, Sep 24, 2020 at 05:11:03PM +0900, Michael Paquier wrote:
> > > It would be good also to check if
> > > we have a partition index tree that maps partially with a partition
> > > table tree (aka no all table partitions have a partition index), where
> > > these don't get clustered because there is no index to work on.
> >
> > This should not happen, since a incomplete partitioned index is "invalid".
@cfbot: rebased over recent changes to indexcmds.c
--
Justin
>From 05cd4feaa5a1d77a1ae8e3a167a68b2a21af1fc6 Mon Sep 17 00:00:00 2001
From: Justin Pryzby
Date: Sat, 6 Jun 2020 17:42:23 -0500
Subject: [PATCH v11 1/9] Allow CREATE INDEX CONCURRENTLY on partitioned table
Note, this effectively reverts 050098b14, so take care to not reintroduce the
bug it fixed.
XXX: does pgstat_progress_update_param() break other commands progress ?
---
doc/src/sgml/ref/create_index.sgml | 9 --
src/backend/commands/indexcmds.c | 141 ++---
src/test/regress/expected/indexing.out | 60 ++-
src/test/regress/sql/indexing.sql | 18 +++-
4 files changed, 172 insertions(+), 56 deletions(-)
diff --git a/doc/src/sgml/ref/create_index.sgml b/doc/src/sgml/ref/create_index.sgml
index 29dee5689e..bd4431a3ce 100644
--- a/doc/src/sgml/ref/create_index.sgml
+++ b/doc/src/sgml/ref/create_index.sgml
@@ -661,15 +661,6 @@ Indexes:
cannot.
-
-Concurrent builds for indexes on partitioned tables are currently not
-supported. However, you may concurrently build the index on each
-partition individually and then finally create the partitioned index
-non-concurrently in order to reduce the time where writes to the
-partitioned table will be locked out. In this case, building the
-partitioned index is a metadata only operation.
-
-
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index ca24620fd0..76219381c1 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -68,6 +68,7 @@
/* non-export function prototypes */
+static void reindex_invalid_child_indexes(Oid indexRelationId);
static bool CompareOpclassOptions(Datum *opts1, Datum *opts2, int natts);
static void CheckPredicate(Expr *predicate);
static void ComputeIndexAttrs(IndexInfo *indexInfo,
@@ -671,17 +672,6 @@ DefineIndex(Oid relationId,
partitioned = rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE;
if (partitioned)
{
- /*
- * Note: we check 'stmt->concurrent' rather than 'concurrent', so that
- * the error is thrown also for temporary tables. Seems better to be
- * consistent, even though we could do it on temporary table because
- * we're not actually doing it concurrently.
- */
- if (stmt->concurrent)
- ereport(ERROR,
- (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("cannot
