From 5c1c27eef94a3d22f2fa4feab922c4af72a3a669 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Tue, 21 Apr 2020 15:58:43 -0400 Subject: [PATCH] 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 --- src/backend/access/transam/xact.c | 7 ++ src/backend/catalog/index.c | 143 +++++++++++++++--------------- src/include/catalog/index.h | 5 +- 3 files changed, 82 insertions(+), 73 deletions(-) diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c index 02aadc0ed4e..c2e1222887f 100644 --- 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 4e901c39a67..f5c12d3d1c9 100644 --- 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; - - /* Reindex all the indexes. */ - foreach(indexId, indexIds) - { - Oid indexOid = lfirst_oid(indexId); - - reindex_index(indexOid, !(flags & REINDEX_REL_CHECK_CONSTRAINTS), - persistence, options); - - CommandCounterIncrement(); - - /* Index should no longer be in the pending list */ - Assert(!ReindexIsProcessingIndex(indexOid)); - } + CommandCounterIncrement(); } - PG_CATCH(); + + /* + * 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 all the indexes. */ + foreach(indexId, indexIds) { - /* Make sure list gets cleared on error exit */ - ResetReindexPending(); - PG_RE_THROW(); + Oid indexOid = lfirst_oid(indexId); + + reindex_index(indexOid, !(flags & REINDEX_REL_CHECK_CONSTRAINTS), + persistence, options); + + 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 5941fec164a..b759cbb3b22 100644 --- 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 */