Re: Support for REINDEX CONCURRENTLY

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 think we should ignore the invalid indexes in that SELECT?
Yes indeed, it doesn't make sense to grab invalid toast indexes.
Changed this way.

>> @@ -1392,19 +1390,62 @@ swap_relation_files(Oid r1, Oid r2, bool target_is_pg_class,
>> }
>>
>> /*
>> - * If we're swapping two toast tables by content, do the same for their
>> - * indexes.
>> + * If we're swapping two toast tables by content, do the same for all of
>> + * their indexes. The swap can actually be safely done only if the
>> + * relations have indexes.
>> */
>> if (swap_toast_by_content &&
>> - relform1->reltoastidxid && relform2->reltoastidxid)
>> - swap_relation_files(relform1->reltoastidxid,
>> - relform2->reltoastidxid,
>> - target_is_pg_class,
>> - swap_toast_by_content,
>> - is_internal,
>> - InvalidTransactionId,
>> - InvalidMultiXactId,
>> - mapped_tables);
>> + relform1->reltoastrelid &&
>> + relform2->reltoastrelid)
>> + {
>> + Relation toastRel1, toastRel2;
>> +
>> + /* Open relations */
>> + toastRel1 = heap_open(relform1->reltoastrelid, AccessExclusiveLock);
>> + toastRel2 = heap_open(relform2->reltoastrelid, AccessExclusiveLock);
>> +
>> + /* Obtain index list */
>> + RelationGetIndexList(toastRel1);
>> + RelationGetIndexList(toastRel2);
>> +
>> + /* Check if the swap is possible for all the toast indexes */
>> + if (list_length(toastRel1->rd_indexlist) == 1 &&
>> + list_length(toastRel2->rd_indexlist) == 1)
>> + {
>> + ListCell *lc1, *lc2;
>> +
>> + /* Now swap each couple */
>> + lc2 = list_head(toastRel2->rd_indexlist);
>> + foreach(lc1, toastRel1->rd_indexlist)
>> + {
>> + Oid indexOid1 = lfirst_oid(lc1);
>> + Oid indexOid2 = lfirst_oid(lc2);
>> + swap_relation_files(indexOid1,
>> + indexOid2,
>> + target_is_pg_class,
>> + swap_toast_by_content,
>> + is_internal,
>> + InvalidTransactionId,
>> + InvalidMultiXactId,
>> + mapped_tables);
>> + lc2 = lnext(lc2);
>> + }
>
> Why are you iterating over the indexlists after checking they are both
> of length == 1? Looks like the code would be noticeably shorter without
> that.
OK. Modified this way.

>> + }
>> + else
>> + {
>> + /*
>> + * As this code path is only taken by shared catalogs, who cannot
>> + * have multiple indexes on their toast relation, simply return
>> + * an error.
>> + */
>> + ereport(ERROR,
>> + (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
>> + errmsg("cannot swap relation files of a shared catalog with multiple indexes on toast relation")));
>> + }
>> +
>
> Absolutely minor thing, using an elog() seems to be better here since
> that uses the appropriate error code for some codepath that's not
> expected to be executed.
OK. Modified this way.

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

>> diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
>> index ec956ad..ac42389 100644
>> --- a/src/bin/pg_dump/pg_dump.c
>> +++ b/src/bin/pg_dump/pg_dump.c
>> @@ -2781,16 +2781,16 @@ binary_upgrade_set_pg_class_oids(Archive *fout,
>> Oid pg_class_reltoastidxid;
>>
>> appendPQExpBuffer(upgrade_query,
>> - "SELECT c.reltoastrelid, t.reltoastidxid "
>> + "SELECT c.reltoastrelid, t.indexrelid "
>> "FROM pg_catalog.pg_class c LEFT JOIN "
>> - "pg_catalog.pg_class t ON (c.reltoastrelid = t.oid) "
>> - "WHERE c.oid = '%u'::pg_catalog.oid;",
>> + "pg_catalog.pg_index t ON (c.reltoastrelid = t.indrelid) "
>> + "WHERE c.oid = '%u'::pg_catalog.oid AND t.indisvalid;",
>> pg_class_oid);
>
> This possibly needs a version qualification due to querying
> indisvalid. How far back do we support pg_upgrade?
By having a look at the docs, pg_upgrade has been added in 9.0 and
support upgrades for version >= 8.3.X. indisvalid has been added in
8.2 so we are fine.

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