On Sun, Jun 23, 2013 at 3:34 PM, Michael Paquier <michael.paqu...@gmail.com> wrote: > OK. Please find an updated patch for the toast part. > > On Sat, Jun 22, 2013 at 10:48 PM, Andres Freund <and...@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 <and...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers