Re: Support for REINDEX CONCURRENTLY

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?

I think we should ignore the invalid indexes in that SELECT?


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

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

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


> 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
indisalid. How far back do we support pg_upgrade?

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

Going to do some performance tests now.

Greetings,

Andres Freund

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