Re: REINDEX CONCURRENTLY unexpectedly fails

Michael Paquier <[email protected]> Fri, 13 Dec 2019 12:45:36 +0900

Newsgroups	gmane.comp.db.postgresql.bugs
Message-ID	<[email protected]>

On Thu, Dec 12, 2019 at 01:37:09PM -0800, Andres Freund wrote:
> On 2019-11-20 12:54:08 +0900, Michael Paquier wrote:
>> ON COMMIT DELETE ROWS does a physical truncation of the relation
>> files. And as DROP INDEX CONCURRENTLY cannot be run in a transaction
>> block, you would never face a case where you have no past TIDs which
>> could be referred to when setting the index as invalid.
> 
> It's probably not reachable, but it strikes me as really fragile and
> dangerous. If e.g. somehow ON COMMIT DROP tables could exist when DROP
> CONCURRENTLY were run, the index_concurrently_set_dead() could very well
> target a row that's since been deleted in an earlier transaction.

Hmm.  That joins with your point downthread about future changes..

>> Now I don't actually object to enforce the non-concurrent path in
>> index_drop() for *all* temporary relations.  Anyway, that makes sense
>> in itself on performance grounds, similarly to the create path, so did
>> that by enforcing the flag in index_drop() (doDeletion would be
>> tempting but I took the problem at its root).  And added some tests
>> for the drop path and an extra assertion.
> 
> Cool.
> 
> I still think we'd be well served to add a few CheckTableNotInUse() type
> checks...

Sure.  We have already one in drop_index, so DROP INDEX is covered, as
well as reindex_index() which is taken by all non-concurrent REINDEX
commands.  Adding one in ReindexRelationConcurrently() may make
sense..

>> Considered that, but ON COMMIT DROP does not make sense because it
>> requires a transaction context, which is why I did not add one.  And
>> it seems to me that there is not much value to just check after CIC or
>> REINDEX's restriction to not run in a transaction block?  I added
>> tests for these two, but I am of the opinion that they don't bring
>> much.
> 
> I think because CIC now falls back to non-concurrent mode, it's
> worthwhile to exercise this path. It seems far from unlikely that the
> code gets moved around enough that suddenly CIC is allowed in
> transactions when targetting temp tables.

That's a good point, we have no guarantee that nobody would play with
this area in the future.  Well, the patch has those tests anyway since
the last version, so I have not touched them.

>> I think that documenting it is good for the end-user as well.
> 
> Why?

Even if using a temporary table, the commands are not allowed within a
transaction block, but we still track them in wait events so seeing
an event related only to a non-concurrent path when using CONCURRENTLY
can be confusing.

>> +	/*
>> +	 * Enforce non-concurrent drop if the relation does not support this
>> +	 * option.
>> +	 */
>> +	if (!RelationSupportsConcurrently(get_rel_persistence(indexId)))
>> +		concurrent = false;
>> +
> 
> Echoing Alvaro, I'm less than convinced by this name.

I would really keep "Relation" in this part of the naming as this can
be used for an index or its parent table, so in the updated attached I
have gone with RelationSupportsConcurrentIndexing(), which is a
suggestion from Alvaro.

> Copying this to some, but not all, the places where
> RelationSupportsConcurrently() is called doesn't seem helpful...

Not sure I follow your point here.  The following code paths are
currently checked in the patch using this routine:
- index_drop, both used by DROP INDEX and REINDEX CONCURRENTLY.  This
routine is called basically via performMultipleDeletions().  For
REINDEX CONCURRENTLY, this cannot be actually reached, but not for
DROP INDEX CONCURRENTLY.  The logic to decide which drop behavior to
choose is done in RemoveRelations().  And while we don't support
dropping multiple objects with CONCURRENTLY, we have no way to say now
for each object which lock level should be used for the drop, so it
seems safer to me now to enforce non-concurrent to be used directly in
index_drop rather than doing so at a higher level.
- ReindexIndex, ReindexTable and ReindexMultipleTables, to check if
the non-concurrent or concurrent paths need to be called for
respectively REINDEX INDEX, TABLE and SCHEMA/DATABASE/SYSTEM.
- RelationSupportsConcurrentIndexing, as the entry point for CREATE
INDEX.

