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;
+       }
    }
 }