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 ([email protected])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
