Re: Support for REINDEX CONCURRENTLY

On 2013-06-19 09:55:24 +0900, Michael Paquier wrote:
> Please find an updated patch. The regression test rules has been
> updated, and all the comments are addressed.
>
> On Tue, Jun 18, 2013 at 6:35 PM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> > Hi,
> >
> > On 2013-06-18 10:53:25 +0900, Michael Paquier wrote:
> >> diff --git a/contrib/pg_upgrade/info.c b/contrib/pg_upgrade/info.c
> >> index c381f11..3a6342c 100644
> >> --- a/contrib/pg_upgrade/info.c
> >> +++ b/contrib/pg_upgrade/info.c
> >> @@ -321,12 +321,17 @@ get_rel_infos(ClusterInfo *cluster, DbInfo *dbinfo)
> >> "INSERT INTO info_rels "
> >> "SELECT reltoastrelid "
> >> "FROM info_rels i JOIN pg_catalog.pg_class c "
> >> - " ON i.reloid = c.oid"));
> >> + " ON i.reloid = c.oid "
> >> + " AND c.reltoastrelid != %u", InvalidOid));
> >> PQclear(executeQueryOrDie(conn,
> >> "INSERT INTO info_rels "
> >> - "SELECT reltoastidxid "
> >> - "FROM info_rels i JOIN pg_catalog.pg_class c "
> >> - " ON i.reloid = c.oid"));
> >> + "SELECT indexrelid "
> >> + "FROM pg_index "
> >> + "WHERE indrelid IN (SELECT reltoastrelid "
> >> + " FROM pg_class "
> >> + " WHERE oid >= %u "
> >> + " AND reltoastrelid != %u)",
> >> + FirstNormalObjectId, InvalidOid));
> >
> > What's the idea behind the >= here?
> It is here to avoid fetching the toast relations of system tables. But
> I see your point, the inner query fetching the toast OIDs should do a
> join on the exising info_rels and not try to do a join on a plain
> pg_index, so changed this way.

I'd also rather not introduce knowledge about FirstNormalObjectId into
client applications... But you fixed it already.

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

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

Greetings,

Andres Freund

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