Re: patch: fix performance problems with repated decomprimation of varlena values in plpgsql - Mailing list pgsql-hackers
From | Pavel Stehule |
---|---|
Subject | Re: patch: fix performance problems with repated decomprimation of varlena values in plpgsql |
Date | |
Msg-id | AANLkTi=vVtxUrpkrsGqf4pz7Lb+cu2_weEAhnoDHGokR@mail.gmail.com Whole thread Raw |
In response to | Re: patch: fix performance problems with repated decomprimation of varlena values in plpgsql (Noah Misch <noah@leadboat.com>) |
Responses |
Re: patch: fix performance problems with repated decomprimation of
varlena values in plpgsql
(Noah Misch <noah@leadboat.com>)
|
List | pgsql-hackers |
Hello 2011/1/15 Noah Misch <noah@leadboat.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 >
Attachment
pgsql-hackers by date: