Hello Pavel,
I'm reviewing this patch for CommitFest 2011-01.
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_zuatdzfp...@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?
> + */
> + 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 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.
\set datscale 1000
CREATE TABLE t (c text);
INSERT INTO t VALUES (repeat('a', 1024 * :datscale));
CREATE OR REPLACE FUNCTION f(runs int) RETURNS void LANGUAGE plpgsql AS $$
DECLARE
foo text;
BEGIN
SELECT c INTO foo FROM t;
FOR n IN 1 .. runs LOOP
PERFORM foo < 'x';
END LOOP;
END
$$;
-- datscale=1000: 20.4s before patch, 4.21s after patch
SELECT f(4000);
-- datscale=1: 20.8s before patch, 21.0s after patch
SELECT f(3000000);
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
