Re: Index corruption with CREATE INDEX CONCURRENTLY

On 2017-02-05 15:14:59 -0500, Tom Lane wrote:
> I do not like any of the other patches proposed in this thread, because
> they fail to guarantee delivering an up-to-date attribute bitmap to the
> caller. I think we need a retry loop, and I think that it needs to look
> like the attached.

That looks like a reasonable approach, although I'm not sure it's the
best one.

> (Note that the tests whether rd_pkindex and rd_replidindex changed might
> be redundant; but I'm not totally sure of that, and they're cheap.)

I don't think they can, but I'm all for re-checking.

> 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.

BTW, the interactions of RelationSetIndexList with
RelationGetIndexAttrBitmap() look more than a bit fragile to me, even if
documented:
*
* We deliberately do not change rd_indexattr here: even when operating
* with a temporary partial index list, HOT-update decisions must be made
* correctly with respect to the full index set. It is up to the caller
* to ensure that a correct rd_indexattr set has been cached before first
* calling RelationSetIndexList; else a subsequent inquiry might cause a
* wrong rd_indexattr set to get computed and cached. Likewise, we do not
* touch rd_keyattr, rd_pkattr or rd_idattr.

Greetings,

Andres Freund