Location via proxy:   [ UP ]  
[Report a bug]   [Manage cookies]                
Skip to content

Commit d12bdba

Browse files
committed
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
1 parent 5836d32 commit d12bdba

File tree

3 files changed

+104
-91
lines changed

3 files changed

+104
-91
lines changed

src/backend/access/transam/xact.c

+7
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,7 @@
3030
#include "access/xlog.h"
3131
#include "access/xloginsert.h"
3232
#include "access/xlogutils.h"
33+
#include "catalog/index.h"
3334
#include "catalog/namespace.h"
3435
#include "catalog/pg_enum.h"
3536
#include "catalog/storage.h"
@@ -2662,6 +2663,9 @@ AbortTransaction(void)
26622663
*/
26632664
SetUserIdAndSecContext(s->prevUser, s->prevSecContext);
26642665

2666+
/* Forget about any active REINDEX. */
2667+
ResetReindexState(s->nestingLevel);
2668+
26652669
/* If in parallel mode, clean up workers and exit parallel mode. */
26662670
if (IsInParallelMode())
26672671
{
@@ -4961,6 +4965,9 @@ AbortSubTransaction(void)
49614965
*/
49624966
SetUserIdAndSecContext(s->prevUser, s->prevSecContext);
49634967

4968+
/* Forget about any active REINDEX. */
4969+
ResetReindexState(s->nestingLevel);
4970+
49644971
/* Exit from parallel mode, if necessary. */
49654972
if (IsInParallelMode())
49664973
{

src/backend/catalog/index.c

+94-90
Original file line numberDiff line numberDiff line change
@@ -131,7 +131,6 @@ static void SetReindexProcessing(Oid heapOid, Oid indexOid);
131131
static void ResetReindexProcessing(void);
132132
static void SetReindexPending(List *indexes);
133133
static void RemoveReindexPending(Oid indexOid);
134-
static void ResetReindexPending(void);
135134

136135

137136
/*
@@ -1543,8 +1542,8 @@ index_concurrently_swap(Oid newIndexId, Oid oldIndexId, const char *oldName)
15431542
newIndexForm->indisclustered = oldIndexForm->indisclustered;
15441543

15451544
/*
1546-
* Mark the new index as valid, and the old index as invalid similarly
1547-
* to what index_set_state_flags() does.
1545+
* Mark the new index as valid, and the old index as invalid similarly to
1546+
* what index_set_state_flags() does.
15481547
*/
15491548
newIndexForm->indisvalid = true;
15501549
oldIndexForm->indisvalid = false;
@@ -3525,25 +3524,18 @@ reindex_index(Oid indexId, bool skip_constraint_checks, char persistence,
35253524
indexInfo->ii_ExclusionStrats = NULL;
35263525
}
35273526

3528-
/* ensure SetReindexProcessing state isn't leaked */
3529-
PG_TRY();
3530-
{
3531-
/* Suppress use of the target index while rebuilding it */
3532-
SetReindexProcessing(heapId, indexId);
3527+
/* Suppress use of the target index while rebuilding it */
3528+
SetReindexProcessing(heapId, indexId);
35333529

3534-
/* Create a new physical relation for the index */
3535-
RelationSetNewRelfilenode(iRel, persistence);
3530+
/* Create a new physical relation for the index */
3531+
RelationSetNewRelfilenode(iRel, persistence);
35363532

3537-
/* Initialize the index and rebuild */
3538-
/* Note: we do not need to re-establish pkey setting */
3539-
index_build(heapRelation, iRel, indexInfo, true, true);
3540-
}
3541-
PG_FINALLY();
3542-
{
3543-
/* Make sure flag gets cleared on error exit */
3544-
ResetReindexProcessing();
3545-
}
3546-
PG_END_TRY();
3533+
/* Initialize the index and rebuild */
3534+
/* Note: we do not need to re-establish pkey setting */
3535+
index_build(heapRelation, iRel, indexInfo, true, true);
3536+
3537+
/* Re-allow use of target index */
3538+
ResetReindexProcessing();
35473539

35483540
/*
35493541
* If the index is marked invalid/not-ready/dead (ie, it's from a failed
@@ -3680,7 +3672,9 @@ reindex_relation(Oid relid, int flags, int options)
36803672
Relation rel;
36813673
Oid toast_relid;
36823674
List *indexIds;
3675+
char persistence;
36833676
bool result;
3677+
ListCell *indexId;
36843678
int i;
36853679

36863680
/*
@@ -3715,77 +3709,65 @@ reindex_relation(Oid relid, int flags, int options)
37153709
*/
37163710
indexIds = RelationGetIndexList(rel);
37173711

3718-
PG_TRY();
3712+
if (flags & REINDEX_REL_SUPPRESS_INDEX_USE)
37193713
{
3720-
ListCell *indexId;
3721-
char persistence;
3714+
/* Suppress use of all the indexes until they are rebuilt */
3715+
SetReindexPending(indexIds);
37223716

3723-
if (flags & REINDEX_REL_SUPPRESS_INDEX_USE)
3724-
{
3725-
/* Suppress use of all the indexes until they are rebuilt */
3726-
SetReindexPending(indexIds);
3717+
/*
3718+
* Make the new heap contents visible --- now things might be
3719+
* inconsistent!
3720+
*/
3721+
CommandCounterIncrement();
3722+
}
37273723

3728-
/*
3729-
* Make the new heap contents visible --- now things might be
3730-
* inconsistent!
3731-
*/
3732-
CommandCounterIncrement();
3733-
}
3724+
/*
3725+
* Compute persistence of indexes: same as that of owning rel, unless
3726+
* caller specified otherwise.
3727+
*/
3728+
if (flags & REINDEX_REL_FORCE_INDEXES_UNLOGGED)
3729+
persistence = RELPERSISTENCE_UNLOGGED;
3730+
else if (flags & REINDEX_REL_FORCE_INDEXES_PERMANENT)
3731+
persistence = RELPERSISTENCE_PERMANENT;
3732+
else
3733+
persistence = rel->rd_rel->relpersistence;
3734+
3735+
/* Reindex all the indexes. */
3736+
i = 1;
3737+
foreach(indexId, indexIds)
3738+
{
3739+
Oid indexOid = lfirst_oid(indexId);
3740+
Oid indexNamespaceId = get_rel_namespace(indexOid);
37343741

37353742
/*
3736-
* Compute persistence of indexes: same as that of owning rel, unless
3737-
* caller specified otherwise.
3743+
* Skip any invalid indexes on a TOAST table. These can only be
3744+
* duplicate leftovers from a failed REINDEX CONCURRENTLY, and if
3745+
* rebuilt it would not be possible to drop them anymore.
37383746
*/
3739-
if (flags & REINDEX_REL_FORCE_INDEXES_UNLOGGED)
3740-
persistence = RELPERSISTENCE_UNLOGGED;
3741-
else if (flags & REINDEX_REL_FORCE_INDEXES_PERMANENT)
3742-
persistence = RELPERSISTENCE_PERMANENT;
3743-
else
3744-
persistence = rel->rd_rel->relpersistence;
3745-
3746-
/* Reindex all the indexes. */
3747-
i = 1;
3748-
foreach(indexId, indexIds)
3747+
if (IsToastNamespace(indexNamespaceId) &&
3748+
!get_index_isvalid(indexOid))
37493749
{
3750-
Oid indexOid = lfirst_oid(indexId);
3751-
Oid indexNamespaceId = get_rel_namespace(indexOid);
3752-
3753-
/*
3754-
* Skip any invalid indexes on a TOAST table. These can only be
3755-
* duplicate leftovers from a failed REINDEX CONCURRENTLY, and if
3756-
* rebuilt it would not be possible to drop them anymore.
3757-
*/
3758-
if (IsToastNamespace(indexNamespaceId) &&
3759-
!get_index_isvalid(indexOid))
3760-
{
3761-
ereport(WARNING,
3762-
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
3763-
errmsg("cannot reindex invalid index \"%s.%s\" on TOAST table, skipping",
3764-
get_namespace_name(indexNamespaceId),
3765-
get_rel_name(indexOid))));
3766-
continue;
3767-
}
3750+
ereport(WARNING,
3751+
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
3752+
errmsg("cannot reindex invalid index \"%s.%s\" on TOAST table, skipping",
3753+
get_namespace_name(indexNamespaceId),
3754+
get_rel_name(indexOid))));
3755+
continue;
3756+
}
37683757

3769-
reindex_index(indexOid, !(flags & REINDEX_REL_CHECK_CONSTRAINTS),
3770-
persistence, options);
3758+
reindex_index(indexOid, !(flags & REINDEX_REL_CHECK_CONSTRAINTS),
3759+
persistence, options);
37713760

3772-
CommandCounterIncrement();
3761+
CommandCounterIncrement();
37733762

3774-
/* Index should no longer be in the pending list */
3775-
Assert(!ReindexIsProcessingIndex(indexOid));
3763+
/* Index should no longer be in the pending list */
3764+
Assert(!ReindexIsProcessingIndex(indexOid));
37763765

3777-
/* Set index rebuild count */
3778-
pgstat_progress_update_param(PROGRESS_CLUSTER_INDEX_REBUILD_COUNT,
3779-
i);
3780-
i++;
3781-
}
3766+
/* Set index rebuild count */
3767+
pgstat_progress_update_param(PROGRESS_CLUSTER_INDEX_REBUILD_COUNT,
3768+
i);
3769+
i++;
37823770
}
3783-
PG_FINALLY();
3784-
{
3785-
/* Make sure list gets cleared on error exit */
3786-
ResetReindexPending();
3787-
}
3788-
PG_END_TRY();
37893771

