Re: Support for REINDEX CONCURRENTLY

On 2013-06-22 22:45:26 +0900, Michael Paquier wrote:
> On Sat, Jun 22, 2013 at 10:34 PM, Andres Freund <andres@2ndquadrant.com> wrote:
> > On 2013-06-22 12:50:52 +0900, Michael Paquier wrote:
> >> By looking at the comments of RelationGetIndexList:relcache.c,
> >> actually the method of the patch is correct because in the event of a
> >> shared cache invalidation, rd_indexvalid is set to 0 when the index
> >> list is reset, so the index list would get recomputed even in the case
> >> of shared mem invalidation.
> >
> > The problem I see is something else. Consider code like the following:
> >
> > RelationFetchIndexListIfInvalid(toastrel);
> > foreach(lc, toastrel->rd_indexlist)
> >    toastidxs[i++] = index_open(lfirst_oid(lc), RowExclusiveLock);
> >
> > index_open calls relation_open calls LockRelationOid which does:
> > if (res != LOCKACQUIRE_ALREADY_HELD)
> >    AcceptInvalidationMessages();
> >
> > So, what might happen is that you open the first index, which accepts an
> > invalidation message which in turn might delete the indexlist. Which
> > means we would likely read invalid memory if there are two indexes.
> And I imagine that you have the same problem even with
> RelationGetIndexList, not only RelationGetIndexListIfInvalid, because
> this would appear as long as you try to open more than 1 index with an
> index list.

No. RelationGetIndexList() returns a copy of the list for exactly that
reason. The danger is not to see an outdated list - we should be
protected by locks against that - but looking at uninitialized or reused
memory.

Greetings,

Andres Freund

-- 
 Andres Freund	                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services