Re: Support for REINDEX CONCURRENTLY

OK. Please find an updated patch for the toast part.

On Sat, Jun 22, 2013 at 10:48 PM, Andres Freund <andres@2ndquadrant.com> wrote:
> 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.
OK, so I removed RelationGetIndexListIfInvalid (such things could be
an optimization for another patch) and replaced it by calls to
RelationGetIndexList to get a copy of rd_indexlist in a local list
variable, list free'd when it is not necessary anymore.

It looks that there is nothing left for this patch, no?
--
Michael