37903772
/*
37913773
* Close rel, but continue to hold the lock.
@@ -3819,6 +3801,7 @@ reindex_relation(Oid relid, int flags, int options)
38193801
static Oid currentlyReindexedHeap = InvalidOid;
38203802
static Oid currentlyReindexedIndex = InvalidOid;
38213803
static List *pendingReindexedIndexes = NIL;
3804+
static int reindexingNestLevel = 0;
38223805

38233806
/*
38243807
* ReindexIsProcessingHeap
@@ -3855,8 +3838,6 @@ ReindexIsProcessingIndex(Oid indexOid)
38553838
/*
38563839
* SetReindexProcessing
38573840
* Set flag that specified heap/index are being reindexed.
3858-
*
3859-
* NB: caller must use a PG_TRY block to ensure ResetReindexProcessing is done.
38603841
*/
38613842
static void
38623843
SetReindexProcessing(Oid heapOid, Oid indexOid)
@@ -3869,6 +3850,8 @@ SetReindexProcessing(Oid heapOid, Oid indexOid)
38693850
currentlyReindexedIndex = indexOid;
38703851
/* Index is no longer "pending" reindex. */
38713852
RemoveReindexPending(indexOid);
3853+
/* This may have been set already, but in case it isn't, do so now. */
3854+
reindexingNestLevel = GetCurrentTransactionNestLevel();
38723855
}
38733856

