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