Fix lookup code for REINDEX INDEX.

This commit adjusts RangeVarCallbackForReindexIndex() to handle an
extremely unlikely race condition involving concurrent OID reuse.
In short, if REINDEX INDEX is executed at the same time that the
index is re-created with the same name and OID but a different
parent table OID, we might lock the wrong parent table.  To fix,
simply detect when this happens and emit an ERROR.  Unfortunately,
we can't gracefully handle this situation because we will have
already locked the index, and we must lock the parent table before
the index to avoid deadlocks.

While at it, I've replaced all but one early return in this
callback function with ERRORs that should be unreachable.  While I
haven't verified the presence of a live bug, the checks in question
appear to be unnecessary, and the early returns seem prone to
breaking the parent table locking code in subtle ways.  If nothing
else, this simplifies the code a bit.

This is a bug fix and could be back-patched, but given the presumed
rarity of the race condition and the lack of reports, I'm not going
to bother.

Reviewed-by: Tom Lane <[email protected]>
Reviewed-by: Jeff Davis <[email protected]>
Discussion: https://postgr.es/m/Z8zwVmGzXyDdkAXj%40nathan

diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index ca2bde62e82ff589690d7d7d08982da9abf32fd1..5712fac36971ebe687fbb637d3ed3576c32e77b1 100644 (file)
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -2976,6 +2976,7 @@ RangeVarCallbackForReindexIndex(const RangeVar *relation,
    struct ReindexIndexCallbackState *state = arg;
    LOCKMODE    table_lockmode;
    Oid         table_oid;
+   AclResult   aclresult;
 
    /*
     * Lock level here should match table lock in reindex_index() for
@@ -3000,43 +3001,42 @@ RangeVarCallbackForReindexIndex(const RangeVar *relation,
    if (!OidIsValid(relId))
        return;
 
-   /*
-    * If the relation does exist, check whether it's an index.  But note that
-    * the relation might have been dropped between the time we did the name
-    * lookup and now.  In that case, there's nothing to do.
-    */
+   /* If the relation does exist, check whether it's an index. */
    relkind = get_rel_relkind(relId);
-   if (!relkind)
-       return;
    if (relkind != RELKIND_INDEX &&
        relkind != RELKIND_PARTITIONED_INDEX)
        ereport(ERROR,
                (errcode(ERRCODE_WRONG_OBJECT_TYPE),
                 errmsg("\"%s\" is not an index", relation->relname)));
 
-   /* Check permissions */
-   table_oid = IndexGetRelation(relId, true);
-   if (OidIsValid(table_oid))
-   {
-       AclResult   aclresult;
+   /* Look up the index's table. */
+   table_oid = IndexGetRelation(relId, false);
 
-       aclresult = pg_class_aclcheck(table_oid, GetUserId(), ACL_MAINTAIN);
-       if (aclresult != ACLCHECK_OK)
-           aclcheck_error(aclresult, OBJECT_INDEX, relation->relname);
-   }
+   /*
+    * In the unlikely event that, upon retry, we get the same index OID with
+    * a different table OID, fail.  RangeVarGetRelidExtended() will have
+    * already locked the index in this case, and it won't retry again, so we
+    * can't lock the newly discovered table OID without risking deadlock.
+    * Also, while this corner case is indeed possible, it is extremely
+    * unlikely to happen in practice, so it's probably not worth any more
+    * effort than this.
+    */
+   if (relId == oldRelId && table_oid != state->locked_table_oid)
+       ereport(ERROR,
+               (errcode(ERRCODE_UNDEFINED_OBJECT),
+                errmsg("index \"%s\" was concurrently dropped",
+                       relation->relname)));
+
+   /* Check permissions. */
+   aclresult = pg_class_aclcheck(table_oid, GetUserId(), ACL_MAINTAIN);
+   if (aclresult != ACLCHECK_OK)
+       aclcheck_error(aclresult, OBJECT_INDEX, relation->relname);
 
    /* Lock heap before index to avoid deadlock. */
    if (relId != oldRelId)
    {
-       /*
-        * If the OID isn't valid, it means the index was concurrently
-        * dropped, which is not a problem for us; just return normally.
-        */
-       if (OidIsValid(table_oid))
-       {
-           LockRelationOid(table_oid, table_lockmode);
-           state->locked_table_oid = table_oid;
-       }
+       LockRelationOid(table_oid, table_lockmode);
+       state->locked_table_oid = table_oid;
    }
 }