+   /*
+    * Enforce non-concurrent build if the relation does not support this
+    * option.
+    */
Or are you suggesting to remove this comment from the two places where
it is used because it does not prove to help much?

> If we want to add docs, I'd say at most something like "For temporary
> tables index creation is always non-concurrent, as no other session can
> access them, and non-concurrent index creation is cheaper.".

Sounds like a better wording to me.  Documenting it still seems rather
important to me as I suspect that it could surprise some users.
--
Michael

reindex-conc-temp-v3.patch (text/x-diff, 14.5 KB)

diff --git a/src/include/catalog/index.h b/src/include/catalog/index.h
index 27d9e537d3..24e3c017cd 100644
--- a/src/include/catalog/index.h
+++ b/src/include/catalog/index.h
@@ -115,6 +115,8 @@ extern bool CompareIndexInfo(IndexInfo *info1, IndexInfo *info2,
 
 extern void BuildSpeculativeIndexInfo(Relation index, IndexInfo *ii);
 
+extern bool RelationSupportsConcurrentIndexing(char relpersistence);
+
 extern void FormIndexDatum(IndexInfo *indexInfo,
 						   TupleTableSlot *slot,
 						   EState *estate,
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index e9955707fa..5809779adc 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -2017,6 +2017,13 @@ index_drop(Oid indexId, bool concurrent, bool concurrent_lock_mode)
 	LOCKTAG		heaplocktag;
 	LOCKMODE	lockmode;
 
+	/*
+	 * Enforce non-concurrent drop if the relation does not support this
+	 * option.
+	 */
+	if (!RelationSupportsConcurrentIndexing(get_rel_persistence(indexId)))
+		concurrent = false;
+
 	/*
 	 * To drop an index safely, we must grab exclusive lock on its parent
 	 * table.  Exclusive lock on the index alone is insufficient because
@@ -2109,6 +2116,9 @@ index_drop(Oid indexId, bool concurrent, bool concurrent_lock_mode)
 		 */
 		CacheInvalidateRelcache(userHeapRelation);
 
+		/* Temporary tables cannot be processed concurrently */
+		Assert(userHeapRelation->rd_rel->relpersistence != RELPERSISTENCE_TEMP);
+
 		/* save lockrelid and locktag for below, then close but keep locks */
 		heaprelid = userHeapRelation->rd_lockInfo.lockRelId;
 		SET_LOCKTAG_RELATION(heaplocktag, heaprelid.dbId, heaprelid.relId);
@@ -2491,6 +2501,30 @@ CompareIndexInfo(IndexInfo *info1, IndexInfo *info2,
 	return true;
 }
 
+/*
+ * RelationSupportsConcurrentIndexing
+ *
+ * Check if a relation supports concurrent builds or not.  This is
+ * used as a sanity check prior processing CREATE INDEX, DROP INDEX
+ * or REINDEX when using CONCURRENTLY.
+ */
+bool
+RelationSupportsConcurrentIndexing(char relpersistence)
+{
+	/*
+	 * Build indexes non-concurrently for temporary relations.  Such
+	 * relations only work with the session assigned to them, so they are
+	 * not subject to concurrent concerns, and a concurrent build would
+	 * cause issues with ON COMMIT actions triggered by the transactions
+	 * of the concurrent build.  A non-concurrent reindex is also more
+	 * efficient in this case.
+	 */
+	if (relpersistence == RELPERSISTENCE_TEMP)
+		return false;
+
+	return true;
+}
+
 /* ----------------
  *		BuildSpeculativeIndexInfo
  *			Add extra state to IndexInfo record
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 374e2d0efe..4dc8a696fe 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -550,6 +550,13 @@ DefineIndex(Oid relationId,
 	lockmode = stmt->concurrent ? ShareUpdateExclusiveLock : ShareLock;
 	rel = table_open(relationId, lockmode);
 
+	/*
+	 * Enforce non-concurrent build if the relation does not support this
+	 * option.
+	 */
+	if (!RelationSupportsConcurrentIndexing(rel->rd_rel->relpersistence))
+		stmt->concurrent = false;
+
 	namespaceId = RelationGetNamespace(rel);
 
 	/* Ensure that it makes sense to index this kind of relation */
@@ -2349,7 +2356,7 @@ ReindexIndex(RangeVar *indexRelation, int options, bool concurrent)
 	persistence = irel->rd_rel->relpersistence;
 	index_close(irel, NoLock);
 
-	if (concurrent)
+	if (concurrent && RelationSupportsConcurrentIndexing(persistence))
 		ReindexRelationConcurrently(indOid, options);
 	else
 		reindex_index(indOid, false, persistence,
@@ -2442,7 +2449,8 @@ ReindexTable(RangeVar *relation, int options, bool concurrent)
 									   0,
 									   RangeVarCallbackOwnsTable, NULL);
 
-	if (concurrent)
+	if (concurrent &&
+		RelationSupportsConcurrentIndexing(get_rel_persistence(heapOid)))
 	{
 		result = ReindexRelationConcurrently(heapOid, options);
 
@@ -2648,7 +2656,8 @@ ReindexMultipleTables(const char *objectName, ReindexObjectType objectKind,
 		/* functions in indexes may want a snapshot set */
 		PushActiveSnapshot(GetTransactionSnapshot());
 
-		if (concurrent)
+		if (concurrent &&
+			RelationSupportsConcurrentIndexing(get_rel_persistence(relid)))
 		{
 			(void) ReindexRelationConcurrently(relid, options);
 			/* ReindexRelationConcurrently() does the verbose output */
@@ -2771,6 +2780,9 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 				/* Open relation to get its indexes */
 				heapRelation = table_open(relationOid, ShareUpdateExclusiveLock);
 
+				/* Temporary tables cannot be processed concurrently */
+				Assert(heapRelation->rd_rel->relpersistence != RELPERSISTENCE_TEMP);
+
 				/* Add all the valid indexes of relation to list */
 				foreach(lc, RelationGetIndexList(heapRelation))
 				{
@@ -2864,6 +2876,9 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 				/* Save the list of relation OIDs in private context */
 				oldcontext = MemoryContextSwitchTo(private_context);
 
+				/* Temporary tables cannot be processed concurrently */
+				Assert(get_rel_persistence(heapId) != RELPERSISTENCE_TEMP);
+
 				/* Track the heap relation of this index for session locks */
 				heapRelationIds = list_make1_oid(heapId);
 
@@ -2939,6 +2954,13 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 		heapRel = table_open(indexRel->rd_index->indrelid,
 							 ShareUpdateExclusiveLock);
 
+		/*
+		 * Also check for active uses of the relation in the current
+		 * transaction, including open scans and pending AFTER trigger
+		 * events.
+		 */
+		CheckTableNotInUse(indexRel, "REINDEX");
+
 		pgstat_progress_start_command(PROGRESS_COMMAND_CREATE_INDEX,
 									  RelationGetRelid(heapRel));
 		pgstat_progress_update_param(PROGRESS_CREATEIDX_COMMAND,
diff --git a/src/test/regress/expected/create_index.out b/src/test/regress/expected/create_index.out
index 645ae2cf34..4be4379f77 100644
--- a/src/test/regress/expected/create_index.out
+++ b/src/test/regress/expected/create_index.out
@@ -1398,6 +1398,31 @@ Indexes:
     "concur_index5" btree (f2) WHERE f1 = 'x'::text
     "std_index" btree (f2)
 
+-- Temporary tables with concurrent builds and on-commit actions
+-- CONCURRENTLY used with CREATE INDEX and DROP INDEX is ignored.
+-- PRESERVE ROWS, the default.
+CREATE TEMP TABLE concur_temp (f1 int, f2 text)
+  ON COMMIT PRESERVE ROWS;
+INSERT INTO concur_temp VALUES (1, 'foo'), (2, 'bar');
+CREATE INDEX CONCURRENTLY concur_temp_ind ON concur_temp(f1);
+DROP INDEX CONCURRENTLY concur_temp_ind;
+DROP TABLE concur_temp;
+-- ON COMMIT DROP
+BEGIN;
+CREATE TEMP TABLE concur_temp (f1 int, f2 text)
+  ON COMMIT DROP;
+INSERT INTO concur_temp VALUES (1, 'foo'), (2, 'bar');
+-- Fails when running in a transaction.
+CREATE INDEX CONCURRENTLY concur_temp_ind ON concur_temp(f1);
+ERROR:  CREATE INDEX CONCURRENTLY cannot run inside a transaction block
+COMMIT;
+-- ON COMMIT DELETE ROWS
+CREATE TEMP TABLE concur_temp (f1 int, f2 text)
+  ON COMMIT DELETE ROWS;
+INSERT INTO concur_temp VALUES (1, 'foo'), (2, 'bar');
+CREATE INDEX CONCURRENTLY concur_temp_ind ON concur_temp(f1);
+DROP INDEX CONCURRENTLY concur_temp_ind;
+DROP TABLE concur_temp;
 --
 -- Try some concurrent index drops
 --
@@ -2381,6 +2406,55 @@ SELECT pg_get_indexdef('concur_exprs_index_pred_2'::regclass);
 (1 row)
 
 DROP TABLE concur_exprs_tab;
+-- Temporary tables and on-commit actions, where CONCURRENTLY is ignored.
+-- ON COMMIT PRESERVE ROWS, the default.
+CREATE TEMP TABLE concur_temp_tab_1 (c1 int, c2 text)
+  ON COMMIT PRESERVE ROWS;
+INSERT INTO concur_temp_tab_1 VALUES (1, 'foo'), (2, 'bar');
+CREATE INDEX concur_temp_ind_1 ON concur_temp_tab_1(c2);
+REINDEX TABLE CONCURRENTLY concur_temp_tab_1;
+REINDEX INDEX CONCURRENTLY concur_temp_ind_1;
+-- Still fails in transaction blocks
+BEGIN;
+REINDEX INDEX CONCURRENTLY concur_temp_ind_1;
+ERROR:  REINDEX CONCURRENTLY cannot run inside a transaction block
+COMMIT;
+-- ON COMMIT DELETE ROWS
+CREATE TEMP TABLE concur_temp_tab_2 (c1 int, c2 text)
+  ON COMMIT DELETE ROWS;
+CREATE INDEX concur_temp_ind_2 ON concur_temp_tab_2(c2);
+REINDEX TABLE CONCURRENTLY concur_temp_tab_2;
+REINDEX INDEX CONCURRENTLY concur_temp_ind_2;
+-- ON COMMIT DROP
+BEGIN;
+CREATE TEMP TABLE concur_temp_tab_3 (c1 int, c2 text)
+  ON COMMIT PRESERVE ROWS;
+INSERT INTO concur_temp_tab_3 VALUES (1, 'foo'), (2, 'bar');
+CREATE INDEX concur_temp_ind_3 ON concur_temp_tab_3(c2);
+-- Fails when running in a transaction
+REINDEX INDEX CONCURRENTLY concur_temp_ind_3;
+ERROR:  REINDEX CONCURRENTLY cannot run inside a transaction block
+COMMIT;
+-- REINDEX SCHEMA processes all temporary relations
+CREATE TABLE reindex_temp_before AS
+SELECT oid, relname, relfilenode, relkind, reltoastrelid
+  FROM pg_class
+  WHERE relname IN ('concur_temp_ind_1', 'concur_temp_ind_2');
+SELECT pg_my_temp_schema()::regnamespace as temp_schema_name \gset
+REINDEX SCHEMA :temp_schema_name;
+SELECT  b.relname,
+        b.relkind,
+        CASE WHEN a.relfilenode = b.relfilenode THEN 'relfilenode is unchanged'
+        ELSE 'relfilenode has changed' END
+  FROM reindex_temp_before b JOIN pg_class a ON b.oid = a.oid
+  ORDER BY 1;
+      relname      | relkind |          case           
+-------------------+---------+-------------------------
+ concur_temp_ind_1 | i       | relfilenode has changed
+ concur_temp_ind_2 | i       | relfilenode has changed
+(2 rows)
+
+DROP TABLE concur_temp_tab_1, concur_temp_tab_2, reindex_temp_before;
 --
 -- REINDEX SCHEMA
 --
diff --git a/src/test/regress/sql/create_index.sql b/src/test/regress/sql/create_index.sql
index 73a55ead4b..0df4eec80a 100644
--- a/src/test/regress/sql/create_index.sql
+++ b/src/test/regress/sql/create_index.sql
@@ -489,6 +489,31 @@ VACUUM FULL concur_heap;
 REINDEX TABLE concur_heap;
 \d concur_heap
 
+-- Temporary tables with concurrent builds and on-commit actions
+-- CONCURRENTLY used with CREATE INDEX and DROP INDEX is ignored.
+-- PRESERVE ROWS, the default.
+CREATE TEMP TABLE concur_temp (f1 int, f2 text)
+  ON COMMIT PRESERVE ROWS;
+INSERT INTO concur_temp VALUES (1, 'foo'), (2, 'bar');
+CREATE INDEX CONCURRENTLY concur_temp_ind ON concur_temp(f1);
+DROP INDEX CONCURRENTLY concur_temp_ind;
+DROP TABLE concur_temp;
+-- ON COMMIT DROP
+BEGIN;
+CREATE TEMP TABLE concur_temp (f1 int, f2 text)
+  ON COMMIT DROP;
+INSERT INTO concur_temp VALUES (1, 'foo'), (2, 'bar');
+-- Fails when running in a transaction.
+CREATE INDEX CONCURRENTLY concur_temp_ind ON concur_temp(f1);
+COMMIT;
+-- ON COMMIT DELETE ROWS
+CREATE TEMP TABLE concur_temp (f1 int, f2 text)
+  ON COMMIT DELETE ROWS;
+INSERT INTO concur_temp VALUES (1, 'foo'), (2, 'bar');
+CREATE INDEX CONCURRENTLY concur_temp_ind ON concur_temp(f1);
+DROP INDEX CONCURRENTLY concur_temp_ind;
+DROP TABLE concur_temp;
+
 --
 -- Try some concurrent index drops
 --
@@ -960,6 +985,48 @@ SELECT pg_get_indexdef('concur_exprs_index_pred'::regclass);
 SELECT pg_get_indexdef('concur_exprs_index_pred_2'::regclass);
 DROP TABLE concur_exprs_tab;
 
+-- Temporary tables and on-commit actions, where CONCURRENTLY is ignored.
+-- ON COMMIT PRESERVE ROWS, the default.
+CREATE TEMP TABLE concur_temp_tab_1 (c1 int, c2 text)
+  ON COMMIT PRESERVE ROWS;
+INSERT INTO concur_temp_tab_1 VALUES (1, 'foo'), (2, 'bar');
+CREATE INDEX concur_temp_ind_1 ON concur_temp_tab_1(c2);
+REINDEX TABLE CONCURRENTLY concur_temp_tab_1;
+REINDEX INDEX CONCURRENTLY concur_temp_ind_1;
+-- Still fails in transaction blocks
+BEGIN;
+REINDEX INDEX CONCURRENTLY concur_temp_ind_1;
+COMMIT;
+-- ON COMMIT DELETE ROWS
+CREATE TEMP TABLE concur_temp_tab_2 (c1 int, c2 text)
+  ON COMMIT DELETE ROWS;
+CREATE INDEX concur_temp_ind_2 ON concur_temp_tab_2(c2);
+REINDEX TABLE CONCURRENTLY concur_temp_tab_2;
+REINDEX INDEX CONCURRENTLY concur_temp_ind_2;
+-- ON COMMIT DROP
+BEGIN;
+CREATE TEMP TABLE concur_temp_tab_3 (c1 int, c2 text)
+  ON COMMIT PRESERVE ROWS;
+INSERT INTO concur_temp_tab_3 VALUES (1, 'foo'), (2, 'bar');
+CREATE INDEX concur_temp_ind_3 ON concur_temp_tab_3(c2);
+-- Fails when running in a transaction
+REINDEX INDEX CONCURRENTLY concur_temp_ind_3;
+COMMIT;
+-- REINDEX SCHEMA processes all temporary relations
+CREATE TABLE reindex_temp_before AS
+SELECT oid, relname, relfilenode, relkind, reltoastrelid
+  FROM pg_class
+  WHERE relname IN ('concur_temp_ind_1', 'concur_temp_ind_2');
+SELECT pg_my_temp_schema()::regnamespace as temp_schema_name \gset
+REINDEX SCHEMA :temp_schema_name;
+SELECT  b.relname,
+        b.relkind,
+        CASE WHEN a.relfilenode = b.relfilenode THEN 'relfilenode is unchanged'
+        ELSE 'relfilenode has changed' END
+  FROM reindex_temp_before b JOIN pg_class a ON b.oid = a.oid
+  ORDER BY 1;
+DROP TABLE concur_temp_tab_1, concur_temp_tab_2, reindex_temp_before;
+
 --
 -- REINDEX SCHEMA
 --
diff --git a/doc/src/sgml/ref/create_index.sgml b/doc/src/sgml/ref/create_index.sgml
index 629a31ef79..8cb01107fa 100644
--- a/doc/src/sgml/ref/create_index.sgml
+++ b/doc/src/sgml/ref/create_index.sgml
@@ -129,6 +129,11 @@ CREATE [ UNIQUE ] INDEX [ CONCURRENTLY ] [ [ IF NOT EXISTS ] <replaceable class=
         &mdash; see <xref linkend="sql-createindex-concurrently"
         endterm="sql-createindex-concurrently-title"/>.
        </para>
+       <para>
+        For temporary tables index creation is always non-concurrent, as no
+        other session can access them, and non-concurrent index creation is
+        cheaper.
+       </para>
       </listitem>
      </varlistentry>
 
diff --git a/doc/src/sgml/ref/drop_index.sgml b/doc/src/sgml/ref/drop_index.sgml
index 2a8ca5bf68..0f8b4ed3a9 100644
--- a/doc/src/sgml/ref/drop_index.sgml
+++ b/doc/src/sgml/ref/drop_index.sgml
@@ -58,6 +58,11 @@ DROP INDEX [ CONCURRENTLY ] [ IF EXISTS ] <replaceable class="parameter">name</r
       performed within a transaction block, but
       <command>DROP INDEX CONCURRENTLY</command> cannot.
      </para>
+     <para>
+      For temporary tables index creation is always non-concurrent, as no
+      other session can access them, and non-concurrent index creation is
+      cheaper.
+     </para>
     </listitem>
    </varlistentry>
 
diff --git a/doc/src/sgml/ref/reindex.sgml b/doc/src/sgml/ref/reindex.sgml
index 10881ab03a..d26b746505 100644
--- a/doc/src/sgml/ref/reindex.sgml
+++ b/doc/src/sgml/ref/reindex.sgml
@@ -162,6 +162,11 @@ REINDEX [ ( VERBOSE ) ] { INDEX | TABLE | SCHEMA | DATABASE | SYSTEM } [ CONCURR
       &mdash; see <xref linkend="sql-reindex-concurrently"
       endterm="sql-reindex-concurrently-title"/>.
      </para>
+     <para>
+      For temporary tables, <command>REINDEX</command> is always
+      non-concurrent, as no other session can access them, and
+      non-concurrent index creation is cheaper.
+     </para>
     </listitem>
    </varlistentry>

signature.asc (application/pgp-signature, 833 B)