Hi, On 2017-02-05 16:37:33 -0800, Andres Freund wrote: > > RelationGetIndexList(Relation relation) > > @@ -4746,8 +4747,10 @@ RelationGetIndexPredicate(Relation relat > > * 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. > > I've not yet read the full thread, but I'm a bit confused so far. We > obviously can get changing information about indexes here, but isn't > that something we have to deal with anyway? If we guarantee that we > don't loose knowledge that there's a pending invalidation, why do we > have to retry? Pretty much by definition the knowledge in a relcache > entry can be outdated as soon as returned unless locking prevents that > from being possible - which is not the case here. > > ISTM it'd be better not to have retry logic here, but to follow the more > general pattern of making sure that we know whether the information > needs to be recomputed at the next access. We could either do that by > having an additional bit of information about the validity of the > bitmaps (so we have invalid, building, valid - and only set valid at the > end of computing the bitmaps when still building and not invalid again), > or we simply move the bitmap computation into the normal relcache build.
To show what I mean here's an *unpolished* and *barely tested* patch implementing the first of my suggestions. Alvaro, Pavan, I think should address the issue as well? - Andres
diff --git a/src/backend/utils/cache/relcache.c b/src/backend/utils/cache/relcache.c index 8a7c560e46..9e94495e75 100644 --- a/src/backend/utils/cache/relcache.c +++ b/src/backend/utils/cache/relcache.c @@ -4745,9 +4745,12 @@ RelationGetIndexPredicate(Relation relation) * Attribute numbers are offset by FirstLowInvalidHeapAttributeNumber so that * 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. + * 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. Note that + * a concurrent CREATE/DROP INDEX CONCURRENTLY can lead to an outdated list + * being returned (will be recomputed at the next access), the CONCURRENTLY + * code deals with that. * * The returned result is palloc'd in the caller's memory context and should * be bms_free'd when not needed anymore. @@ -4766,7 +4769,7 @@ RelationGetIndexAttrBitmap(Relation relation, IndexAttrBitmapKind attrKind) MemoryContext oldcxt; /* Quick exit if we already computed the result. */ - if (relation->rd_indexattr != NULL) + if (relation->rd_bitmapsvalid == 2) { switch (attrKind) { @@ -4788,6 +4791,14 @@ RelationGetIndexAttrBitmap(Relation relation, IndexAttrBitmapKind attrKind) return NULL; /* + * Signal that the attribute bitmaps are being built. If there's any + * relcache invalidations while building them, rd_bitmapsvalid will be + * reset to 0. In that case return the bitmaps, but don't mark them as + * valid. + */ + relation->rd_bitmapsvalid = 1; + + /* * Get cached list of index OIDs */ indexoidlist = RelationGetIndexList(relation); @@ -4892,13 +4903,7 @@ RelationGetIndexAttrBitmap(Relation relation, IndexAttrBitmapKind attrKind) bms_free(relation->rd_idattr); relation->rd_idattr = NULL; - /* - * Now save copies of the bitmaps in the relcache entry. We intentionally - * set rd_indexattr last, because that's the one that signals validity of - * the values; if we run out of memory before making that copy, we won't - * leave the relcache entry looking like the other ones are valid but - * empty. - */ + /* now save copies of the bitmaps in the relcache entry */ oldcxt = MemoryContextSwitchTo(CacheMemoryContext); relation->rd_keyattr = bms_copy(uindexattrs); relation->rd_pkattr = bms_copy(pkindexattrs); @@ -4906,6 +4911,18 @@ RelationGetIndexAttrBitmap(Relation relation, IndexAttrBitmapKind attrKind) relation->rd_indexattr = bms_copy(indexattrs); MemoryContextSwitchTo(oldcxt); + /* + * If there've been no invalidations while building the entry, mark the + * stored bitmaps as being valid. Need to do so after the copies above, + * as we could run out of memory while doing so. + * + * NB: No relcache accesses should happen inside this routine after this. + */ + if (relation->rd_bitmapsvalid == 1) + { + relation->rd_bitmapsvalid = 2; + } + /* We return our original working copy for caller to play with */ switch (attrKind) { diff --git a/src/include/utils/rel.h b/src/include/utils/rel.h index a617a7cf56..4fbf6632a0 100644 --- a/src/include/utils/rel.h +++ b/src/include/utils/rel.h @@ -137,6 +137,7 @@ typedef struct RelationData Oid rd_replidindex; /* OID of replica identity index, if any */ /* data managed by RelationGetIndexAttrBitmap: */ + int rd_bitmapsvalid; /* 0 invalid, 1 building, 2 valid */ Bitmapset *rd_indexattr; /* identifies columns used in indexes */ Bitmapset *rd_keyattr; /* cols that can be ref'd by foreign keys */ Bitmapset *rd_pkattr; /* cols included in primary key */
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers