Re: patch: fix performance problems with repated decomprimation of varlena values in plpgsql

Hello

2011/1/15 Noah Misch <noah(at)leadboat(dot)com>:
> Hello Pavel,
>
> I'm reviewing this patch for CommitFest 2011-01.
>

Thank you very much,

I am sending a updated version with little bit more comments. But I am
sure, so somebody with good English have to edit my comments.
Minimally I hope, so your questions will be answered.

> The patch seems fully desirable.  It appropriately contains no documentation
> updates.  It contains no new tests, and that's probably fine, too; I can't think
> of any corner cases where this would do something other than work correctly or
> break things comprehensively.
>
> Using your test case from here:
> http://archives.postgresql.org/message-id/AANLkTikDCW+c-C4U4NgaOBhpFSZkb5Uy_ZuaTDZfPMSn@mail.gmail.com
> I observed a 28x speedup, similar to Álvaro's report.  I also made my own test
> case, attached, to evaluate this from a somewhat different angle and also to
> consider the worst case.  With a 100 KiB string (good case), I see a 4.8x
> speedup.  With a 1 KiB string (worst case - never toasted), I see no
> statistically significant change.  This is to be expected.
>
> A few specific questions and comments follow, all minor.  Go ahead and mark the
> patch ready for committer when you've acted on them, or declined to do so, to
> your satisfaction.  I don't think a re-review will be needed.
>
> Thanks,
> nm
>
> On Mon, Nov 22, 2010 at 01:46:51PM +0100, Pavel Stehule wrote:
>> *** ./pl_exec.c.orig  2010-11-16 10:28:42.000000000 +0100
>> --- ./pl_exec.c       2010-11-22 13:33:01.597726809 +0100
>
> The patch applies cleanly, but the format is slightly nonstandard (-p0 when
> applied from src/pl/plpgsql/src, rather than -p1 from the root).
>
>> ***************
>> *** 3944,3949 ****
>> --- 3965,3993 ----
>>
>>                               *typeid = var->datatype->typoid;
>>                               *typetypmod = var->datatype->atttypmod;
>> +
>> +                             /*
>> +                              * explicit deTOAST and decomprim for varlena types
>
> "decompress", perhaps?
>

fixed

>> +                              */
>> +                             if (var->should_be_detoasted)
>> +                             {
>> +                                     Datum dvalue;
>> +
>> +                                     Assert(!var->isnull);
>> +
>> +                                     oldcontext = MemoryContextSwitchTo(estate->fn_mcxt);
>> +                                     dvalue = PointerGetDatum(PG_DETOAST_DATUM(var->value));
>> +                                     if (dvalue != var->value)
>> +                                     {
>> +                                             if (var->freeval)
>> +                                                     free_var(var);
>> +                                             var->value = dvalue;
>> +                                             var->freeval = true;
>> +                                     }
>> +                                     MemoryContextSwitchTo(oldcontext);
>
> This line adds trailing white space.
>
>> +                                     var->should_be_detoasted = false;
>> +                             }
>> +
>>                               *value = var->value;
>>                               *isnull = var->isnull;
>>                               break;
>
>> *** ./plpgsql.h.orig  2010-11-16 10:28:42.000000000 +0100
>> --- ./plpgsql.h       2010-11-22 13:12:38.897851879 +0100
>
>> ***************
>> *** 644,649 ****
>> --- 645,651 ----
>>       bool            fn_is_trigger;
>>       PLpgSQL_func_hashkey *fn_hashkey;       /* back-link to hashtable key */
>>       MemoryContext fn_cxt;
>> +     MemoryContext   fn_mcxt;                /* link to function's memory context */
>>
>>       Oid                     fn_rettype;
>>       int                     fn_rettyplen;
>> ***************
>> *** 692,697 ****
>> --- 694,701 ----
>>       Oid                     rettype;                /* type of current retval */
>>
>>       Oid                     fn_rettype;             /* info about declared function rettype */
>> +     MemoryContext   fn_mcxt;                /* link to function's memory context */
>> +
>>       bool            retistuple;
>>       bool            retisset;
>>
>
> I only see PLpgSQL_execstate.fn_mcxt getting populated in this patch.  Is the
> PLpgSQL_function.fn_mcxt leftover from an earlier design?

I have to access to top execution context from exec_eval_datum
function. This function can be called from parser's context, and
without explicit switch to top execution context a variables are
detoasted in wrong context.

>
> I could not readily tell the difference between fn_cxt and fn_mcxt.  As I
> understand it, fn_mcxt is the SPI procCxt, and fn_cxt is the long-lived context
> used to cache facts across many transactions.  Perhaps name the member something
> like "top_cxt", "exec_cxt" or "proc_cxt" and comment it like "/* SPI Proc memory
> context */" to make this clearer.

I used a top_exec_cxt name

Pavel Stehule
Regards

>