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 <pry...@telsasoft.com>
> > > 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 <pryz...@telsasoft.com>
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.
</para>
- <para>
- 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.
- </para>
-
</refsect2>
</refsect1>
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 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(oldcontext);
+
/*
* We can't use the same index name for the child index,
* so clear idxname to let the recursive invocation choose
@@ -1366,10 +1377,18 @@ DefineIndex(Oid relationId,
createdConstraintId,
is_alter_table, check_rights, check_not_in_use,
skip_build, quiet);
+ if (concurrent)
+ {
+ PopActiveSnapshot();
+ PushActiveSnapshot(GetTransactionSnapshot());
+ invalidate_parent = true;
+ }
}
- pgstat_progress_update_param(PROGRESS_CREATEIDX_PARTITIONS_DONE,
- i + 1);
+ /* For concurrent build, this is a catalog-only stage */
+ if (!concurrent)
+ pgstat_progress_update_param(PROGRESS_CREATEIDX_PARTITIONS_DONE,
+ i + 1);
free_attrmap(attmap);
}
@@ -1379,41 +1398,33 @@ DefineIndex(Oid relationId,
* invalid, this is incorrect, so update our row to invalid too.
*/
if (invalidate_parent)
- {
- Relation pg_index = table_open(IndexRelationId, RowExclusiveLock);
- HeapTuple tup,
- newtup;
-
- tup = SearchSysCache1(INDEXRELID,
- ObjectIdGetDatum(indexRelationId));
- if (!HeapTupleIsValid(tup))
- elog(ERROR, "cache lookup failed for index %u",
- indexRelationId);
- newtup = heap_copytuple(tup);
- ((Form_pg_index) GETSTRUCT(newtup))->indisvalid = false;
- CatalogTupleUpdate(pg_index, &tup->t_self, newtup);
- ReleaseSysCache(tup);
- table_close(pg_index, RowExclusiveLock);
- heap_freetuple(newtup);
- }
- }
+ index_set_state_flags(indexRelationId, INDEX_DROP_CLEAR_VALID);
+ } else
+ table_close(rel, NoLock);
/*
* Indexes on partitioned tables are not themselves built, so we're
* done here.
*/
- table_close(rel, NoLock);
if (!OidIsValid(parentIndexId))
+ {
+ if (concurrent)
+ reindex_invalid_child_indexes(indexRelationId);
+
pgstat_progress_end_command();
+ }
+
return address;
}
- if (!concurrent)
+ if (!concurrent || OidIsValid(parentIndexId))
{
- /* Close the heap and we're done, in the non-concurrent case */
- table_close(rel, NoLock);
+ /*
+ * We're done if this is the top-level index,
+ * or the catalog-only phase of a partition built concurrently
+ */
- /* If this is the top-level index, we're done. */
+ table_close(rel, NoLock);
if (!OidIsValid(parentIndexId))
pgstat_progress_end_command();
@@ -1617,6 +1628,62 @@ DefineIndex(Oid relationId,
return address;
}
+/* Reindex invalid child indexes created earlier */
+static void
+reindex_invalid_child_indexes(Oid indexRelationId)
+{
+ ListCell *lc;
+ int npart = 0;
+ ReindexParams params = {
+ .options = REINDEXOPT_CONCURRENTLY
+ };
+
+ MemoryContext ind_context = AllocSetContextCreate(PortalContext, "CREATE INDEX",
+ ALLOCSET_DEFAULT_SIZES);
+ MemoryContext oldcontext;
+ List *childs = find_inheritance_children(indexRelationId, ShareLock);
+ List *partitions = NIL;
+
+ PreventInTransactionBlock(true, "REINDEX INDEX");
+
+ foreach (lc, childs)
+ {
+ Oid partoid = lfirst_oid(lc);
+
+ /* XXX: need to retrofit progress reporting into it */
+ // pgstat_progress_update_param(PROGRESS_CREATEIDX_PARTITIONS_DONE,
+ // npart++);
+
+ if (get_index_isvalid(partoid) ||
+ !RELKIND_HAS_STORAGE(get_rel_relkind(partoid)))
+ continue;
+
+ /* Save partition OID */
+ oldcontext = MemoryContextSwitchTo(ind_context);
+ partitions = lappend_oid(partitions, partoid);
+ MemoryContextSwitchTo(oldcontext);
+ }
+
+ /*
+ * Process each partition listed in a separate transaction. Note that
+ * this commits and then starts a new transaction immediately.
+ * XXX: since this is done in 2*N transactions, it could just as well
+ * call ReindexRelationConcurrently directly
+ */
+ ReindexMultipleInternal(partitions, ¶ms);
+
+ /*
+ * CIC needs to mark a partitioned index as VALID, which itself
+ * requires setting READY, which is unset for CIC (even though
+ * it's meaningless for an index without storage).
+ * This must be done only while holding a lock which precludes adding
+ * partitions.
+ * See also: validatePartitionedIndex().
+ */
+ index_set_state_flags(indexRelationId, INDEX_CREATE_SET_READY);
+ CommandCounterIncrement();
+ index_set_state_flags(indexRelationId, INDEX_CREATE_SET_VALID);
+}
/*
* CheckMutability
diff --git a/src/test/regress/expected/indexing.out b/src/test/regress/expected/indexing.out
index c93f4470c9..f04abc6897 100644
--- a/src/test/regress/expected/indexing.out
+++ b/src/test/regress/expected/indexing.out
@@ -50,11 +50,63 @@ select relname, relkind, relhassubclass, inhparent::regclass
(8 rows)
drop table idxpart;
--- Some unsupported features
+-- CIC on partitioned table
create table idxpart (a int, b int, c text) partition by range (a);
-create table idxpart1 partition of idxpart for values from (0) to (10);
-create index concurrently on idxpart (a);
-ERROR: cannot create index on partitioned table "idxpart" concurrently
+create table idxpart1 partition of idxpart for values from (0) to (10) partition by range(a);
+create table idxpart11 partition of idxpart1 for values from (0) to (10) partition by range(a);
+create table idxpart111 partition of idxpart11 default partition by range(a);
+create table idxpart1111 partition of idxpart111 default partition by range(a);
+create table idxpart2 partition of idxpart for values from (10) to (20);
+insert into idxpart2 values(10),(10); -- not unique
+create index concurrently on idxpart (a); -- partitioned
+create index concurrently on idxpart1 (a); -- partitioned and partition
+create index concurrently on idxpart11 (a); -- partitioned and partition, with no leaves
+create index concurrently on idxpart2 (a); -- leaf
+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
+ Partitioned table "public.idxpart"
+ Column | Type | Collation | Nullable | Default
+--------+---------+-----------+----------+---------
+ a | integer | | |
+ b | integer | | |
+ c | text | | |
+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.)
+
+\d idxpart1
+ Partitioned table "public.idxpart1"
+ Column | Type | Collation | Nullable | Default
+--------+---------+-----------+----------+---------
+ a | integer | | |
+ b | integer | | |
+ c | text | | |
+Partition of: idxpart FOR VALUES FROM (0) TO (10)
+Partition key: RANGE (a)
+Indexes:
+ "idxpart1_a_idx" btree (a) INVALID
+ "idxpart1_a_idx1" btree (a)
+ "idxpart1_a_idx2" UNIQUE, btree (a) INVALID
+Number of partitions: 1 (Use \d+ to list them.)
+
+\d idxpart2
+ Table "public.idxpart2"
+ Column | Type | Collation | Nullable | Default
+--------+---------+-----------+----------+---------
+ a | integer | | |
+ b | integer | | |
+ c | text | | |
+Partition of: idxpart FOR VALUES FROM (10) TO (20)
+Indexes:
+ "idxpart2_a_idx" btree (a)
+ "idxpart2_a_idx1" btree (a)
+ "idxpart2_a_idx2" UNIQUE, btree (a) INVALID
+ "idxpart2_a_idx2_ccnew" UNIQUE, btree (a) INVALID
+
drop table idxpart;
-- Verify bugfix with query on indexed partitioned table with no partitions
-- https://postgr.es/m/20180124162006.pmapfiznhgngwtjf@alvherre.pgsql
diff --git a/src/test/regress/sql/indexing.sql b/src/test/regress/sql/indexing.sql
index 42f398b67c..3d4b6e9bc9 100644
--- a/src/test/regress/sql/indexing.sql
+++ b/src/test/regress/sql/indexing.sql
@@ -29,10 +29,22 @@ select relname, relkind, relhassubclass, inhparent::regclass
where relname like 'idxpart%' order by relname;
drop table idxpart;
--- Some unsupported features
+-- CIC on partitioned table
create table idxpart (a int, b int, c text) partition by range (a);
-create table idxpart1 partition of idxpart for values from (0) to (10);
-create index concurrently on idxpart (a);
+create table idxpart1 partition of idxpart for values from (0) to (10) partition by range(a);
+create table idxpart11 partition of idxpart1 for values from (0) to (10) partition by range(a);
+create table idxpart111 partition of idxpart11 default partition by range(a);
+create table idxpart1111 partition of idxpart111 default partition by range(a);
+create table idxpart2 partition of idxpart for values from (10) to (20);
+insert into idxpart2 values(10),(10); -- not unique
+create index concurrently on idxpart (a); -- partitioned
+create index concurrently on idxpart1 (a); -- partitioned and partition
+create index concurrently on idxpart11 (a); -- partitioned and partition, with no leaves
+create index concurrently on idxpart2 (a); -- leaf
+create unique index concurrently on idxpart (a); -- partitioned, unique failure
+\d idxpart
+\d idxpart1
+\d idxpart2
drop table idxpart;
-- Verify bugfix with query on indexed partitioned table with no partitions
--
2.17.0
>From 6cbf4b5f0e3f795e0253896b13d22a67e513b602 Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryz...@telsasoft.com>
Date: Sun, 14 Feb 2021 18:31:43 -0600
Subject: [PATCH v13 02/18] f! progress reporting
---
src/backend/commands/indexcmds.c | 33 +++++++-------------------------
1 file changed, 7 insertions(+), 26 deletions(-)
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 4ac1dacd7d..d6567ec231 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -1638,40 +1638,20 @@ reindex_invalid_child_indexes(Oid indexRelationId)
.options = REINDEXOPT_CONCURRENTLY
};
- MemoryContext ind_context = AllocSetContextCreate(PortalContext, "CREATE INDEX",
- ALLOCSET_DEFAULT_SIZES);
- MemoryContext oldcontext;
- List *childs = find_inheritance_children(indexRelationId, ShareLock);
- List *partitions = NIL;
-
PreventInTransactionBlock(true, "REINDEX INDEX");
- foreach (lc, childs)
+ foreach (lc, find_inheritance_children(indexRelationId, ShareLock))
{
Oid partoid = lfirst_oid(lc);
- /* XXX: need to retrofit progress reporting into it */
- // pgstat_progress_update_param(PROGRESS_CREATEIDX_PARTITIONS_DONE,
- // npart++);
-
- if (get_index_isvalid(partoid) ||
- !RELKIND_HAS_STORAGE(get_rel_relkind(partoid)))
- continue;
+ if (!get_index_isvalid(partoid) &&
+ RELKIND_HAS_STORAGE(get_rel_relkind(partoid)))
+ ReindexRelationConcurrently(partoid, ¶ms);
- /* Save partition OID */
- oldcontext = MemoryContextSwitchTo(ind_context);
- partitions = lappend_oid(partitions, partoid);
- MemoryContextSwitchTo(oldcontext);
+ pgstat_progress_update_param(PROGRESS_CREATEIDX_PARTITIONS_DONE,
+ npart++);
}
- /*
- * Process each partition listed in a separate transaction. Note that
- * this commits and then starts a new transaction immediately.
- * XXX: since this is done in 2*N transactions, it could just as well
- * call ReindexRelationConcurrently directly
- */
- ReindexMultipleInternal(partitions, ¶ms);
-
/*
* CIC needs to mark a partitioned index as VALID, which itself
* requires setting READY, which is unset for CIC (even though
@@ -1680,6 +1660,7 @@ reindex_invalid_child_indexes(Oid indexRelationId)
* partitions.
* See also: validatePartitionedIndex().
*/
+ CommandCounterIncrement();
index_set_state_flags(indexRelationId, INDEX_CREATE_SET_READY);
CommandCounterIncrement();
index_set_state_flags(indexRelationId, INDEX_CREATE_SET_VALID);
--
2.17.0
>From 6dff5512b07735e51c2c7f82cce5b3bd8b9a6faa Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryz...@telsasoft.com>
Date: Fri, 30 Oct 2020 16:23:02 -0500
Subject: [PATCH v13 03/18] WIP: Add SKIPVALID flag for more integration
XXX: this breaks progress reporting?
---
src/backend/commands/indexcmds.c | 36 +++++++++++++++-----------------
src/include/catalog/index.h | 1 +
2 files changed, 18 insertions(+), 19 deletions(-)
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index d6567ec231..10d4da136f 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -1628,40 +1628,33 @@ DefineIndex(Oid relationId,
return address;
}
-/* Reindex invalid child indexes created earlier */
+/*
+ * Reindex invalid child indexes created earlier thereby validating
+ * the parent index.
+ */
static void
reindex_invalid_child_indexes(Oid indexRelationId)
{
- ListCell *lc;
- int npart = 0;
ReindexParams params = {
- .options = REINDEXOPT_CONCURRENTLY
+ .options = REINDEXOPT_CONCURRENTLY | REINDEXOPT_SKIPVALID
};
- PreventInTransactionBlock(true, "REINDEX INDEX");
-
- foreach (lc, find_inheritance_children(indexRelationId, ShareLock))
- {
- Oid partoid = lfirst_oid(lc);
-
- if (!get_index_isvalid(partoid) &&
- RELKIND_HAS_STORAGE(get_rel_relkind(partoid)))
- ReindexRelationConcurrently(partoid, ¶ms);
-
- pgstat_progress_update_param(PROGRESS_CREATEIDX_PARTITIONS_DONE,
- npart++);
- }
-
/*
* CIC needs to mark a partitioned index as VALID, which itself
* requires setting READY, which is unset for CIC (even though
* it's meaningless for an index without storage).
* This must be done only while holding a lock which precludes adding
* partitions.
- * See also: validatePartitionedIndex().
*/
CommandCounterIncrement();
index_set_state_flags(indexRelationId, INDEX_CREATE_SET_READY);
+
+ /*
+ * Process each partition listed in a separate transaction. Note that
+ * this commits and then starts a new transaction immediately.
+ */
+ ReindexPartitions(indexRelationId, ¶ms, true);
+
CommandCounterIncrement();
index_set_state_flags(indexRelationId, INDEX_CREATE_SET_VALID);
}
@@ -3085,6 +3078,11 @@ ReindexPartitions(Oid relid, ReindexParams *params, bool isTopLevel)
if (!RELKIND_HAS_STORAGE(partkind))
continue;
+ /* Skip invalid indexes, if requested */
+ if ((params->options & REINDEXOPT_SKIPVALID) != 0 &&
+ get_index_isvalid(partoid))
+ continue;
+
Assert(partkind == RELKIND_INDEX ||
partkind == RELKIND_RELATION);
diff --git a/src/include/catalog/index.h b/src/include/catalog/index.h
index e22d506436..994fe94fa1 100644
--- a/src/include/catalog/index.h
+++ b/src/include/catalog/index.h
@@ -42,6 +42,7 @@ typedef struct ReindexParams
#define REINDEXOPT_REPORT_PROGRESS 0x02 /* report pgstat progress */
#define REINDEXOPT_MISSING_OK 0x04 /* skip missing relations */
#define REINDEXOPT_CONCURRENTLY 0x08 /* concurrent mode */
+#define REINDEXOPT_SKIPVALID 0x10 /* skip valid indexes */
/* state info for validate_index bulkdelete callback */
typedef struct ValidateIndexState
--
2.17.0
>From 04d6b45f488d67400f7e6d7beb73ad92c00083e3 Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryz...@telsasoft.com>
Date: Fri, 30 Oct 2020 23:52:31 -0500
Subject: [PATCH v13 04/18] ReindexPartitions() to set indisvalid..
Something like this should probably have been included in
a6642b3ae060976b42830b7dc8f29ec190ab05e4
See also 71a05b223, which mentioned the absence of any way to validate an
index.
---
src/backend/commands/indexcmds.c | 23 ++++++++++++++++++-----
1 file changed, 18 insertions(+), 5 deletions(-)
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 10d4da136f..b21555b6cf 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -1643,8 +1643,6 @@ reindex_invalid_child_indexes(Oid indexRelationId)
* CIC needs to mark a partitioned index as VALID, which itself
* requires setting READY, which is unset for CIC (even though
* it's meaningless for an index without storage).
- * This must be done only while holding a lock which precludes adding
- * partitions.
*/
CommandCounterIncrement();
index_set_state_flags(indexRelationId, INDEX_CREATE_SET_READY);
@@ -1654,9 +1652,6 @@ reindex_invalid_child_indexes(Oid indexRelationId)
* this commits and then starts a new transaction immediately.
*/
ReindexPartitions(indexRelationId, ¶ms, true);
-
- CommandCounterIncrement();
- index_set_state_flags(indexRelationId, INDEX_CREATE_SET_VALID);
}
/*
@@ -3098,6 +3093,24 @@ ReindexPartitions(Oid relid, ReindexParams *params, bool isTopLevel)
*/
ReindexMultipleInternal(partitions, params);
+ /*
+ * If indexes exist on all of the partitioned table's children, and we
+ * just reindexed them, then we know they're valid, and so can mark the
+ * parent index as valid.
+ * This handles the case of CREATE INDEX CONCURRENTLY.
+ * See also: validatePartitionedIndex().
+ */
+ if (get_rel_relkind(relid) == RELKIND_PARTITIONED_INDEX
+ && !get_index_isvalid(relid))
+ {
+ 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 */
+ if (list_length(inhoids) == list_length(child_tables))
+ index_set_state_flags(relid, INDEX_CREATE_SET_VALID);
+ }
+
/*
* Clean up working storage --- note we must do this after
* StartTransactionCommand, else we might be trying to delete the active
--
2.17.0
>From 0ba6f83c6df1140037016eb100f8c85dd13070fe Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryz...@telsasoft.com>
Date: Sun, 1 Nov 2020 12:25:15 -0600
Subject: [PATCH v13 05/18] Refactor to allow reindexing all index partitions
at once..
The utility of this is to reindex N partitions in 2 transactions, rather than
2*N transactions.
---
src/backend/commands/indexcmds.c | 258 ++++++++++++++-------
src/test/regress/expected/create_index.out | 4 +-
2 files changed, 182 insertions(+), 80 deletions(-)
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index b21555b6cf..f5fea14ff4 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -102,6 +102,8 @@ static void ReindexMultipleInternal(List *relids,
ReindexParams *params);
static bool ReindexRelationConcurrently(Oid relationOid,
ReindexParams *params);
+static List *ReindexIndexesConcurrently(List *indexIds, List *heapRelationIds,
+ ReindexParams *params, MemoryContext private_context);
static void update_relispartition(Oid relationId, bool newval);
static inline void set_indexsafe_procflags(void);
@@ -124,6 +126,15 @@ typedef struct ReindexErrorInfo
char relkind;
} ReindexErrorInfo;
+/* Argument to ReindexIndexConcurrently takes a List* of these */
+typedef struct ReindexIndexInfo
+{
+ Oid indexId;
+ Oid tableId;
+ Oid amId;
+ bool safe; /* for set_indexsafe_procflags */
+} ReindexIndexInfo;
+
/*
* CheckIndexCompatible
* Determine whether an existing index definition is compatible with a
@@ -2636,7 +2647,15 @@ ReindexIndex(RangeVar *indexRelation, ReindexParams *params, bool isTopLevel)
ReindexPartitions(indOid, params, isTopLevel);
else if ((params->options & REINDEXOPT_CONCURRENTLY) != 0 &&
persistence != RELPERSISTENCE_TEMP)
- ReindexRelationConcurrently(indOid, params);
+ {
+ ReindexIndexInfo idxinfo = {
+ .indexId = indOid,
+ /* other fields set later */
+ };
+ ReindexIndexesConcurrently(list_make1(&idxinfo),
+ list_make1_oid(IndexGetRelation(indOid, false)),
+ params, CurrentMemoryContext);
+ }
else
{
ReindexParams newparams = *params;
@@ -3002,20 +3021,69 @@ reindex_error_callback(void *arg)
errinfo->relnamespace, errinfo->relname);
}
+
+/*
+ * Given a list of index oids, return a list of leaf partitions by removing
+ * any intermediate parents. heaprels is populated with the corresponding
+ * tables.
+ */
+static List *
+leaf_indexes(List *inhoids, int options, List **heaprels)
+{
+ List *partitions = NIL;
+ ListCell *lc;
+
+ foreach(lc, inhoids)
+ {
+ Oid partoid = lfirst_oid(lc);
+ Oid tableoid;
+ Relation table;
+ char partkind = get_rel_relkind(partoid);
+
+ /*
+ * This discards partitioned indexes and foreign tables.
+ */
+ if (!RELKIND_HAS_STORAGE(partkind))
+ continue;
+
+ Assert(partkind == RELKIND_INDEX);
+
+ /* Skip invalid indexes, if requested */
+ if ((options & REINDEXOPT_SKIPVALID) != 0 &&
+ get_index_isvalid(partoid))
+ continue;
+
+ /* (try to) Open the table, with lock */
+ tableoid = IndexGetRelation(partoid, false);
+ table = table_open(tableoid, ShareLock);
+ table_close(table, NoLock);
+
+ /* Save partition OID in current MemoryContext */
+ partitions = lappend_oid(partitions, partoid);
+ *heaprels = lappend_oid(*heaprels, tableoid);
+ }
+
+ return partitions;
+}
+
+
/*
* ReindexPartitions
*
* Reindex a set of partitions, per the partitioned index or table given
* by the caller.
+ * XXX: should be further refactored with logic from ReindexRelationConcurrently
*/
static void
ReindexPartitions(Oid relid, ReindexParams *params, bool isTopLevel)
{
- List *partitions = NIL;
+ List *partitions = NIL,
+ *heaprels = NIL;
char relkind = get_rel_relkind(relid);
char *relname = get_rel_name(relid);
char *relnamespace = get_namespace_name(get_rel_namespace(relid));
MemoryContext reindex_context;
+ MemoryContext old_context;
List *inhoids;
ListCell *lc;
ErrorContextCallback errcallback;
@@ -3060,38 +3128,58 @@ ReindexPartitions(Oid relid, ReindexParams *params, bool isTopLevel)
* The list of relations to reindex are the physical partitions of the
* tree so discard any partitioned table or index.
*/
- foreach(lc, inhoids)
- {
- Oid partoid = lfirst_oid(lc);
- char partkind = get_rel_relkind(partoid);
- MemoryContext old_context;
-
- /*
- * This discards partitioned tables, partitioned indexes and foreign
- * tables.
- */
- if (!RELKIND_HAS_STORAGE(partkind))
- continue;
- /* Skip invalid indexes, if requested */
- if ((params->options & REINDEXOPT_SKIPVALID) != 0 &&
- get_index_isvalid(partoid))
- continue;
+ if (relkind == RELKIND_PARTITIONED_INDEX)
+ {
+ old_context = MemoryContextSwitchTo(reindex_context);
+ partitions = leaf_indexes(inhoids, params->options, &heaprels);
+ MemoryContextSwitchTo(old_context);
+ } else {
+ /* Loop over parent tables */
+ foreach(lc, inhoids)
+ {
+ Oid partoid = lfirst_oid(lc);
+ Relation parttable;
+ List *partindexes;
+
+ parttable = table_open(partoid, ShareLock);
+ old_context = MemoryContextSwitchTo(reindex_context);
+ partindexes = RelationGetIndexList(parttable);
+ partindexes = leaf_indexes(partindexes, params->options, &heaprels);
+ partitions = list_concat(partitions, partindexes);
+
+ MemoryContextSwitchTo(old_context);
+ table_close(parttable, ShareLock);
+ }
+ }
- Assert(partkind == RELKIND_INDEX ||
- partkind == RELKIND_RELATION);
+ if ((params->options & REINDEXOPT_CONCURRENTLY) != 0 &&
+ relkind == RELKIND_PARTITIONED_INDEX &&
+ get_rel_persistence(relid) != RELPERSISTENCE_TEMP)
+ {
+ List *idxinfos = NIL;
+ ReindexIndexInfo *idxinfo;
- /* Save partition OID */
old_context = MemoryContextSwitchTo(reindex_context);
- partitions = lappend_oid(partitions, partoid);
+ foreach (lc, partitions)
+ {
+ Oid partoid = lfirst_oid(lc);
+ idxinfo = palloc(sizeof(ReindexIndexInfo));
+ idxinfo->indexId = partoid;
+ /* other fields set later */
+ idxinfos = lappend(idxinfos, idxinfo);
+ }
MemoryContextSwitchTo(old_context);
- }
- /*
- * Process each partition listed in a separate transaction. Note that
- * this commits and then starts a new transaction immediately.
- */
- ReindexMultipleInternal(partitions, params);
+ /* Process all indexes in a single loop */
+ ReindexIndexesConcurrently(idxinfos, heaprels, params, reindex_context);
+ } else {
+ /*
+ * Process each partition listed in a separate transaction. Note that
+ * this commits and then starts a new transaction immediately.
+ */
+ ReindexMultipleInternal(partitions, params);
+ }
/*
* If indexes exist on all of the partitioned table's children, and we
@@ -3254,18 +3342,9 @@ ReindexMultipleInternal(List *relids, ReindexParams *params)
static bool
ReindexRelationConcurrently(Oid relationOid, ReindexParams *params)
{
- typedef struct ReindexIndexInfo
- {
- Oid indexId;
- Oid tableId;
- Oid amId;
- bool safe; /* for set_indexsafe_procflags */
- } ReindexIndexInfo;
List *heapRelationIds = NIL;
List *indexIds = NIL;
List *newIndexIds = NIL;
- List *relationLocks = NIL;
- List *lockTags = NIL;
ListCell *lc,
*lc2;
MemoryContext private_context;
@@ -3274,13 +3353,6 @@ ReindexRelationConcurrently(Oid relationOid, ReindexParams *params)
char *relationName = NULL;
char *relationNamespace = NULL;
PGRUsage ru0;
- const int progress_index[] = {
- PROGRESS_CREATEIDX_COMMAND,
- PROGRESS_CREATEIDX_PHASE,
- PROGRESS_CREATEIDX_INDEX_OID,
- PROGRESS_CREATEIDX_ACCESS_METHOD_OID
- };
- int64 progress_vals[4];
/*
* Create a memory context that will survive forced transaction commits we
@@ -3553,6 +3625,69 @@ ReindexRelationConcurrently(Oid relationOid, ReindexParams *params)
Assert(heapRelationIds != NIL);
+ /* Do the work */
+ newIndexIds = ReindexIndexesConcurrently(indexIds, heapRelationIds, params, private_context);
+
+ /* Log what we did */
+ if ((params->options & REINDEXOPT_VERBOSE) != 0)
+ {
+ if (relkind == RELKIND_INDEX)
+ ereport(INFO,
+ (errmsg("index \"%s.%s\" was reindexed",
+ relationNamespace, relationName),
+ errdetail("%s.",
+ pg_rusage_show(&ru0))));
+ else
+ {
+ foreach(lc, newIndexIds)
+ {
+ Oid indOid = lfirst_oid(lc);
+ ereport(INFO,
+ (errmsg("index \"%s.%s\" was reindexed",
+ get_namespace_name(get_rel_namespace(indOid)),
+ get_rel_name(indOid))));
+ /* Don't show rusage here, since it's not per index. */
+ }
+
+ ereport(INFO,
+ (errmsg("table \"%s.%s\" was reindexed",
+ relationNamespace, relationName),
+ errdetail("%s.",
+ pg_rusage_show(&ru0))));
+ }
+ }
+
+
+ MemoryContextDelete(private_context);
+
+ return true;
+}
+
+/*
+ * Reindex concurrently for an arbitrary list of index relations
+ * This is called by ReindexRelationConcurrently and
+ */
+static List *
+ReindexIndexesConcurrently(List *indexIds, List *heapRelationIds,
+ ReindexParams *params, MemoryContext private_context)
+{
+ List *newIndexIds = NIL;
+ List *relationLocks = NIL;
+ List *lockTags = NIL;
+
+ ListCell *lc,
+ *lc2;
+
+ MemoryContext oldcontext;
+
+ const int progress_index[] = {
+ PROGRESS_CREATEIDX_COMMAND,
+ PROGRESS_CREATEIDX_PHASE,
+ PROGRESS_CREATEIDX_INDEX_OID,
+ PROGRESS_CREATEIDX_ACCESS_METHOD_OID
+ };
+ int64 progress_vals[4];
+
/*-----
* Now we have all the indexes we want to process in indexIds.
*
@@ -4026,42 +4161,9 @@ ReindexRelationConcurrently(Oid relationOid, ReindexParams *params)
/* Start a new transaction to finish process properly */
StartTransactionCommand();
- /* Log what we did */
- if ((params->options & REINDEXOPT_VERBOSE) != 0)
- {
- if (relkind == RELKIND_INDEX)
- ereport(INFO,
- (errmsg("index \"%s.%s\" was reindexed",
- relationNamespace, relationName),
- errdetail("%s.",
- pg_rusage_show(&ru0))));
- else
- {
- foreach(lc, newIndexIds)
- {
- ReindexIndexInfo *idx = lfirst(lc);
- Oid indOid = idx->indexId;
-
- ereport(INFO,
- (errmsg("index \"%s.%s\" was reindexed",
- get_namespace_name(get_rel_namespace(indOid)),
- get_rel_name(indOid))));
- /* Don't show rusage here, since it's not per index. */
- }
-
- ereport(INFO,
- (errmsg("table \"%s.%s\" was reindexed",
- relationNamespace, relationName),
- errdetail("%s.",
- pg_rusage_show(&ru0))));
- }
- }
-
- MemoryContextDelete(private_context);
-
pgstat_progress_end_command();
- return true;
+ return newIndexIds;
}
/*
diff --git a/src/test/regress/expected/create_index.out b/src/test/regress/expected/create_index.out
index 830fdddf24..6f41adf736 100644
--- a/src/test/regress/expected/create_index.out
+++ b/src/test/regress/expected/create_index.out
@@ -2470,12 +2470,12 @@ COMMIT;
REINDEX TABLE CONCURRENTLY pg_class; -- no catalog relation
ERROR: cannot reindex system catalogs concurrently
REINDEX INDEX CONCURRENTLY pg_class_oid_index; -- no catalog index
-ERROR: cannot reindex system catalogs concurrently
+ERROR: concurrent index creation on system catalog tables is not supported
-- These are the toast table and index of pg_authid.
REINDEX TABLE CONCURRENTLY pg_toast.pg_toast_1260; -- no catalog toast table
ERROR: cannot reindex system catalogs concurrently
REINDEX INDEX CONCURRENTLY pg_toast.pg_toast_1260_index; -- no catalog toast index
-ERROR: cannot reindex system catalogs concurrently
+ERROR: concurrent index creation on system catalog tables is not supported
REINDEX SYSTEM CONCURRENTLY postgres; -- not allowed for SYSTEM
ERROR: cannot reindex system catalogs concurrently
-- Warns about catalog relations
--
2.17.0
>From edde23839fe1dafffe804b24f9068c0f294c0651 Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryz...@telsasoft.com>
Date: Sun, 1 Nov 2020 13:46:18 -0600
Subject: [PATCH v13 06/18] More refactoring
---
src/backend/commands/indexcmds.c | 201 ++++++++++-----------
src/test/regress/expected/create_index.out | 4 +-
2 files changed, 93 insertions(+), 112 deletions(-)
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index f5fea14ff4..5c5596cd28 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -102,8 +102,8 @@ static void ReindexMultipleInternal(List *relids,
ReindexParams *params);
static bool ReindexRelationConcurrently(Oid relationOid,
ReindexParams *params);
-static List *ReindexIndexesConcurrently(List *indexIds, List *heapRelationIds,
- ReindexParams *params, MemoryContext private_context);
+static List *ReindexIndexesConcurrently(List *indexIds, ReindexParams *params,
+ MemoryContext private_context);
static void update_relispartition(Oid relationId, bool newval);
static inline void set_indexsafe_procflags(void);
@@ -2652,8 +2652,8 @@ ReindexIndex(RangeVar *indexRelation, ReindexParams *params, bool isTopLevel)
.indexId = indOid,
/* other fields set later */
};
+
ReindexIndexesConcurrently(list_make1(&idxinfo),
- list_make1_oid(IndexGetRelation(indOid, false)),
params, CurrentMemoryContext);
}
else
@@ -3023,12 +3023,11 @@ reindex_error_callback(void *arg)
/*
- * Given a list of index oids, return a list of leaf partitions by removing
- * any intermediate parents. heaprels is populated with the corresponding
- * tables.
+ * Given a list of index oids, return a new list of leaf partitions by
+ * excluding any intermediate parents.
*/
static List *
-leaf_indexes(List *inhoids, int options, List **heaprels)
+leaf_indexes(List *inhoids, int options)
{
List *partitions = NIL;
ListCell *lc;
@@ -3060,7 +3059,6 @@ leaf_indexes(List *inhoids, int options, List **heaprels)
/* Save partition OID in current MemoryContext */
partitions = lappend_oid(partitions, partoid);
- *heaprels = lappend_oid(*heaprels, tableoid);
}
return partitions;
@@ -3072,13 +3070,11 @@ leaf_indexes(List *inhoids, int options, List **heaprels)
*
* Reindex a set of partitions, per the partitioned index or table given
* by the caller.
- * XXX: should be further refactored with logic from ReindexRelationConcurrently
*/
static void
ReindexPartitions(Oid relid, ReindexParams *params, bool isTopLevel)
{
- List *partitions = NIL,
- *heaprels = NIL;
+ List *partitions = NIL;
char relkind = get_rel_relkind(relid);
char *relname = get_rel_name(relid);
char *relnamespace = get_namespace_name(get_rel_namespace(relid));
@@ -3132,7 +3128,7 @@ ReindexPartitions(Oid relid, ReindexParams *params, bool isTopLevel)
if (relkind == RELKIND_PARTITIONED_INDEX)
{
old_context = MemoryContextSwitchTo(reindex_context);
- partitions = leaf_indexes(inhoids, params->options, &heaprels);
+ partitions = leaf_indexes(inhoids, params->options);
MemoryContextSwitchTo(old_context);
} else {
/* Loop over parent tables */
@@ -3145,7 +3141,7 @@ ReindexPartitions(Oid relid, ReindexParams *params, bool isTopLevel)
parttable = table_open(partoid, ShareLock);
old_context = MemoryContextSwitchTo(reindex_context);
partindexes = RelationGetIndexList(parttable);
- partindexes = leaf_indexes(partindexes, params->options, &heaprels);
+ partindexes = leaf_indexes(partindexes, params->options);
partitions = list_concat(partitions, partindexes);
MemoryContextSwitchTo(old_context);
@@ -3154,10 +3150,9 @@ ReindexPartitions(Oid relid, ReindexParams *params, bool isTopLevel)
}
if ((params->options & REINDEXOPT_CONCURRENTLY) != 0 &&
- relkind == RELKIND_PARTITIONED_INDEX &&
get_rel_persistence(relid) != RELPERSISTENCE_TEMP)
{
- List *idxinfos = NIL;
+ List *idxinfos = NIL;
ReindexIndexInfo *idxinfo;
old_context = MemoryContextSwitchTo(reindex_context);
@@ -3172,7 +3167,7 @@ ReindexPartitions(Oid relid, ReindexParams *params, bool isTopLevel)
MemoryContextSwitchTo(old_context);
/* Process all indexes in a single loop */
- ReindexIndexesConcurrently(idxinfos, heaprels, params, reindex_context);
+ ReindexIndexesConcurrently(idxinfos, params, reindex_context);
} else {
/*
* Process each partition listed in a separate transaction. Note that
@@ -3342,7 +3337,6 @@ ReindexMultipleInternal(List *relids, ReindexParams *params)
static bool
ReindexRelationConcurrently(Oid relationOid, ReindexParams *params)
{
- List *heapRelationIds = NIL;
List *indexIds = NIL;
List *newIndexIds = NIL;
ListCell *lc,
@@ -3395,14 +3389,6 @@ ReindexRelationConcurrently(Oid relationOid, ReindexParams *params)
*/
Relation heapRelation;
- /* Save the list of relation OIDs in private context */
- oldcontext = MemoryContextSwitchTo(private_context);
-
- /* Track this relation for session locks */
- heapRelationIds = lappend_oid(heapRelationIds, relationOid);
-
- MemoryContextSwitchTo(oldcontext);
-
if (IsCatalogRelationOid(relationOid))
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
@@ -3415,7 +3401,7 @@ ReindexRelationConcurrently(Oid relationOid, ReindexParams *params)
ShareUpdateExclusiveLock);
/* leave if relation does not exist */
if (!heapRelation)
- break;
+ break; // XXX: lremove
}
else
heapRelation = table_open(relationOid,
@@ -3473,14 +3459,6 @@ ReindexRelationConcurrently(Oid relationOid, ReindexParams *params)
Relation toastRelation = table_open(toastOid,
ShareUpdateExclusiveLock);
- /* Save the list of relation OIDs in private context */
- oldcontext = MemoryContextSwitchTo(private_context);
-
- /* Track this relation for session locks */
- heapRelationIds = lappend_oid(heapRelationIds, toastOid);
-
- MemoryContextSwitchTo(oldcontext);
-
foreach(lc2, RelationGetIndexList(toastRelation))
{
Oid cellOid = lfirst_oid(lc2);
@@ -3521,78 +3499,6 @@ ReindexRelationConcurrently(Oid relationOid, ReindexParams *params)
break;
}
case RELKIND_INDEX:
- {
- Oid heapId = IndexGetRelation(relationOid,
- (params->options & REINDEXOPT_MISSING_OK) != 0);
- Relation heapRelation;
- ReindexIndexInfo *idx;
-
- /* if relation is missing, leave */
- if (!OidIsValid(heapId))
- break;
-
- if (IsCatalogRelationOid(heapId))
- ereport(ERROR,
- (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("cannot reindex system catalogs concurrently")));
-
- /*
- * Don't allow reindex for an invalid index on TOAST table, as
- * if rebuilt it would not be possible to drop it. Match
- * error message in reindex_index().
- */
- if (IsToastNamespace(get_rel_namespace(relationOid)) &&
- !get_index_isvalid(relationOid))
- ereport(ERROR,
- (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("cannot reindex invalid index on TOAST table")));
-
- /*
- * Check if parent relation can be locked and if it exists,
- * this needs to be done at this stage as the list of indexes
- * to rebuild is not complete yet, and REINDEXOPT_MISSING_OK
- * should not be used once all the session locks are taken.
- */
- if ((params->options & REINDEXOPT_MISSING_OK) != 0)
- {
- heapRelation = try_table_open(heapId,
- ShareUpdateExclusiveLock);
- /* leave if relation does not exist */
- if (!heapRelation)
- break;
- }
- else
- heapRelation = table_open(heapId,
- ShareUpdateExclusiveLock);
-
- if (OidIsValid(params->tablespaceOid) &&
- IsSystemRelation(heapRelation))
- ereport(ERROR,
- (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("cannot move system relation \"%s\"",
- get_rel_name(relationOid))));
-
- table_close(heapRelation, NoLock);
-
- /* Save the list of relation OIDs in private context */
- oldcontext = MemoryContextSwitchTo(private_context);
-
- /* Track the heap relation of this index for session locks */
- heapRelationIds = list_make1_oid(heapId);
-
- /*
- * Save the list of relation OIDs in private context. Note
- * that invalid indexes are allowed here.
- */
- idx = palloc(sizeof(ReindexIndexInfo));
- idx->indexId = relationOid;
- indexIds = lappend(indexIds, idx);
- /* other fields set later */
-
- MemoryContextSwitchTo(oldcontext);
- break;
- }
-
case RELKIND_PARTITIONED_TABLE:
case RELKIND_PARTITIONED_INDEX:
default:
@@ -3623,10 +3529,10 @@ ReindexRelationConcurrently(Oid relationOid, ReindexParams *params)
errmsg("cannot move non-shared relation to tablespace \"%s\"",
get_tablespace_name(params->tablespaceOid))));
- Assert(heapRelationIds != NIL);
+ // Assert(heapRelationIds != NIL);
/* Do the work */
- newIndexIds = ReindexIndexesConcurrently(indexIds, heapRelationIds, params, private_context);
+ newIndexIds = ReindexIndexesConcurrently(indexIds, params, private_context);
/* Log what we did */
if ((params->options & REINDEXOPT_VERBOSE) != 0)
@@ -3668,9 +3574,10 @@ ReindexRelationConcurrently(Oid relationOid, ReindexParams *params)
* This is called by ReindexRelationConcurrently and
*/
static List *
-ReindexIndexesConcurrently(List *indexIds, List *heapRelationIds,
- ReindexParams *params, MemoryContext private_context)
+ReindexIndexesConcurrently(List *indexIds, ReindexParams *params,
+ MemoryContext private_context)
{
+ List *heapRelationIds = NIL;
List *newIndexIds = NIL;
List *relationLocks = NIL;
List *lockTags = NIL;
@@ -3688,6 +3595,80 @@ ReindexIndexesConcurrently(List *indexIds, List *heapRelationIds,
};
int64 progress_vals[4];
+ /* It's not a shared catalog, so refuse to move it to shared tablespace */
+ if (params->tablespaceOid == GLOBALTABLESPACE_OID && false)
+ ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("cannot move non-shared relation to tablespace \"%s\"",
+ get_tablespace_name(params->tablespaceOid))));
+
+ foreach(lc, indexIds)
+ {
+ ReindexIndexInfo *idx = lfirst(lc);
+ Oid indexrelid = idx->indexId;
+ Oid heapId = IndexGetRelation(indexrelid,
+ (params->options & REINDEXOPT_MISSING_OK) != 0);
+ Relation heapRelation;
+
+ /* if relation is missing, leave */
+ if (!OidIsValid(heapId))
+ break; // XXX: ldelete?
+
+ if (IsCatalogRelationOid(heapId))
+ ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("cannot reindex system catalogs concurrently")));
+
+ /*
+ * Don't allow reindex for an invalid index on TOAST table, as
+ * if rebuilt it would not be possible to drop it. Match
+ * error message in reindex_index().
+ */
+ if (IsToastNamespace(get_rel_namespace(indexrelid)) &&
+ !get_index_isvalid(indexrelid))
+ ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("cannot reindex invalid index on TOAST table")));
+
+ if (OidIsValid(params->tablespaceOid) &&
+ IsCatalogRelationOid(indexrelid))
+ ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("cannot move system relation \"%s\"",
+ get_rel_name(indexrelid))));
+
+ /*
+ * Check if parent relation can be locked and if it exists,
+ * this needs to be done at this stage as the list of indexes
+ * to rebuild is not complete yet, and REINDEXOPT_MISSING_OK
+ * should not be used once all the session locks are taken.
+ */
+ if ((params->options & REINDEXOPT_MISSING_OK) != 0)
+ {
+ heapRelation = try_table_open(heapId,
+ ShareUpdateExclusiveLock);
+ /* leave if relation does not exist */
+ if (!heapRelation)
+ break; // ldelete
+ }
+ else
+ heapRelation = table_open(heapId,
+ ShareUpdateExclusiveLock);
+ table_close(heapRelation, NoLock);
+
+ /* Save the list of relation OIDs in private context */
+ oldcontext = MemoryContextSwitchTo(private_context);
+
+ /* Track the heap relation of this index for session locks */
+ heapRelationIds = lappend_oid(heapRelationIds, heapId);
+ // heapRelationIds = list_make1_oid(heapId);
+
+ /* Note that invalid indexes are allowed here. */
+
+ MemoryContextSwitchTo(oldcontext);
+ // break;
+ }
+
/*-----
* Now we have all the indexes we want to process in indexIds.
*
diff --git a/src/test/regress/expected/create_index.out b/src/test/regress/expected/create_index.out
index 6f41adf736..830fdddf24 100644
--- a/src/test/regress/expected/create_index.out
+++ b/src/test/regress/expected/create_index.out
@@ -2470,12 +2470,12 @@ COMMIT;
REINDEX TABLE CONCURRENTLY pg_class; -- no catalog relation
ERROR: cannot reindex system catalogs concurrently
REINDEX INDEX CONCURRENTLY pg_class_oid_index; -- no catalog index
-ERROR: concurrent index creation on system catalog tables is not supported
+ERROR: cannot reindex system catalogs concurrently
-- These are the toast table and index of pg_authid.
REINDEX TABLE CONCURRENTLY pg_toast.pg_toast_1260; -- no catalog toast table
ERROR: cannot reindex system catalogs concurrently
REINDEX INDEX CONCURRENTLY pg_toast.pg_toast_1260_index; -- no catalog toast index
-ERROR: concurrent index creation on system catalog tables is not supported
+ERROR: cannot reindex system catalogs concurrently
REINDEX SYSTEM CONCURRENTLY postgres; -- not allowed for SYSTEM
ERROR: cannot reindex system catalogs concurrently
-- Warns about catalog relations
--
2.17.0
