database/postgres
database/postgres
1
0
Fork 0
mirror of https://github.com/postgres/postgres.git synced 2025-05-06 19:59:18 +03:00
Code

Fix table lock levels for REINDEX INDEX CONCURRENTLY

Browse Source 
REINDEX CONCURRENTLY locks tables with ShareUpdateExclusiveLock rather
than the ShareLock used by a plain REINDEX.  However,
RangeVarCallbackForReindexIndex() was not updated for that and still
used the ShareLock only.  This would lead to lock upgrades later,
leading to possible deadlocks.

Reported-by: Andres Freund <andres@anarazel.de>
Reviewed-by: Andres Freund <andres@anarazel.de>
Reviewed-by: Michael Paquier <michael@paquier.xyz>
Discussion: https://www.postgresql.org/message-id/flat/20190430151735.wi52sxjvxsjvaxxt%40alap3.anarazel.de
This commit is contained in:
Peter Eisentraut 2019-05-08 14:15:01 +02:00
parent 8efe710d9c
commit add85ead4a
1 changed files with 33 additions and 12 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

45
src/backend/commands/indexcmds.c
View File

@ -91,6 +91,15 @@ static bool ReindexRelationConcurrently(Oid relationOid, int options);
static void ReindexPartitionedIndex(Relation parentIdx); static void ReindexPartitionedIndex(Relation parentIdx);
static void update_relispartition(Oid relationId, bool newval); static void update_relispartition(Oid relationId, bool newval);
/*
* callback argument type for RangeVarCallbackForReindexIndex()
*/
struct ReindexIndexCallbackState
{
bool concurrent; /* flag from statement */
Oid locked_table_oid; /* tracks previously locked table */
};
/* /*
* CheckIndexCompatible * CheckIndexCompatible
* Determine whether an existing index definition is compatible with a * Determine whether an existing index definition is compatible with a
@ -2303,8 +2312,8 @@ ChooseIndexColumnNames(List *indexElems)
void void
ReindexIndex(RangeVar *indexRelation, int options, bool concurrent) ReindexIndex(RangeVar *indexRelation, int options, bool concurrent)
{ {
struct ReindexIndexCallbackState state;
Oid indOid; Oid indOid;
Oid heapOid = InvalidOid;
Relation irel; Relation irel;
char persistence; char persistence;
@ -2313,11 +2322,13 @@ ReindexIndex(RangeVar *indexRelation, int options, bool concurrent)
* obtain lock on table first, to avoid deadlock hazard. The lock level * obtain lock on table first, to avoid deadlock hazard. The lock level
* used here must match the index lock obtained in reindex_index(). * used here must match the index lock obtained in reindex_index().
*/ */
state.concurrent = concurrent;
state.locked_table_oid = InvalidOid;
indOid = RangeVarGetRelidExtended(indexRelation, indOid = RangeVarGetRelidExtended(indexRelation,
concurrent ? ShareUpdateExclusiveLock : AccessExclusiveLock, concurrent ? ShareUpdateExclusiveLock : AccessExclusiveLock,
0, 0,
RangeVarCallbackForReindexIndex, RangeVarCallbackForReindexIndex,
(void *) &heapOid); &state);
/* /*
* Obtain the current persistence of the existing index. We already hold * Obtain the current persistence of the existing index. We already hold
@ -2350,7 +2361,15 @@ RangeVarCallbackForReindexIndex(const RangeVar *relation,
Oid relId, Oid oldRelId, void *arg) Oid relId, Oid oldRelId, void *arg)
{ {
char relkind; char relkind;
Oid *heapOid = (Oid *) arg; struct ReindexIndexCallbackState *state = arg;
LOCKMODE table_lockmode;
/*
* Lock level here should match table lock in reindex_index() for
* non-concurrent case and table locks used by index_concurrently_*() for
* concurrent case.
*/
table_lockmode = state->concurrent ? ShareUpdateExclusiveLock : ShareLock;
/* /*
* If we previously locked some other index's heap, and the name we're * If we previously locked some other index's heap, and the name we're
@ -2359,9 +2378,8 @@ RangeVarCallbackForReindexIndex(const RangeVar *relation,
*/ */
if (relId != oldRelId && OidIsValid(oldRelId)) if (relId != oldRelId && OidIsValid(oldRelId))
{ {
/* lock level here should match reindex_index() heap lock */ UnlockRelationOid(state->locked_table_oid, table_lockmode);
UnlockRelationOid(*heapOid, ShareLock); state->locked_table_oid = InvalidOid;
*heapOid = InvalidOid;
} }
/* If the relation does not exist, there's nothing more to do. */ /* If the relation does not exist, there's nothing more to do. */
@ -2389,14 +2407,17 @@ RangeVarCallbackForReindexIndex(const RangeVar *relation,
/* Lock heap before index to avoid deadlock. */ /* Lock heap before index to avoid deadlock. */
if (relId != oldRelId) if (relId != oldRelId)
{ {
Oid table_oid = IndexGetRelation(relId, true);
/* /*
* Lock level here should match reindex_index() heap lock. If the OID * If the OID isn't valid, it means the index was concurrently
* isn't valid, it means the index as concurrently dropped, which is * dropped, which is not a problem for us; just return normally.
* not a problem for us; just return normally.
*/ */
*heapOid = IndexGetRelation(relId, true); if (OidIsValid(table_oid))
if (OidIsValid(*heapOid)) {
LockRelationOid(*heapOid, ShareLock); LockRelationOid(table_oid, table_lockmode);
state->locked_table_oid = table_oid;
}
} }
} }