Re: Support for REINDEX CONCURRENTLY

OK, please find attached a new patch for the toast part. IMHO, the
patch is now in a pretty good shape... But I cannot judge for others.

On Fri, Jun 21, 2013 at 10:47 PM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> On 2013-06-21 20:54:34 +0900, Michael Paquier wrote:
>> On Fri, Jun 21, 2013 at 6:19 PM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
>> > On 2013-06-19 09:55:24 +0900, Michael Paquier wrote:
>> >> >> @@ -1529,12 +1570,13 @@ finish_heap_swap(Oid OIDOldHeap, Oid OIDNewHeap,
>> >> > Is it actually possible to get here with multiple toast indexes?
>> >> Actually it is possible. finish_heap_swap is also called for example
>> >> in ALTER TABLE where rewriting the table (phase 3), so I think it is
>> >> better to protect this code path this way.
>> >
>> > But why would we copy invalid toast indexes over to the new relation?
>> > Shouldn't the new relation have been freshly built in the previous
>> > steps?
>> What do you think about that? Using only the first valid index would be enough?
>
> What I am thinking about is the following: When we rewrite a relation,
> we build a completely new toast relation. Which will only have one
> index, right? So I don't see how this could could be correct if we deal
> with multiple indexes. In fact, the current patch's swap_relation_files
> throws an error if there are multiple ones around.
I have reworked the code in cluster.c and made the changes more
consistent knowing that a given toast relation should only have one
valid index. This minimizes modifications where relfilenode is swapped
for toast indexes as now the swap is done only on the unique valid
indexes that a toast relation has. Also, I removed the error that was
in previouss versions triggered when a toast relation had more than
one index.

>> >> >> diff --git a/src/include/utils/relcache.h b/src/include/utils/relcache.h
>> >> >> index 8ac2549..31309ed 100644
>> >> >> --- a/src/include/utils/relcache.h
>> >> >> +++ b/src/include/utils/relcache.h
>> >> >> @@ -29,6 +29,16 @@ typedef struct RelationData *Relation;
>> >> >> typedef Relation *RelationPtr;
>> >> >>
>> >> >> /*
>> >> >> + * RelationGetIndexListIfValid
>> >> >> + * Get index list of relation without recomputing it.
>> >> >> + */
>> >> >> +#define RelationGetIndexListIfValid(rel) \
>> >> >> +do { \
>> >> >> + if (rel->rd_indexvalid == 0) \
>> >> >> + RelationGetIndexList(rel); \
>> >> >> +} while(0)
>> >> >
>> >> > Isn't this function misnamed and should be
>> >> > RelationGetIndexListIfInValid?
>> >> When naming that; I had more in mind: "get the list of indexes if it
>> >> is already there". It looks more intuitive to my mind.
>> >
>> > I can't follow. RelationGetIndexListIfValid() doesn't return
>> > anything. And it doesn't do anything if the list is already valid. It
>> > only does something iff the list currently is invalid.
>> In this case RelationGetIndexListIfInvalid?
>
> Yep. Suggested that above ;). Maybe RelationFetchIndexListIfInvalid()?
Changed the function name this way.

Also, I ran quickly the performance test that Andres sent previously
on my MBA and I couldn't notice any difference in performance.
master branch + patch:
tps = 2034.339242 (including connections establishing)
tps = 2034.406515 (excluding connections establishing)
master branch:
tps = 2083.172009 (including connections establishing)
tps = 2083.237669 (excluding connections establishing)

Thanks,
--
Michael