Re: Relation locking and relcache load (was Re: Going for "all green" buildfarm results)

I wrote:
>> ... This means the only thing stopping us from
>> taking lock before we invoke relcache is lack of knowledge about the
>> rel's relisshared status.

While digging through all the places that open relcache entries,
I've realized that there's another problem, specifically the way that
we lock indexes.  The current theory is that index_open() takes no lock,
and then we establish a lock just for the duration of an index scan.
The comments for index_open explain:
*    Note: we acquire no lock on the index. A lock is not needed when*    simply examining the index reldesc; the
index'sschema information*    is considered to be protected by the lock that the caller had better*    be holding on
theparent relation. Some type of lock should be*    obtained on the index before physically accessing it, however.*
Thisis handled automatically for most uses by index_beginscan*    and index_endscan for scan cases, or by
ExecOpenIndicesand*    ExecCloseIndices for update cases. Other callers will need to*    obtain their own locks.
 

However, the lionfish failure makes the folly of this approach evident
(it was in fact index_open that failed in that example, unless my theory
about it is all wrong).  If we're going to move to lock-before-open then
we've got to fix index_open too.  I envision making index_open just
about like heap_open, ie add a lockmode parameter, and then get rid of
the separate lock step in index_beginscan.

There is one small problem with doing that, which is this code in
ExecOpenIndices:
       indexDesc = index_open(indexOid);
       if (indexDesc->rd_am->amconcurrent)           LockRelation(indexDesc, RowExclusiveLock);       else
LockRelation(indexDesc,AccessExclusiveLock);
 

IOW you need to already have the index open to find out what sort of
lock to take on it.

I have a modest proposal for fixing that: let's get rid of amconcurrent,
so that RowExclusiveLock is always the right lock to take here.  All of
the currently supported AMs have amconcurrent = true, and so does the
proposed bitmap index patch, and given the project's current focus on
high concurrent performance I cannot imagine that we'd accept a future
patch to add a nonconcurrent index type.  So let's just legislate that
all AMs have to support concurrent updates (or at least that they can't
rely on the main system to protect them from the case).

Any objections?
        regards, tom lane