Re: Support for REINDEX CONCURRENTLY

On Mon, Jun 24, 2013 at 7:39 PM, Andres Freund <andres@2ndquadrant.com> wrote:
> 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?

Yep, will check.

Regards,

-- 
Fujii Masao