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

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_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?

> + 				 */
> + 				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.