Date: Fri, 12 Apr 2019 12:10:46 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Alvaro Herrera <alvherre@2ndquadrant.com>
Cc: Dagfinn Ilmari =?iso-8859-1?Q?Manns=E5ker?= <ilmari@ilmari.org>,
	Peter Eisentraut <peter.eisentraut@2ndquadrant.com>,
	Sergei Kornilov <sk@zsrv.org>,
	"pgsql-hackers@lists.postgresql.org" <pgsql-hackers@lists.postgresql.org>
Subject: Re: REINDEX CONCURRENTLY 2.0
Message-ID: <20190412031046.GD2144@paquier.xyz>
References: <20190411132704.GC30766@paquier.xyz>
 <20190411134947.GA22043@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="idY8LE8SD6/8DnRI"
Content-Disposition: inline
In-Reply-To: <20190411134947.GA22043@alvherre.pgsql>
Precedence: bulk


--idY8LE8SD6/8DnRI
Content-Type: multipart/mixed; boundary="TybLhxa8M7aNoW+V"
Content-Disposition: inline


--TybLhxa8M7aNoW+V
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

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

--TybLhxa8M7aNoW+V
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment; filename="reindex-conc-invalid.patch"
Content-Transfer-Encoding: quoted-printable

diff --git a/doc/src/sgml/ref/create_index.sgml b/doc/src/sgml/ref/create_i=
ndex.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:
=20
     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>RE=
INDEX</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>
=20
    <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 | DATA=
BASE | SYSTEM } [ CONCURR
=20
     <listitem>
      <para>
-      An index build with the <literal>CONCURRENTLY</literal> option faile=
d, leaving
-      an <quote>invalid</quote> index. Such indexes are useless but it can=
 be
-      convenient to use <command>REINDEX</command> to rebuild them. Note t=
hat
-      <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> opt=
ion,
+      this index is left as <quote>invalid</quote>. Such indexes are usele=
ss
+      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>
=20
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexc=
mds.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 opt=
ions)
 			}
 		case RELKIND_INDEX:
 			{
-				/*
-				 * For an index simply add its Oid to list. Invalid indexes
-				 * cannot be included in list.
-				 */
-				Relation	indexRelation =3D index_open(relationOid, ShareUpdateExclusiv=
eLock);
 				Oid			heapId =3D IndexGetRelation(relationOid, false);
=20
 				/* A shared relation cannot be reindexed concurrently */
@@ -2801,25 +2796,13 @@ ReindexRelationConcurrently(Oid relationOid, int op=
tions)
 				/* Track the heap relation of this index for session locks */
 				heapRelationIds =3D list_make1_oid(heapId);
=20
+				/*
+				 * Save the list of relation OIDs in private context.  Note
+				 * that invalid indexes are allowed here.
+				 */
+				indexIds =3D 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\", skipp=
ing",
-									get_namespace_name(get_rel_namespace(relationOid)),
-									get_rel_name(relationOid))));
-				else
-				{
-					/* Save the list of relation OIDs in private context */
-					oldcontext =3D MemoryContextSwitchTo(private_context);
-
-					indexIds =3D 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:
=20
 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_tab=
4 (c1);
+ERROR:  could not create unique index "concur_reindex_ind5"
+DETAIL:  Key (c1)=3D(1) is duplicated.
+-- And this makes the previous failure go away, so the index can become va=
lid.
+DELETE FROM concur_reindex_tab4 WHERE c1 =3D 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=20
+--------+---------+-----------+----------+---------
+ c1     | integer |           |          |=20
+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=20
+--------+---------+-----------+----------+---------
+ c1     | integer |           |          |=20
+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/c=
reate_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;
=20
+-- 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_tab=
4 (c1);
+-- And this makes the previous failure go away, so the index can become va=
lid.
+DELETE FROM concur_reindex_tab4 WHERE c1 =3D 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
 --

--TybLhxa8M7aNoW+V--

--idY8LE8SD6/8DnRI
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAlywAbYACgkQnvQgOdby
QH2LsBAAlEYAMwgzxr3IoPRzH6BAHRJgUgL0OhFVz3ZWld/Y6XTzGSimRSxOPWdd
5zQWdV0YfuKc51FeLyuz6JxT5VYKFPlvQdQdINln9bjHlZh1bOPa8D/s9ml+YFsZ
cx9dAvK5qAVGc+ZiMo/9cOPnzE82IL18lqNeWYq4Cs+jwlxp23rSd/iS/snlNV2y
r6HKgFrkFu26AKeO3gr31i+P5V6y979VgGdJN/KAVTaZjShQe00rPC5aPqcKxEx+
tFbb9YSXLYD2FXiIzMNb2KIyXdNxPNJT/fAFYWMiV4/FweXwgPIRfWwHPjphtYhy
7cux1xIR9WQHqrxjQNuyZjpzKzykJayQnv62A4mzX9cJcvz586N+plfAWePy0IER
6lNwoFgXSI+kn9L3TC4V5alhCRL4TGhP+5KlBmVknLSrNatlce8nglA2z0QKyXel
T8zHX9hLc3lci8IzooNS0vjWUYGHYDPd1fdSEFu9CaBrJ/JOZ5Eh+recWmyfSpVJ
goB2j8JBNN00Mdk80YedM/vkC6vs/tN2EOA4qqapczJH+flYuov4JTIIDJ7PU+lB
vzcV1eQpZjoNUAcK/6cxWtcgsBid1mgBgQflZrWw6wJlOuELMeNBeemFvOr+1RMG
9eVbiYqgdk0P/6rwF/lMQ6Gf7/VCAY/Beo51yEQKKeo+xMLM4iU=
=AGHf
-----END PGP SIGNATURE-----

--idY8LE8SD6/8DnRI--