Re: Support for REINDEX CONCURRENTLY

On Sun, Jun 23, 2013 at 3:34 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> 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?

Compile error ;)

gcc -O0 -Wall -Wmissing-prototypes -Wpointer-arith
-Wdeclaration-after-statement -Wendif-labels
-Wmissing-format-attribute -Wformat-security -fno-strict-aliasing
-fwrapv -g -I../../../src/include   -c -o index.o index.c
index.c: In function 'index_constraint_create':
index.c:1257: error: too many arguments to function 'index_update_stats'
index.c: At top level:
index.c:1785: error: conflicting types for 'index_update_stats'
index.c:106: error: previous declaration of 'index_update_stats' was here
index.c: In function 'index_update_stats':
index.c:1881: error: 'FormData_pg_class' has no member named 'reltoastidxid'
index.c:1883: error: 'FormData_pg_class' has no member named 'reltoastidxid'
make[3]: *** [index.o] Error 1
make[2]: *** [catalog-recursive] Error 2
make[1]: *** [install-backend-recurse] Error 2
make: *** [install-src-recurse] Error 2

Regards,

-- 
Fujii Masao