Fix possible crash during FATAL exit from reindexing.

index.c supposed that it could just use a PG_TRY block to clean up the
state associated with an active REINDEX operation.  However, that code
doesn't run if we do a FATAL exit --- for example, due to a SIGTERM
shutdown signal --- while the REINDEX is happening.  And that state does
get consulted during catalog accesses, which makes it problematic if we
do any catalog accesses during shutdown --- for example, to clean up any
temp tables created in the session.

If this combination of circumstances occurred, we could find ourselves
trying to access already-freed memory.  In debug builds that'd fairly
reliably cause an assertion failure.  In production we might often
get away with it, but with some bad luck it could cause a core dump.

Another possible bad outcome is an erroneous conclusion that an
index-to-be-accessed is being reindexed; but it looks like that would
be unlikely to have any consequences worse than failing to drop temp
tables right away.  (They'd still get dropped by the next session that
uses that temp schema.)

To fix, get rid of the use of PG_TRY here, and instead hook into
the transaction abort mechanisms to clean up reindex state.

Per bug #16378 from Alexander Lakhin.  This has been wrong for a
very long time, so back-patch to all supported branches.

Discussion: https://postgr.es/m/16378-7a70ca41b3ec2009@postgresql.org

diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
index 9b252213d95137ba4b7b3a16386e670b6eb5a1e7..37f31a2c31ead06cf6572ac493c48bdcd7555edf 100644 (file)
--- a/src/backend/access/transam/xact.c
+++ b/src/backend/access/transam/xact.c
@@ -31,6 +31,7 @@
 #include "access/xloginsert.h"
 #include "access/xlogutils.h"
 #include "catalog/catalog.h"
+#include "catalog/index.h"
 #include "catalog/namespace.h"
 #include "catalog/storage.h"
 #include "commands/async.h"
@@ -2577,6 +2578,9 @@ AbortTransaction(void)
     */
    SetUserIdAndSecContext(s->prevUser, s->prevSecContext);
 
