On 2019-05-02 10:44, Peter Eisentraut wrote: >> so there's a lock upgrade hazard. > Confirmed.
Here is a patch along the lines of your sketch. I cleaned up the variable naming a bit too. REINDEX CONCURRENTLY is still deadlock prone because of WaitForOlderSnapshots(), so this doesn't actually fix your test case, but that seems unrelated to this particular issue. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
From 8a4dcc3e6b849fd72bed29c9308c9a897eec0cc5 Mon Sep 17 00:00:00 2001 From: Peter Eisentraut <pe...@eisentraut.org> Date: Fri, 3 May 2019 09:15:29 +0200 Subject: [PATCH] Fix table lock levels for REINDEX INDEX CONCURRENTLY 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 <and...@anarazel.de> Discussion: https://www.postgresql.org/message-id/flat/20190430151735.wi52sxjvxsjvaxxt%40alap3.anarazel.de --- src/backend/commands/indexcmds.c | 45 +++++++++++++++++++++++--------- 1 file changed, 33 insertions(+), 12 deletions(-) diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c index fabba3bacd..acfb2d24c9 100644 --- a/src/backend/commands/indexcmds.c +++ b/src/backend/commands/indexcmds.c @@ -91,6 +91,15 @@ static bool ReindexRelationConcurrently(Oid relationOid, int options); static void ReindexPartitionedIndex(Relation parentIdx); 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 * Determine whether an existing index definition is compatible with a @@ -2303,8 +2312,8 @@ ChooseIndexColumnNames(List *indexElems) void ReindexIndex(RangeVar *indexRelation, int options, bool concurrent) { + struct ReindexIndexCallbackState state; Oid indOid; - Oid heapOid = InvalidOid; Relation irel; 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 * used here must match the index lock obtained in reindex_index(). */ + state.concurrent = concurrent; + state.locked_table_oid = InvalidOid; indOid = RangeVarGetRelidExtended(indexRelation, concurrent ? ShareUpdateExclusiveLock : AccessExclusiveLock, 0, RangeVarCallbackForReindexIndex, - (void *) &heapOid); + &state); /* * 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) { 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 @@ -2359,9 +2378,8 @@ RangeVarCallbackForReindexIndex(const RangeVar *relation, */ if (relId != oldRelId && OidIsValid(oldRelId)) { - /* lock level here should match reindex_index() heap lock */ - UnlockRelationOid(*heapOid, ShareLock); - *heapOid = InvalidOid; + UnlockRelationOid(state->locked_table_oid, table_lockmode); + state->locked_table_oid = InvalidOid; } /* 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. */ if (relId != oldRelId) { + Oid table_oid = IndexGetRelation(relId, true); + /* - * Lock level here should match reindex_index() heap lock. If the OID - * isn't valid, it means the index as concurrently dropped, which is - * not a problem for us; just return normally. + * If the OID isn't valid, it means the index as concurrently dropped, + * which is not a problem for us; just return normally. */ - *heapOid = IndexGetRelation(relId, true); - if (OidIsValid(*heapOid)) - LockRelationOid(*heapOid, ShareLock); + if (OidIsValid(table_oid)) + { + LockRelationOid(table_oid, table_lockmode); + state->locked_table_oid = table_oid; + } } } -- 2.21.0
