Re: Support for REINDEX CONCURRENTLY

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:
>> >> /* 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