Re: Support for REINDEX CONCURRENTLY

On 2013-06-24 07:46:34 +0900, Michael Paquier wrote:
> On Mon, Jun 24, 2013 at 7:22 AM, Fujii Masao <masao.fujii@gmail.com> wrote:
> > Compile error ;)
> It looks like filterdiff did not work correctly when generating the
> latest patch with context diffs, I cannot apply it cleanly wither.
> This is perhaps due to a wrong manipulation from me. Please try the
> attached that has been generated as a raw git output. It works
> correctly with a git apply. I just checked.

Did you check whether that introduces a performance regression?


>  /* ----------
> + * toast_get_valid_index
> + *
> + *	Get the valid index of given toast relation. A toast relation can only
> + *	have one valid index at the same time. The lock taken on the index
> + *	relations is released at the end of this function call.
> + */
> +Oid
> +toast_get_valid_index(Oid toastoid, LOCKMODE lock)
> +{
> +	ListCell   *lc;
> +	List	   *indexlist;
> +	int			num_indexes, i = 0;
> +	Oid			validIndexOid;
> +	Relation	validIndexRel;
> +	Relation   *toastidxs;
> +	Relation	toastrel;
> +
> +	/* Get the index list of relation */
> +	toastrel = heap_open(toastoid, lock);
> +	indexlist = RelationGetIndexList(toastrel);
> +	num_indexes = list_length(indexlist);
> +
> +	/* Open all the index relations */
> +	toastidxs = (Relation *) palloc(num_indexes * sizeof(Relation));
> +	foreach(lc, indexlist)
> +		toastidxs[i++] = index_open(lfirst_oid(lc), lock);
> +
> +	/* Fetch valid toast index */
> +	validIndexRel = toast_index_fetch_valid(toastidxs, num_indexes);
> +	validIndexOid = RelationGetRelid(validIndexRel);
> +
> +	/* Close all the index relations */
> +	for (i = 0; i < num_indexes; i++)
> +		index_close(toastidxs[i], lock);
> +	pfree(toastidxs);
> +	list_free(indexlist);
> +
> +	heap_close(toastrel, lock);
> +	return validIndexOid;
> +}

Just to make sure, could you check we've found a valid index?

>  static bool
> -toastrel_valueid_exists(Relation toastrel, Oid valueid)
> +toastrel_valueid_exists(Relation toastrel, Oid valueid, LOCKMODE lockmode)
>  {
>  	bool		result = false;
>  	ScanKeyData toastkey;
>  	SysScanDesc toastscan;
> +	int			i = 0;
> +	int			num_indexes;
> +	Relation   *toastidxs;
> +	Relation	validtoastidx;
> +	ListCell   *lc;
> +	List	   *indexlist;
> +
> +	/* Ensure that the list of indexes of toast relation is computed */
> +	indexlist = RelationGetIndexList(toastrel);
> +	num_indexes = list_length(indexlist);
> +
> +	/* Open each index relation necessary */
> +	toastidxs = (Relation *) palloc(num_indexes * sizeof(Relation));
> +	foreach(lc, indexlist)
> +		toastidxs[i++] = index_open(lfirst_oid(lc), lockmode);
> +
> +	/* Fetch a valid index relation */
> +	validtoastidx = toast_index_fetch_valid(toastidxs, num_indexes);

Those 10 lines are repeated multiple times, in different
functions. Maybe move them into toast_index_fetch_valid and rename that
to
Relation *
toast_open_indexes(Relation toastrel, LOCKMODE mode, size_t *numindexes, size_t valididx);

That way we also wouldn't fetch/copy the indexlist twice in some
functions.

> +	/* Clean up */
> +	for (i = 0; i < num_indexes; i++)
> +		index_close(toastidxs[i], lockmode);
> +	list_free(indexlist);
> +	pfree(toastidxs);

The indexlist could already be freed inside the function proposed
above...


> diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
> index 8294b29..2b777da 100644
> --- a/src/backend/commands/tablecmds.c
> +++ b/src/backend/commands/tablecmds.c
> @@ -8782,7 +8783,13 @@ ATExecSetTableSpace(Oid tableOid, Oid newTableSpace, LOCKMODE lockmode)
>  				 errmsg("cannot move temporary tables of other sessions")));
>  

> +	foreach(lc, reltoastidxids)
> +	{
> +		Oid toastidxid = lfirst_oid(lc);
> +		if (OidIsValid(toastidxid))
> +			ATExecSetTableSpace(toastidxid, newTableSpace, lockmode);
> +	}

Copy & pasted OidIsValid(), shouldn't be necessary anymore.


Otherwise I think there's not really much left to be done. Fujii?

Greetings,

Andres Freund

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