Re: Support for REINDEX CONCURRENTLY

OK let's finalize this patch first. I'll try to send an updated patch
within today.

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.
Yes, OK. Let me have a look at the code of CLUSTER more in details
before giving a precise answer, but I'll try to remove that renaming
part. Btw, I'd like to add an assertion in the code at least to
prevent wrong use of this code path.

>> >> >> 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()?
>
> 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=
Yes, I was wondering about potential memory leak that list_copy could
introduce in tuptoaster.c when doing a bulk insert, that's the only
reason why I added this macro.
--
Michael