On Mon, Jun 24, 2013 at 7:39 PM, Andres Freund <and...@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.fu...@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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
