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(dot)fujii(at)gmail(dot)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