Re: Support for REINDEX CONCURRENTLY

Patch updated according to comments.

On Mon, Jun 24, 2013 at 7:39 PM, Andres Freund <andres(at)2ndquadrant(dot)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(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?
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