38743857
/*
@@ -3878,17 +3861,16 @@ SetReindexProcessing(Oid heapOid, Oid indexOid)
38783861
static void
38793862
ResetReindexProcessing(void)
38803863
{
3881-
/* This may be called in leader error path */
38823864
currentlyReindexedHeap = InvalidOid;
38833865
currentlyReindexedIndex = InvalidOid;
3866+
/* reindexingNestLevel remains set till end of (sub)transaction */
38843867
}
38853868

38863869
/*
38873870
* SetReindexPending
38883871
* Mark the given indexes as pending reindex.
38893872
*
3890-
* NB: caller must use a PG_TRY block to ensure ResetReindexPending is done.
3891-
* Also, we assume that the current memory context stays valid throughout.
3873+
* NB: we assume that the current memory context stays valid throughout.
38923874
*/
38933875
static void
38943876
SetReindexPending(List *indexes)
@@ -3899,6 +3881,7 @@ SetReindexPending(List *indexes)
38993881
if (IsInParallelMode())
39003882
elog(ERROR, "cannot modify reindex state during a parallel operation");
39013883
pendingReindexedIndexes = list_copy(indexes);
3884+
reindexingNestLevel = GetCurrentTransactionNestLevel();
39023885
}
39033886

39043887
/*
@@ -3915,14 +3898,32 @@ RemoveReindexPending(Oid indexOid)
39153898
}
39163899

39173900
/*
3918-
* ResetReindexPending
3919-
* Unset reindex-pending status.
3901+
* ResetReindexState
3902+
* Clear all reindexing state during (sub)transaction abort.
39203903
*/
3921-
static void
3922-
ResetReindexPending(void)
3904+
void
3905+
ResetReindexState(int nestLevel)
39233906
{
3924-
/* This may be called in leader error path */
3925-
pendingReindexedIndexes = NIL;
3907+
/*
3908+
* Because reindexing is not re-entrant, we don't need to cope with nested
3909+
* reindexing states. We just need to avoid messing up the outer-level
3910+
* state in case a subtransaction fails within a REINDEX. So checking the
3911+
* current nest level against that of the reindex operation is sufficient.
3912+
*/
3913+
if (reindexingNestLevel >= nestLevel)
3914+
{
3915+
currentlyReindexedHeap = InvalidOid;
3916+
currentlyReindexedIndex = InvalidOid;
3917+
3918+
/*
3919+
* We needn't try to release the contents of pendingReindexedIndexes;
3920+
* that list should be in a transaction-lifespan context, so it will
3921+
* go away automatically.
3922+
*/
3923+
pendingReindexedIndexes = NIL;
3924+
3925+
reindexingNestLevel = 0;
3926+
}
39263927
}
39273928

