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 <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

diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 663407c65d33f1e66374c4978f3d0beb8308564a..1db10d2360623507c4c1d7fcc12b77d894eb41d0 100644 (file)
--- 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 was 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;
+               }
        }
 }