Re: Support for REINDEX CONCURRENTLY

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:
>> On Fri, Jun 21, 2013 at 10:47 PM, Andres Freund <andres@2ndquadrant.com> wrote:
>> > Hm. Looking at how this is currently used - I am afraid it's not
>> > correct... the reason RelationGetIndexList() returns a copy is that
>> > cache invalidations will throw away that list. And you do index_open()
>> > while iterating over it which will accept invalidation messages.
>> > Mybe it's better to try using RelationGetIndexList directly and measure
>> > whether that has a measurable impact=
>> 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.
--
Michael