Subject: [PATCH] ERROR out early on attempt to touch user indexes while they are being (re)built.

From: Arseny Sher <[email-masked]>
Date: Thu, 12 Sep 2019 17:35:16 +0300

Existing ReindexIsProcessingIndex check is consulted only in genam.c and thus
enforced only for system catalogs. Check it also in the planner, so that indexes
which are currently being rebuilt are never tried. Also cock SetReindexProcessing
in index_create to defend from index self usage during its creation.

Without this, VACUUM FULL or just CREATE INDEX might fail with something like

ERROR:  could not read block 3534 in file "base/41366676/56697497": read only 0 of 8192 bytes

if there are indexes which usage can be considered during these very
indexes (re)building, i.e. index expression scans indexed table.

Such error might seem scary, so catch this earlier.
---
 src/backend/catalog/index.c                | 22 ++++++++++++++++++++--
 src/backend/optimizer/util/plancat.c       |  9 +++++++++
 src/test/regress/expected/create_index.out | 15 +++++++++++++++
 src/test/regress/sql/create_index.sql      | 13 +++++++++++++
 4 files changed, 57 insertions(+), 2 deletions(-)

diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index 3e1d40662d..5bc764ce46 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -1174,7 +1174,22 @@ index_create(Relation heapRelation,
 	}
 	else
 	{
-		index_build(heapRelation, indexRelation, indexInfo, false, true);
+		/* ensure SetReindexProcessing state isn't leaked */
+		PG_TRY();
+		{
+			/* Suppress use of the target index while building it */
+			SetReindexProcessing(heapRelationId, indexRelationId);
+
+			index_build(heapRelation, indexRelation, indexInfo, false, true);
+		}
+		PG_CATCH();
+		{
+			/* Make sure flag gets cleared on error exit */
+			ResetReindexProcessing();
+			PG_RE_THROW();
+		}
+		PG_END_TRY();
+		ResetReindexProcessing();
 	}
 
 	/*
@@ -1379,7 +1394,10 @@ index_concurrently_build(Oid heapRelationId,
 	indexInfo->ii_Concurrent = true;
 	indexInfo->ii_BrokenHotChain = false;
 
-	/* Now build the index */
+	/*
+	 * Now build the index
+	 * SetReindexProcessing is not required since indisvalid is false anyway
+	 */
 	index_build(heapRel, indexRelation, indexInfo, false, true);
 
 	/* Close both the relations, but keep the locks */
diff --git a/src/backend/optimizer/util/plancat.c b/src/backend/optimizer/util/plancat.c
index cf1761401d..e42471cac8 100644
--- a/src/backend/optimizer/util/plancat.c
+++ b/src/backend/optimizer/util/plancat.c
@@ -27,6 +27,7 @@
 #include "access/xlog.h"
 #include "catalog/catalog.h"
 #include "catalog/dependency.h"
+#include "catalog/index.h"
 #include "catalog/heap.h"
 #include "catalog/pg_am.h"
 #include "catalog/pg_proc.h"
@@ -199,6 +200,14 @@ get_relation_info(PlannerInfo *root, Oid relationObjectId, bool inhparent,
 			indexRelation = index_open(indexoid, lmode);
 			index = indexRelation->rd_index;
 
+			if (ReindexIsProcessingIndex(indexoid))
+				ereport(ERROR,
+						(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
+						 errmsg("cannot use index %s which is currently being reindexed",
+								RelationGetRelationName(indexRelation)),
+						 errhint("Probably index expressions include functions "
+								 "accessing indexed table itself, but they must be immutable.")));
+
 			/*
 			 * Ignore invalid indexes, since they can't safely be used for
 			 * queries.  Note that this is OK because the data structure we
diff --git a/src/test/regress/expected/create_index.out b/src/test/regress/expected/create_index.out
index 324db1b6ae..f9aeb97ed8 100644
--- a/src/test/regress/expected/create_index.out
+++ b/src/test/regress/expected/create_index.out
@@ -1323,6 +1323,21 @@ create unique index hash_f8_index_1 on hash_f8_heap(abs(random));
 create unique index hash_f8_index_2 on hash_f8_heap((seqno + 1), random);
 create unique index hash_f8_index_3 on hash_f8_heap(random) where seqno > 1000;
 --
+-- Create an index which might consider using this very index during the build.
+--
+-- primary key ensures relhasindex is set
+CREATE TABLE pears(f1 int primary key, f2 int);
+INSERT INTO pears SELECT i, i+1 FROM generate_series(1, 100) i;
+CREATE FUNCTION pears_f(i int) RETURNS int LANGUAGE SQL IMMUTABLE AS $$
+  SELECT f1 FROM pears WHERE pears.f2 = 42
+$$;
+CREATE index ON pears ((pears_f(f1)));
+ERROR:  cannot use index pears_pears_f_idx which is currently being reindexed
+HINT:  Probably index expressions include functions accessing indexed table itself, but they must be immutable.
+CONTEXT:  SQL function "pears_f" during startup
+DROP TABLE pears;
+DROP FUNCTION pears_f;
+--
 -- Try some concurrent index builds
 --
 -- Unfortunately this only tests about half the code paths because there are
diff --git a/src/test/regress/sql/create_index.sql b/src/test/regress/sql/create_index.sql
index f96bebf410..76a781f6b0 100644
--- a/src/test/regress/sql/create_index.sql
+++ b/src/test/regress/sql/create_index.sql
@@ -446,6 +446,19 @@ create unique index hash_f8_index_2 on hash_f8_heap((seqno + 1), random);
 create unique index hash_f8_index_3 on hash_f8_heap(random) where seqno > 1000;
 
 --
+-- Create an index which might consider using this very index during the build.
+--
+-- primary key ensures relhasindex is set
+CREATE TABLE pears(f1 int primary key, f2 int);
+INSERT INTO pears SELECT i, i+1 FROM generate_series(1, 100) i;
+CREATE FUNCTION pears_f(i int) RETURNS int LANGUAGE SQL IMMUTABLE AS $$
+  SELECT f1 FROM pears WHERE pears.f2 = 42
+$$;
+CREATE index ON pears ((pears_f(f1)));
+DROP TABLE pears;
+DROP FUNCTION pears_f;
+
+--
 -- Try some concurrent index builds
 --
 -- Unfortunately this only tests about half the code paths because there are
-- 
2.11.0

--=-=-=--