On Thu, Apr 11, 2019 at 09:49:47AM -0400, Alvaro Herrera wrote: > Hmm, I suppose that makes sense for REINDEX TABLE or anything that > reindexes more than one index, but if you do REINDEX INDEX surely it > is reasonable to allow the case?
Yes, we could revisit the REINDEX INDEX portion of the decision, and after sleeping on it my previous argument makes limited sense for REINDEX processes using only one index. One could note that the header comment of ReindexRelationConcurrently() kind of implies the same conclusion as you do, but that's perhaps just an accident. So... I am coming up with the patch attached. I have introduced some tests using a trick with CIC to have an invalid index to work on. -- Michael
diff --git a/doc/src/sgml/ref/create_index.sgml b/doc/src/sgml/ref/create_index.sgml
index d9d95b20e3..929a326ae7 100644
--- a/doc/src/sgml/ref/create_index.sgml
+++ b/doc/src/sgml/ref/create_index.sgml
@@ -555,10 +555,8 @@ Indexes:
The recommended recovery
method in such cases is to drop the index and try again to perform
- <command>CREATE INDEX CONCURRENTLY</command>. (Another possibility is to rebuild
- the index with <command>REINDEX</command>. However, since <command>REINDEX</command>
- does not support concurrent builds, this option is unlikely to seem
- attractive.)
+ <command>CREATE INDEX CONCURRENTLY</command>. (Another possibility is
+ to rebuild the index with <command>REINDEX INDEX CONCURRENTLY</command>.
</para>
<para>
diff --git a/doc/src/sgml/ref/reindex.sgml b/doc/src/sgml/ref/reindex.sgml
index e05a76c6d8..303436c89d 100644
--- a/doc/src/sgml/ref/reindex.sgml
+++ b/doc/src/sgml/ref/reindex.sgml
@@ -65,12 +65,11 @@ REINDEX [ ( VERBOSE ) ] { INDEX | TABLE | SCHEMA | DATABASE | SYSTEM } [ CONCURR
<listitem>
<para>
- An index build with the <literal>CONCURRENTLY</literal> option failed, leaving
- an <quote>invalid</quote> index. Such indexes are useless but it can be
- convenient to use <command>REINDEX</command> to rebuild them. Note that
- <command>REINDEX</command> will not perform a concurrent build on an invalid index. To build the
- index without interfering with production you should drop the index and
- reissue the <command>CREATE INDEX CONCURRENTLY</command> command.
+ If an index build fails with the <literal>CONCURRENTLY</literal> option,
+ this index is left as <quote>invalid</quote>. Such indexes are useless
+ but it can be convenient to use <command>REINDEX</command> to rebuild
+ them. Note that only <command>REINDEX INDEX</command> is able
+ to perform a concurrent build on an invalid index.
</para>
</listitem>
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 46f32c21f9..a1c91b5fb8 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -2776,11 +2776,6 @@ ReindexRelationConcurrently(Oid relationOid, int options)
}
case RELKIND_INDEX:
{
- /*
- * For an index simply add its Oid to list. Invalid indexes
- * cannot be included in list.
- */
- Relation indexRelation = index_open(relationOid, ShareUpdateExclusiveLock);
Oid heapId = IndexGetRelation(relationOid, false);
/* A shared relation cannot be reindexed concurrently */
@@ -2801,25 +2796,13 @@ ReindexRelationConcurrently(Oid relationOid, int options)
/* 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.
+ */
+ indexIds = lappend_oid(indexIds, relationOid);
+
MemoryContextSwitchTo(oldcontext);
-
- if (!indexRelation->rd_index->indisvalid)
- ereport(WARNING,
- (errcode(ERRCODE_INDEX_CORRUPTED),
- errmsg("cannot reindex concurrently invalid index \"%s.%s\", skipping",
- get_namespace_name(get_rel_namespace(relationOid)),
- get_rel_name(relationOid))));
- else
- {
- /* Save the list of relation OIDs in private context */
- oldcontext = MemoryContextSwitchTo(private_context);
-
- indexIds = lappend_oid(indexIds, relationOid);
-
- MemoryContextSwitchTo(oldcontext);
- }
-
- index_close(indexRelation, NoLock);
break;
}
case RELKIND_PARTITIONED_TABLE:
diff --git a/src/test/regress/expected/create_index.out b/src/test/regress/expected/create_index.out
index f9b4768aee..8bfcf57d5a 100644
--- a/src/test/regress/expected/create_index.out
+++ b/src/test/regress/expected/create_index.out
@@ -2033,6 +2033,38 @@ Referenced by:
DROP MATERIALIZED VIEW concur_reindex_matview;
DROP TABLE concur_reindex_tab, concur_reindex_tab2, concur_reindex_tab3;
+-- Check handling of invalid indexes
+CREATE TABLE concur_reindex_tab4 (c1 int);
+INSERT INTO concur_reindex_tab4 VALUES (1), (1), (2);
+-- This trick creates an invalid index.
+CREATE UNIQUE INDEX CONCURRENTLY concur_reindex_ind5 ON concur_reindex_tab4 (c1);
+ERROR: could not create unique index "concur_reindex_ind5"
+DETAIL: Key (c1)=(1) is duplicated.
+-- And this makes the previous failure go away, so the index can become valid.
+DELETE FROM concur_reindex_tab4 WHERE c1 = 1;
+-- The invalid index is not processed when running REINDEX TABLE.
+REINDEX TABLE CONCURRENTLY concur_reindex_tab4;
+WARNING: cannot reindex concurrently invalid index "public.concur_reindex_ind5", skipping
+NOTICE: table "concur_reindex_tab4" has no indexes
+\d concur_reindex_tab4
+ Table "public.concur_reindex_tab4"
+ Column | Type | Collation | Nullable | Default
+--------+---------+-----------+----------+---------
+ c1 | integer | | |
+Indexes:
+ "concur_reindex_ind5" UNIQUE, btree (c1) INVALID
+
+-- But it is fixed with REINDEX INDEX
+REINDEX INDEX CONCURRENTLY concur_reindex_ind5;
+\d concur_reindex_tab4
+ Table "public.concur_reindex_tab4"
+ Column | Type | Collation | Nullable | Default
+--------+---------+-----------+----------+---------
+ c1 | integer | | |
+Indexes:
+ "concur_reindex_ind5" UNIQUE, btree (c1)
+
+DROP TABLE concur_reindex_tab4;
--
-- REINDEX SCHEMA
--
diff --git a/src/test/regress/sql/create_index.sql b/src/test/regress/sql/create_index.sql
index 2f0e9a63e6..74242098e3 100644
--- a/src/test/regress/sql/create_index.sql
+++ b/src/test/regress/sql/create_index.sql
@@ -806,6 +806,21 @@ REINDEX SCHEMA CONCURRENTLY pg_catalog;
DROP MATERIALIZED VIEW concur_reindex_matview;
DROP TABLE concur_reindex_tab, concur_reindex_tab2, concur_reindex_tab3;
+-- Check handling of invalid indexes
+CREATE TABLE concur_reindex_tab4 (c1 int);
+INSERT INTO concur_reindex_tab4 VALUES (1), (1), (2);
+-- This trick creates an invalid index.
+CREATE UNIQUE INDEX CONCURRENTLY concur_reindex_ind5 ON concur_reindex_tab4 (c1);
+-- And this makes the previous failure go away, so the index can become valid.
+DELETE FROM concur_reindex_tab4 WHERE c1 = 1;
+-- The invalid index is not processed when running REINDEX TABLE.
+REINDEX TABLE CONCURRENTLY concur_reindex_tab4;
+\d concur_reindex_tab4
+-- But it is fixed with REINDEX INDEX
+REINDEX INDEX CONCURRENTLY concur_reindex_ind5;
+\d concur_reindex_tab4
+DROP TABLE concur_reindex_tab4;
+
--
-- REINDEX SCHEMA
--
signature.asc
Description: PGP signature
