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 02aadc0ed4e85b76122819195f0b9bb93b30ddeb..c2e1222887f9210a5ea18d8f930b1daf22c7b6f9 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"
@@ -2548,6 +2549,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())
    {
@@ -4650,6 +4654,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 4e901c39a67df783dae4d417ea1ac34856a1dbc2..f5c12d3d1c9e089768f74291442f3cf1c536a7fc 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);
 
 
 /*
@@ -3440,27 +3439,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();
 
    /*
@@ -3596,7 +3586,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
@@ -3614,56 +3606,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.
@@ -3696,6 +3674,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
@@ -3732,8 +3711,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)
@@ -3746,6 +3723,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();
 }
 
 /*
@@ -3757,14 +3736,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)
@@ -3773,6 +3752,7 @@ SetReindexPending(List *indexes)
    if (pendingReindexedIndexes)
        elog(ERROR, "cannot reindex while reindexing");
    pendingReindexedIndexes = list_copy(indexes);
+   reindexingNestLevel = GetCurrentTransactionNestLevel();
 }
 
 /*
@@ -3787,11 +3767,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 5941fec164ab3c105f7d034fc755eb05b0e9592f..b759cbb3b22e49b87978f8a13a41836e55b4be14 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 */