Handle interrupts within a transaction context in REINDEX CONCURRENTLY

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
index 9c9ddd0fd78513fb61f1ca5832e9a9611aed6c46..90378474d7dd41f94e0b7a1115e60dbe14790778 100644 (file)
--- 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);
        }