Re: Support for REINDEX CONCURRENTLY

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.

> >> >> 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=

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services