Re: Support for REINDEX CONCURRENTLY

On Fri, Jun 21, 2013 at 6:19 PM, Andres Freund <andres@2ndquadrant.com> wrote:
> On 2013-06-19 09:55:24 +0900, Michael Paquier wrote:
>> >>       /* Clean up. */
>> >>       heap_freetuple(reltup1);
>> >> @@ -1529,12 +1570,13 @@ finish_heap_swap(Oid OIDOldHeap, Oid OIDNewHeap,
>> >>               if (OidIsValid(newrel->rd_rel->reltoastrelid))
>> >>               {
>> >>                       Relation        toastrel;
>> >> -                     Oid                     toastidx;
>> >>                       char            NewToastName[NAMEDATALEN];
>> >> +                     ListCell   *lc;
>> >> +                     int                     count = 0;
>> >>
>> >>                       toastrel = relation_open(newrel->rd_rel->reltoastrelid,
>> >>                                                                        AccessShareLock);
>> >> -                     toastidx = toastrel->rd_rel->reltoastidxid;
>> >> +                     RelationGetIndexList(toastrel);
>> >>                       relation_close(toastrel, AccessShareLock);
>> >>
>> >>                       /* rename the toast table ... */
>> >> @@ -1543,11 +1585,23 @@ finish_heap_swap(Oid OIDOldHeap, Oid OIDNewHeap,
>> >>                       RenameRelationInternal(newrel->rd_rel->reltoastrelid,
>> >>                                                                  NewToastName, true);
>> >>
>> >> -                     /* ... and its index too */
>> >> -                     snprintf(NewToastName, NAMEDATALEN, "pg_toast_%u_index",
>> >> -                                      OIDOldHeap);
>> >> -                     RenameRelationInternal(toastidx,
>> >> -                                                                NewToastName, true);
>> >> +                     /* ... and its indexes too */
>> >> +                     foreach(lc, toastrel->rd_indexlist)
>> >> +                     {
>> >> +                             /*
>> >> +                              * The first index keeps the former toast name and the
>> >> +                              * following entries have a suffix appended.
>> >> +                              */
>> >> +                             if (count == 0)
>> >> +                                     snprintf(NewToastName, NAMEDATALEN, "pg_toast_%u_index",
>> >> +                                                      OIDOldHeap);
>> >> +                             else
>> >> +                                     snprintf(NewToastName, NAMEDATALEN, "pg_toast_%u_index_%d",
>> >> +                                                      OIDOldHeap, count);
>> >> +                             RenameRelationInternal(lfirst_oid(lc),
>> >> +                                                                        NewToastName, true);
>> >> +                             count++;
>> >> +                     }
>> >>               }
>> >>               relation_close(newrel, NoLock);
>> >>       }
>> >
>> > 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?

>
>> >> 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?
--
Michael