+   /* Forget about any active REINDEX. */
+   ResetReindexState(s->nestingLevel);
+
    /* If in parallel mode, clean up workers and exit parallel mode. */
    if (IsInParallelMode())
    {
@@ -4684,6 +4688,9 @@ AbortSubTransaction(void)
     */
    SetUserIdAndSecContext(s->prevUser, s->prevSecContext);
 
+   /* Forget about any active REINDEX. */
+   ResetReindexState(s->nestingLevel);
+
    /* Exit from parallel mode, if necessary. */
    if (IsInParallelMode())
    {

diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index 9a0f45ac3e0b1835f3c489c877abc2e86c98da2a..ee2a0e9fe1970d474d1ae6e6b92811e991c799b8 100644 (file)
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -126,7 +126,6 @@ static void SetReindexProcessing(Oid heapOid, Oid indexOid);
 static void ResetReindexProcessing(void);
 static void SetReindexPending(List *indexes);
 static void RemoveReindexPending(Oid indexOid);
-static void ResetReindexPending(void);
 
 
 /*
@@ -3457,27 +3456,18 @@ reindex_index(Oid indexId, bool skip_constraint_checks, char persistence,
        indexInfo->ii_ExclusionStrats = NULL;
    }
 
-   /* ensure SetReindexProcessing state isn't leaked */
-   PG_TRY();
-   {
-       /* Suppress use of the target index while rebuilding it */
-       SetReindexProcessing(heapId, indexId);
+   /* Suppress use of the target index while rebuilding it */
+   SetReindexProcessing(heapId, indexId);
 
-       /* Create a new physical relation for the index */
-       RelationSetNewRelfilenode(iRel, persistence, InvalidTransactionId,
-                                 InvalidMultiXactId);
+   /* Create a new physical relation for the index */
+   RelationSetNewRelfilenode(iRel, persistence, InvalidTransactionId,
+                             InvalidMultiXactId);
 
-       /* Initialize the index and rebuild */
-       /* Note: we do not need to re-establish pkey setting */
-       index_build(heapRelation, iRel, indexInfo, false, true);
-   }
-   PG_CATCH();
-   {
-       /* Make sure flag gets cleared on error exit */
-       ResetReindexProcessing();
-       PG_RE_THROW();
-   }
-   PG_END_TRY();
+   /* Initialize the index and rebuild */
+   /* Note: we do not need to re-establish pkey setting */
+   index_build(heapRelation, iRel, indexInfo, false, true);
+
+   /* Re-allow use of target index */
    ResetReindexProcessing();
 
    /*
@@ -3612,7 +3602,9 @@ reindex_relation(Oid relid, int flags, int options)
    Relation    rel;
    Oid         toast_relid;
    List       *indexIds;
+   char        persistence;
    bool        result;
+   ListCell   *indexId;
 
    /*
     * Open and lock the relation.  ShareLock is sufficient since we only need
@@ -3630,56 +3622,42 @@ reindex_relation(Oid relid, int flags, int options)
     */
    indexIds = RelationGetIndexList(rel);
 
-   PG_TRY();
+   if (flags & REINDEX_REL_SUPPRESS_INDEX_USE)
    {
-       ListCell   *indexId;
-       char        persistence;
-
-       if (flags & REINDEX_REL_SUPPRESS_INDEX_USE)
-       {
-           /* Suppress use of all the indexes until they are rebuilt */
-           SetReindexPending(indexIds);
-
-           /*
-            * Make the new heap contents visible --- now things might be
-            * inconsistent!
-            */
-           CommandCounterIncrement();
-       }
+       /* Suppress use of all the indexes until they are rebuilt */
+       SetReindexPending(indexIds);
 
        /*
-        * Compute persistence of indexes: same as that of owning rel, unless
-        * caller specified otherwise.
+        * Make the new heap contents visible --- now things might be
+        * inconsistent!
         */
-       if (flags & REINDEX_REL_FORCE_INDEXES_UNLOGGED)
-           persistence = RELPERSISTENCE_UNLOGGED;
-       else if (flags & REINDEX_REL_FORCE_INDEXES_PERMANENT)
-           persistence = RELPERSISTENCE_PERMANENT;
-       else
-           persistence = rel->rd_rel->relpersistence;
+       CommandCounterIncrement();
+   }
 
-       /* Reindex all the indexes. */
-       foreach(indexId, indexIds)
-       {
-           Oid         indexOid = lfirst_oid(indexId);
+   /*
+    * Compute persistence of indexes: same as that of owning rel, unless
+    * caller specified otherwise.
+    */
+   if (flags & REINDEX_REL_FORCE_INDEXES_UNLOGGED)
+       persistence = RELPERSISTENCE_UNLOGGED;
+   else if (flags & REINDEX_REL_FORCE_INDEXES_PERMANENT)
+       persistence = RELPERSISTENCE_PERMANENT;
+   else
+       persistence = rel->rd_rel->relpersistence;
 
-           reindex_index(indexOid, !(flags & REINDEX_REL_CHECK_CONSTRAINTS),
-                         persistence, options);
+   /* Reindex all the indexes. */
+   foreach(indexId, indexIds)
+   {
+       Oid         indexOid = lfirst_oid(indexId);
 
-           CommandCounterIncrement();
+       reindex_index(indexOid, !(flags & REINDEX_REL_CHECK_CONSTRAINTS),
+                     persistence, options);
 
-           /* Index should no longer be in the pending list */
-           Assert(!ReindexIsProcessingIndex(indexOid));
-       }
-   }
-   PG_CATCH();
-   {
-       /* Make sure list gets cleared on error exit */
-       ResetReindexPending();
-       PG_RE_THROW();
+       CommandCounterIncrement();
+
+       /* Index should no longer be in the pending list */
+       Assert(!ReindexIsProcessingIndex(indexOid));
    }
-   PG_END_TRY();
-   ResetReindexPending();
 
    /*
     * Close rel, but continue to hold the lock.
@@ -3712,6 +3690,7 @@ reindex_relation(Oid relid, int flags, int options)
 static Oid currentlyReindexedHeap = InvalidOid;
 static Oid currentlyReindexedIndex = InvalidOid;
 static List *pendingReindexedIndexes = NIL;
+static int reindexingNestLevel = 0;
 
 /*
  * ReindexIsProcessingHeap
@@ -3748,8 +3727,6 @@ ReindexIsProcessingIndex(Oid indexOid)
 /*
  * SetReindexProcessing
  *     Set flag that specified heap/index are being reindexed.
- *
- * NB: caller must use a PG_TRY block to ensure ResetReindexProcessing is done.
  */
 static void
 SetReindexProcessing(Oid heapOid, Oid indexOid)
@@ -3762,6 +3739,8 @@ SetReindexProcessing(Oid heapOid, Oid indexOid)
    currentlyReindexedIndex = indexOid;
    /* Index is no longer "pending" reindex. */
    RemoveReindexPending(indexOid);
+   /* This may have been set already, but in case it isn't, do so now. */
+   reindexingNestLevel = GetCurrentTransactionNestLevel();
 }
 
 /*
@@ -3773,14 +3752,14 @@ ResetReindexProcessing(void)
 {
    currentlyReindexedHeap = InvalidOid;
    currentlyReindexedIndex = InvalidOid;
+   /* reindexingNestLevel remains set till end of (sub)transaction */
 }
 
 /*
  * SetReindexPending
  *     Mark the given indexes as pending reindex.
  *
- * NB: caller must use a PG_TRY block to ensure ResetReindexPending is done.
- * Also, we assume that the current memory context stays valid throughout.
+ * NB: we assume that the current memory context stays valid throughout.
  */
 static void
 SetReindexPending(List *indexes)
