Re: Support for REINDEX CONCURRENTLY

Patch updated according to comments.

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?
I don't notice any difference. Here are some results on one of my
boxes with a single client using your previous test case.
master:
tps = 1753.374740 (including connections establishing)
tps = 1753.505288 (excluding connections establishing)
master + patch:
tps = 1738.354976 (including connections establishing)
tps = 1738.482424 (excluding connections establishing)

>>  /* ----------
>> + * 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?
Added an elog(ERROR) if valid index is not found.

>
>>  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.
Good suggestion, this makes the code cleaner. However I didn't use
exactly what you suggested:
static int toast_open_indexes(Relation toastrel,
                              LOCKMODE lock,
                              Relation **toastidxs,
                              int *num_indexes);
static void toast_close_indexes(Relation *toastidxs, int num_indexes,
                                LOCKMODE lock);

toast_open_indexes returns the position of valid index in the array of
toast indexes. This looked clearer to me when coding.

>
>> +     /* 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...
Done.

>> 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.
Yep, indeed. If there are no indexes list would be simply empty.

Thanks for your patience.
--
Michael