Avoid returning stale attribute bitmaps in RelationGetIndexAttrBitmap().

The problem with the original coding here is that we might receive (and
clear) a relcache invalidation signal for the target relation down inside
one of the index_open calls we're doing.  Since the target is open, we
would not drop the relcache entry, just reset its rd_indexvalid and
rd_indexlist fields.  But RelationGetIndexAttrBitmap() kept going, and
would eventually cache and return potentially-obsolete attribute bitmaps.

The case where this matters is where the inval signal was from a CREATE
INDEX CONCURRENTLY telling us about a new index on a formerly-unindexed
column.  (In all other cases, the lock we hold on the target rel should
prevent any concurrent change in index state.)  Even just returning the
stale attribute bitmap is not such a problem, because it shouldn't matter
during the transaction in which we receive the signal.  What hurts is
caching the stale data, because it can survive into later transactions,
breaking CREATE INDEX CONCURRENTLY's expectation that later transactions
will not create new broken HOT chains.  The upshot is that there's a window
for building corrupted indexes during CREATE INDEX CONCURRENTLY.

This patch fixes the problem by rechecking that the set of index OIDs
is still the same at the end of RelationGetIndexAttrBitmap() as it was
at the start.  If not, we loop back and try again.  That's a little
more than is strictly necessary to fix the bug --- in principle, we
could return the stale data but not cache it --- but it seems like a
bad idea on general principles for relcache to return data it knows
is stale.

There might be more hazards of the same ilk, or there might be a better
way to fix this one, but this patch definitely improves matters and seems
unlikely to make anything worse.  So let's push it into today's releases
even as we continue to study the problem.

Pavan Deolasee and myself

Discussion: https://postgr.es/m/CABOikdM2MUq9cyZJi1KyLmmkCereyGp5JQ4fuwKoyKEde_mzkQ@mail.gmail.com

diff --git a/src/backend/utils/cache/relcache.c b/src/backend/utils/cache/relcache.c
index 8c6a34afb489a15b239221ab1d92b3f537be55b3..8c8ef66c8f42f90b5d29d4053e8840e552466e5c 100644 (file)
--- a/src/backend/utils/cache/relcache.c
+++ b/src/backend/utils/cache/relcache.c
@@ -4363,8 +4363,10 @@ RelationGetIndexPredicate(Relation relation)
  * we can include system attributes (e.g., OID) in the bitmap representation.
  *
  * Caller had better hold at least RowExclusiveLock on the target relation
- * to ensure that it has a stable set of indexes.  This also makes it safe
- * (deadlock-free) for us to take locks on the relation's indexes.
+ * to ensure it is safe (deadlock-free) for us to take locks on the relation's
+ * indexes.  Note that since the introduction of CREATE INDEX CONCURRENTLY,
+ * that lock level doesn't guarantee a stable set of indexes, so we have to
+ * be prepared to retry here in case of a change in the set of indexes.
  *
  * The returned result is palloc'd in the caller's memory context and should
  * be bms_free'd when not needed anymore.
@@ -4376,6 +4378,7 @@ RelationGetIndexAttrBitmap(Relation relation, IndexAttrBitmapKind attrKind)
        Bitmapset  *uindexattrs;        /* columns in unique indexes */
        Bitmapset  *idindexattrs;       /* columns in the replica identity */
        List       *indexoidlist;
+       List       *newindexoidlist;
        Oid                     relreplindex;
        ListCell   *l;
        MemoryContext oldcxt;
@@ -4401,8 +4404,9 @@ RelationGetIndexAttrBitmap(Relation relation, IndexAttrBitmapKind attrKind)
                return NULL;
 
        /*
-        * Get cached list of index OIDs
+        * Get cached list of index OIDs. If we have to start over, we do so here.
         */
+restart:
        indexoidlist = RelationGetIndexList(relation);
 
        /* Fall out if no indexes (but relhasindex was set) */
@@ -4413,8 +4417,7 @@ RelationGetIndexAttrBitmap(Relation relation, IndexAttrBitmapKind attrKind)
         * Copy the rd_replidindex value computed by RelationGetIndexList before
         * proceeding.  This is needed because a relcache flush could occur inside
         * index_open below, resetting the fields managed by RelationGetIndexList.
-        * (The values we're computing will still be valid, assuming that caller
-        * has a sufficient lock on the relation.)
+        * We need to do the work with a stable value for relreplindex.
         */
        relreplindex = relation->rd_replidindex;
 
@@ -4482,7 +4485,31 @@ RelationGetIndexAttrBitmap(Relation relation, IndexAttrBitmapKind attrKind)
                index_close(indexDesc, AccessShareLock);
        }
 
-       list_free(indexoidlist);
+       /*
+        * During one of the index_opens in the above loop, we might have received
+        * a relcache flush event on this relcache entry, which might have been
+        * signaling a change in the rel's index list.  If so, we'd better start
+        * over to ensure we deliver up-to-date attribute bitmaps.
+        */
+       newindexoidlist = RelationGetIndexList(relation);
+       if (equal(indexoidlist, newindexoidlist) &&
+               relreplindex == relation->rd_replidindex)
+       {
+               /* Still the same index set, so proceed */
+               list_free(newindexoidlist);
+               list_free(indexoidlist);
+       }
+       else
+       {
+               /* Gotta do it over ... might as well not leak memory */
+               list_free(newindexoidlist);
+               list_free(indexoidlist);
+               bms_free(uindexattrs);
+               bms_free(idindexattrs);
+               bms_free(indexattrs);
+
+               goto restart;
+       }
 
        /* Don't leak the old values of these bitmaps, if any */
        bms_free(relation->rd_indexattr);