@@ -3789,6 +3768,7 @@ SetReindexPending(List *indexes)
    if (pendingReindexedIndexes)
        elog(ERROR, "cannot reindex while reindexing");
    pendingReindexedIndexes = list_copy(indexes);
+   reindexingNestLevel = GetCurrentTransactionNestLevel();
 }
 
 /*
@@ -3803,11 +3783,30 @@ RemoveReindexPending(Oid indexOid)
 }
 
 /*
- * ResetReindexPending
- *     Unset reindex-pending status.
+ * ResetReindexState
+ *     Clear all reindexing state during (sub)transaction abort.
  */
-static void
-ResetReindexPending(void)
+void
+ResetReindexState(int nestLevel)
 {
-   pendingReindexedIndexes = NIL;
+   /*
+    * Because reindexing is not re-entrant, we don't need to cope with nested
+    * reindexing states.  We just need to avoid messing up the outer-level
+    * state in case a subtransaction fails within a REINDEX.  So checking the
+    * current nest level against that of the reindex operation is sufficient.
+    */
+   if (reindexingNestLevel >= nestLevel)
+   {
+       currentlyReindexedHeap = InvalidOid;
+       currentlyReindexedIndex = InvalidOid;
+
+       /*
+        * We needn't try to release the contents of pendingReindexedIndexes;
+        * that list should be in a transaction-lifespan context, so it will
+        * go away automatically.
+        */
+       pendingReindexedIndexes = NIL;
+
+       reindexingNestLevel = 0;
+   }
 }

diff --git a/src/include/catalog/index.h b/src/include/catalog/index.h
index 7c56182c72311b97b8b374b8e6f95bfbdb0f89ed..7a7ad3e3d13eb3cb2cb891559d8a475baf42822e 100644 (file)
--- a/src/include/catalog/index.h
+++ b/src/include/catalog/index.h
@@ -118,6 +118,8 @@ extern void validate_index(Oid heapId, Oid indexId, Snapshot snapshot);
 
 extern void index_set_state_flags(Oid indexId, IndexStateFlagsAction action);
 
+extern Oid IndexGetRelation(Oid indexId, bool missing_ok);
+
 extern void reindex_index(Oid indexId, bool skip_constraint_checks,
              char relpersistence, int options);
 
@@ -132,6 +134,7 @@ extern bool reindex_relation(Oid relid, int flags, int options);
 
 extern bool ReindexIsProcessingHeap(Oid heapOid);
 extern bool ReindexIsProcessingIndex(Oid indexOid);
-extern Oid IndexGetRelation(Oid indexId, bool missing_ok);
+
+extern void ResetReindexState(int nestLevel);
 
 #endif                         /* INDEX_H */