Handle interrupts within a transaction context in REINDEX CONCURRENTLY

michaelpq · michaelpq · commit 7f84b0ef0bfb · 2019-10-25T10:20:16.000+09:00
Phases 2 (building the new index) and 3 (validating the new index) checked for interrupts outside a transaction context, having as consequence to not release session-level locks taken on the parent relation and the old and new indexes processed. This could for example be triggered with statement_timeout and a bad timing, and would issue confusing error messages when shutting down the session still holding the locks (note that an assertion failure would be triggered first), on top of more issues with concurrent sessions trying to take a lock that would interfere with the SHARE UPDATE EXCLUSIVE locks hold here. This moves all the interruption checks inside a transaction context. Note that I have manually tested all interruptions to make sure that invalid indexes can be cleaned up properly. Partition indexes still have issues on their own with some missing dependency handling, which will be dealt with in a follow-up patch. Reported-by: Justin Pryzby Author: Michael Paquier Discussion: https://postgr.es/m/20191013025145.GC4475@telsasoft.com Backpatch-through: 12
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
@@ -3058,11 +3058,16 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 		Oid			newIndexId = lfirst_oid(lc2);
 		Oid			heapId;
 
-		CHECK_FOR_INTERRUPTS();
-
 		/* Start new transaction for this index's concurrent build */
 		StartTransactionCommand();
 
+		/*
+		 * Check for user-requested abort.  This is inside a transaction so as
+		 * xact.c does not issue a useless WARNING, and ensures that
+		 * session-level locks are cleaned up on abort.
+		 */
+		CHECK_FOR_INTERRUPTS();
+
 		/* Set ActiveSnapshot since functions in the indexes may need it */
 		PushActiveSnapshot(GetTransactionSnapshot());
 
@@ -3102,10 +3107,15 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 		TransactionId limitXmin;
 		Snapshot	snapshot;
 
-		CHECK_FOR_INTERRUPTS();
-
 		StartTransactionCommand();
 
+		/*
+		 * Check for user-requested abort.  This is inside a transaction so as
+		 * xact.c does not issue a useless WARNING, and ensures that
+		 * session-level locks are cleaned up on abort.
+		 */
+		CHECK_FOR_INTERRUPTS();
+
 		heapId = IndexGetRelation(newIndexId, false);
 
 		/*
@@ -3167,6 +3177,11 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 		Oid			newIndexId = lfirst_oid(lc2);
 		Oid			heapId;
 
+		/*
+		 * Check for user-requested abort.  This is inside a transaction so as
+		 * xact.c does not issue a useless WARNING, and ensures that
+		 * session-level locks are cleaned up on abort.
+		 */
 		CHECK_FOR_INTERRUPTS();
 
 		heapId = IndexGetRelation(oldIndexId, false);
@@ -3222,7 +3237,13 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 		Oid			oldIndexId = lfirst_oid(lc);
 		Oid			heapId;
 
+		/*
+		 * Check for user-requested abort.  This is inside a transaction so as
+		 * xact.c does not issue a useless WARNING, and ensures that
+		 * session-level locks are cleaned up on abort.
+		 */
 		CHECK_FOR_INTERRUPTS();
+
 		heapId = IndexGetRelation(oldIndexId, false);
 		index_concurrently_set_dead(heapId, oldIndexId);
 	}