39283929
/*
@@ -3975,4 +3976,7 @@ RestoreReindexState(void *reindexstate)
39753976
lappend_oid(pendingReindexedIndexes,
39763977
sistate->pendingReindexedIndexes[c]);
39773978
MemoryContextSwitchTo(oldcontext);
3979+
3980+
/* Note the worker has its own transaction nesting level */
3981+
reindexingNestLevel = GetCurrentTransactionNestLevel();
39783982
}

src/include/catalog/index.h

+3-1
Original file line numberDiff line numberDiff line change
@@ -131,6 +131,8 @@ extern void validate_index(Oid heapId, Oid indexId, Snapshot snapshot);
131131

132132
extern void index_set_state_flags(Oid indexId, IndexStateFlagsAction action);
133133

134+
extern Oid IndexGetRelation(Oid indexId, bool missing_ok);
135+
134136
extern void reindex_index(Oid indexId, bool skip_constraint_checks,
135137
char relpersistence, int options);
136138

@@ -145,8 +147,8 @@ extern bool reindex_relation(Oid relid, int flags, int options);
145147

146148
extern bool ReindexIsProcessingHeap(Oid heapOid);
147149
extern bool ReindexIsProcessingIndex(Oid indexOid);
148-
extern Oid IndexGetRelation(Oid indexId, bool missing_ok);
149150

151+
extern void ResetReindexState(int nestLevel);
150152
extern Size EstimateReindexStateSpace(void);
151153
extern void SerializeReindexState(Size maxsize, char *start_address);
152154
extern void RestoreReindexState(void *reindexstate);

0 commit comments

Comments
 (0)