Restructure plpgsql's caching of 'simple' expression evaluation trees

to be less dangerous, and often faster as well.  ExprState trees are
not kept across transaction boundaries; this eliminates problems with
resource leakage in failed transactions.  But by keeping them in a
per-transaction EState, we can safely arrange for a single ExprState
to be shared by all the expression evaluations done in a given plpgsql
function call.  (Formerly it seemed necessary to create and destroy an
ExprState for each exec_eval_simple_expr() call.)  This saves time in
any scenario where a plpgsql function executes more than one expression.
Seems to be about as fast as 7.3 for simple cases, and significantly
faster for functions that do a lot of calculations.

src/pl/plpgsql/src/pl_exec.c

@@ -3,7 +3,7 @@
  *			  procedural language
  *
  * IDENTIFICATION
- *	  $Header: /cvsroot/pgsql/src/pl/plpgsql/src/pl_exec.c,v 1.91 2003/09/25 23:02:12 tgl Exp $
+ *	  $Header: /cvsroot/pgsql/src/pl/plpgsql/src/pl_exec.c,v 1.92 2003/09/28 23:37:45 tgl Exp $
  *
  *	  This software is copyrighted by Jan Wieck - Hamburg.
  *
@@ -57,6 +57,19 @@
 
 static const char *const raise_skip_msg = "RAISE";
 
+/*
+ * All plpgsql function executions within a single transaction share
+ * the same executor EState for evaluating "simple" expressions.  Each
+ * function call creates its own "eval_econtext" ExprContext within this
+ * estate.  We destroy the estate at transaction shutdown to ensure there
+ * is no permanent leakage of memory (especially for xact abort case).
+ *
+ * If a simple PLpgSQL_expr has been used in the current xact, it is
+ * linked into the active_simple_exprs list.
+ */
+static EState *simple_eval_estate = NULL;
+static PLpgSQL_expr *active_simple_exprs = NULL;
+
 /************************************************************
  * Local function forward declarations
  ************************************************************/
@@ -135,7 +148,7 @@ static void exec_eval_datum(PLpgSQL_execstate * estate,
 				Oid *typeid,
 				Datum *value,
 				bool *isnull);
-static int exec_eval_subscript(PLpgSQL_execstate * estate,
+static int exec_eval_integer(PLpgSQL_execstate * estate,
 					PLpgSQL_expr * expr,
 					bool *isNull);
 static Datum exec_eval_expr(PLpgSQL_execstate * estate,
@@ -381,6 +394,9 @@ plpgsql_exec_function(PLpgSQL_function * func, FunctionCallInfo fcinfo)
 	}
 
 	/* Clean up any leftover temporary memory */
+	if (estate.eval_econtext != NULL)
+		FreeExprContext(estate.eval_econtext);
+	estate.eval_econtext = NULL;
 	exec_eval_cleanup(&estate);
 
 	/*
@@ -653,6 +669,9 @@ plpgsql_exec_trigger(PLpgSQL_function * func,
 	}
 
 	/* Clean up any leftover temporary memory */
+	if (estate.eval_econtext != NULL)
+		FreeExprContext(estate.eval_econtext);
+	estate.eval_econtext = NULL;
 	exec_eval_cleanup(&estate);
 
 	/*
@@ -1915,10 +1934,9 @@ exec_eval_cleanup(PLpgSQL_execstate * estate)
 		SPI_freetuptable(estate->eval_tuptable);
 	estate->eval_tuptable = NULL;
 
-	/* Clear result of exec_eval_simple_expr */
+	/* Clear result of exec_eval_simple_expr (but keep the econtext) */
 	if (estate->eval_econtext != NULL)
-		FreeExprContext(estate->eval_econtext);
+		ResetExprContext(estate->eval_econtext);
-	estate->eval_econtext = NULL;
 }
 
 
@@ -1962,7 +1980,7 @@ exec_prepare_plan(PLpgSQL_execstate * estate,
 	expr->plan = SPI_saveplan(plan);
 	spi_plan = (_SPI_plan *) expr->plan;
 	expr->plan_argtypes = spi_plan->argtypes;
-	expr->plan_simple_expr = NULL;
+	expr->expr_simple_expr = NULL;
 	exec_simple_check_plan(expr);
 
 	SPI_freeplan(plan);
@@ -2931,9 +2949,9 @@ exec_assign_value(PLpgSQL_execstate * estate,
 				bool		subisnull;
 
 				subscriptvals[i] =
-					exec_eval_subscript(estate,
+					exec_eval_integer(estate,
-										subscripts[nsubscripts - 1 - i],
+									  subscripts[nsubscripts - 1 - i],
-										&subisnull);
+									  &subisnull);
 				havenullsubscript |= subisnull;
 			}
 
@@ -3065,7 +3083,7 @@ exec_eval_datum(PLpgSQL_execstate * estate,
 		case PLPGSQL_DTYPE_TRIGARG:
 			trigarg = (PLpgSQL_trigarg *) datum;
 			*typeid = TEXTOID;
-			tgargno = exec_eval_subscript(estate, trigarg->argnum, isnull);
+			tgargno = exec_eval_integer(estate, trigarg->argnum, isnull);
 			if (*isnull || tgargno < 0 || tgargno >= estate->trig_nargs)
 			{
 				*value = (Datum) 0;
@@ -3089,33 +3107,28 @@ exec_eval_datum(PLpgSQL_execstate * estate,
 }
 
 /* ----------
- * exec_eval_subscript		Hack to allow subscripting of result variables.
+ * exec_eval_integer		Evaluate an expression, coerce result to int4
  *
- * The caller may already have an open eval_econtext, which we have to
+ * Note we do not do exec_eval_cleanup here; the caller must do it at
- * save and restore around the call of exec_eval_expr.
+ * some later point.  (We do this because the caller may be holding the
+ * results of other, pass-by-reference, expression evaluations, such as
+ * an array value to be subscripted.  Also see notes in exec_eval_simple_expr
+ * about allocation of the parameter array.)
  * ----------
  */
 static int
-exec_eval_subscript(PLpgSQL_execstate * estate,
+exec_eval_integer(PLpgSQL_execstate * estate,
-					PLpgSQL_expr * expr,
+				  PLpgSQL_expr * expr,
-					bool *isNull)
+				  bool *isNull)
 {
-	ExprContext *save_econtext;
+	Datum		exprdatum;
-	Datum		subscriptdatum;
+	Oid			exprtypeid;
-	Oid			subscripttypeid;
-	int			result;
 
-	save_econtext = estate->eval_econtext;
+	exprdatum = exec_eval_expr(estate, expr, isNull, &exprtypeid);
-	estate->eval_econtext = NULL;
+	exprdatum = exec_simple_cast_value(exprdatum, exprtypeid,
-	subscriptdatum = exec_eval_expr(estate, expr, isNull, &subscripttypeid);
+									   INT4OID, -1,
-	subscriptdatum = exec_simple_cast_value(subscriptdatum,
+									   isNull);
-											subscripttypeid,
+	return DatumGetInt32(exprdatum);
-											INT4OID, -1,
-											isNull);
-	result = DatumGetInt32(subscriptdatum);
-	exec_eval_cleanup(estate);
-	estate->eval_econtext = save_econtext;
-	return result;
 }
 
 /* ----------
@@ -3143,7 +3156,7 @@ exec_eval_expr(PLpgSQL_execstate * estate,
 	 * If this is a simple expression, bypass SPI and use the executor
 	 * directly
 	 */
-	if (expr->plan_simple_expr != NULL)
+	if (expr->expr_simple_expr != NULL)
 		return exec_eval_simple_expr(estate, expr, isNull, rettype);
 
 	rc = exec_run_select(estate, expr, 2, NULL);
@@ -3262,6 +3275,10 @@ exec_run_select(PLpgSQL_execstate * estate,
 /* ----------
  * exec_eval_simple_expr -		Evaluate a simple expression returning
  *								a Datum by directly calling ExecEvalExpr().
+ *
+ * Note: if pass-by-reference, the result is in the eval_econtext's
+ * temporary memory context.  It will be freed when exec_eval_cleanup
+ * is done.
  * ----------
  */
 static Datum
@@ -3271,64 +3288,97 @@ exec_eval_simple_expr(PLpgSQL_execstate * estate,
 					  Oid *rettype)
 {
 	Datum		retval;
-	int			i;
 	ExprContext *econtext;
 	ParamListInfo paramLI;
+	int			i;
 
 	/*
-	 * Create an expression context to hold the arguments and the result
+	 * Pass back previously-determined result type.
-	 * of this expression evaluation.  This must be a child of the EState
-	 * we created in the SPI plan's context.
 	 */
-	econtext = CreateExprContext(expr->plan_simple_estate);
+	*rettype = expr->expr_simple_type;
+
+	/*
+	 * Create an EState for evaluation of simple expressions, if there's
+	 * not one already in the current transaction.  The EState is made a
+	 * child of TopTransactionContext so it will have the right lifespan.
+	 */
+	if (simple_eval_estate == NULL)
+	{
+		MemoryContext oldcontext;
+
+		oldcontext = MemoryContextSwitchTo(TopTransactionContext);
+		simple_eval_estate = CreateExecutorState();
+		MemoryContextSwitchTo(oldcontext);
+	}
+
+	/*
+	 * Prepare the expression for execution, if it's not been done already
+	 * in the current transaction.
+	 */
+	if (expr->expr_simple_state == NULL)
+	{
+		expr->expr_simple_state = ExecPrepareExpr(expr->expr_simple_expr,
+												  simple_eval_estate);
+		/* Add it to list for cleanup */
+		expr->expr_simple_next = active_simple_exprs;
+		active_simple_exprs = expr;
+	}
+
+	/*
+	 * Create an expression context for simple expressions, if there's
+	 * not one already in the current function call.  This must be a
+	 * child of simple_eval_estate.
+	 */
+	econtext = estate->eval_econtext;
+	if (econtext == NULL)
+	{
+		econtext = CreateExprContext(simple_eval_estate);
+		estate->eval_econtext = econtext;
+	}
 
 	/*
 	 * Param list can live in econtext's temporary memory context.
+	 *
+	 * XXX think about avoiding repeated palloc's for param lists?
+	 * Beware however that this routine is re-entrant: exec_eval_datum()
+	 * can call it back for subscript evaluation, and so there can be a
+	 * need to have more than one active param list.
 	 */
 	paramLI = (ParamListInfo)
 		MemoryContextAlloc(econtext->ecxt_per_tuple_memory,
 						(expr->nparams + 1) * sizeof(ParamListInfoData));
-	econtext->ecxt_param_list_info = paramLI;
 
 	/*
-	 * Put the parameter values into the parameter list info of the
+	 * Put the parameter values into the parameter list entries.
-	 * expression context.
 	 */
-	for (i = 0; i < expr->nparams; i++, paramLI++)
+	for (i = 0; i < expr->nparams; i++)
 	{
 		PLpgSQL_datum *datum = estate->datums[expr->params[i]];
 		Oid			paramtypeid;
 
-		paramLI->kind = PARAM_NUM;
+		paramLI[i].kind = PARAM_NUM;
-		paramLI->id = i + 1;
+		paramLI[i].id = i + 1;
 		exec_eval_datum(estate, datum, expr->plan_argtypes[i],
-						&paramtypeid, &paramLI->value, &paramLI->isnull);
+						&paramtypeid,
+						&paramLI[i].value, &paramLI[i].isnull);
 	}
-	paramLI->kind = PARAM_INVALID;
+	paramLI[i].kind = PARAM_INVALID;
 
 	/*
-	 * Initialize things
+	 * Now we can safely make the econtext point to the param list.
 	 */
-	*rettype = expr->plan_simple_type;
+	econtext->ecxt_param_list_info = paramLI;
 
 	/*
 	 * Now call the executor to evaluate the expression
 	 */
 	SPI_push();
-	retval = ExecEvalExprSwitchContext(expr->plan_simple_expr,
+	retval = ExecEvalExprSwitchContext(expr->expr_simple_state,
 									   econtext,
 									   isNull,
 									   NULL);
 	SPI_pop();
 
-	/*
-	 * Note: if pass-by-reference, the result is in the econtext's
-	 * temporary memory context.  It will be freed when exec_eval_cleanup
-	 * is done.
-	 */
-	Assert(estate->eval_econtext == NULL);
-	estate->eval_econtext = econtext;
-
 	/*
 	 * That's it.
 	 */
@@ -3795,9 +3845,8 @@ exec_simple_check_plan(PLpgSQL_expr * expr)
 	_SPI_plan  *spi_plan = (_SPI_plan *) expr->plan;
 	Plan	   *plan;
 	TargetEntry *tle;
-	MemoryContext oldcontext;
 
-	expr->plan_simple_expr = NULL;
+	expr->expr_simple_expr = NULL;
 
 	/*
 	 * 1. We can only evaluate queries that resulted in one single
@@ -3842,21 +3891,14 @@ exec_simple_check_plan(PLpgSQL_expr * expr)
 		return;
 
 	/*
-	 * Yes - this is a simple expression.  Prepare to execute it. We need
+	 * Yes - this is a simple expression.  Mark it as such, and initialize
-	 * an EState and an expression state tree, which we'll put into the
+	 * state to "not executing".
-	 * plan context so they will have appropriate lifespan.
 	 */
-	oldcontext = MemoryContextSwitchTo(spi_plan->plancxt);
+	expr->expr_simple_expr = tle->expr;
-
+	expr->expr_simple_state = NULL;
-	expr->plan_simple_estate = CreateExecutorState();
+	expr->expr_simple_next = NULL;
-
-	expr->plan_simple_expr = ExecPrepareExpr(tle->expr,
-											 expr->plan_simple_estate);
-
-	MemoryContextSwitchTo(oldcontext);
-
 	/* Also stash away the expression result type */
-	expr->plan_simple_type = exprType((Node *) tle->expr);
+	expr->expr_simple_type = exprType((Node *) tle->expr);
 }
 
 /*
@@ -3893,3 +3935,35 @@ exec_set_found(PLpgSQL_execstate * estate, bool state)
 	var->value = (Datum) state;
 	var->isnull = false;
 }
+
+/*
+ * plpgsql_eoxact --- post-transaction-commit-or-abort cleanup
+ *
+ * If a simple_eval_estate was created in the current transaction,
+ * it has to be cleaned up, and we have to mark all active PLpgSQL_expr
+ * structs that are using it as no longer active.
+ */
+void
+plpgsql_eoxact(bool isCommit, void *arg)
+{
+	PLpgSQL_expr *expr;
+	PLpgSQL_expr *enext;
+
+	/* Mark all active exprs as inactive */
+	for (expr = active_simple_exprs; expr; expr = enext)
+	{
+		enext = expr->expr_simple_next;
+		expr->expr_simple_state = NULL;
+		expr->expr_simple_next = NULL;
+	}
+	active_simple_exprs = NULL;
+	/*
+	 * If we are doing a clean transaction shutdown, free the EState
+	 * (so that any remaining resources will be released correctly).
+	 * In an abort, we expect the regular abort recovery procedures to
+	 * release everything of interest.
+	 */
+	if (isCommit && simple_eval_estate)
+		FreeExecutorState(simple_eval_estate);
+	simple_eval_estate = NULL;
+}

src/pl/plpgsql/src/pl_handler.c

@@ -3,7 +3,7 @@
  *			  procedural language
  *
  * IDENTIFICATION
- *	  $Header: /cvsroot/pgsql/src/pl/plpgsql/src/pl_handler.c,v 1.17 2003/08/04 00:43:33 momjian Exp $
+ *	  $Header: /cvsroot/pgsql/src/pl/plpgsql/src/pl_handler.c,v 1.18 2003/09/28 23:37:45 tgl Exp $
  *
  *	  This software is copyrighted by Jan Wieck - Hamburg.
  *
@@ -46,7 +46,6 @@
 
 static int	plpgsql_firstcall = 1;
 
-void		plpgsql_init(void);
 static void plpgsql_init_all(void);
 
 
@@ -64,6 +63,8 @@ plpgsql_init(void)
 
 	plpgsql_HashTableInit();
 
+	RegisterEOXactCallback(plpgsql_eoxact, NULL);
+
 	plpgsql_firstcall = 0;
 }
 

src/pl/plpgsql/src/plpgsql.h

@@ -3,7 +3,7 @@
  *			  procedural language
  *
  * IDENTIFICATION
- *	  $Header: /cvsroot/pgsql/src/pl/plpgsql/src/plpgsql.h,v 1.41 2003/09/25 23:02:12 tgl Exp $
+ *	  $Header: /cvsroot/pgsql/src/pl/plpgsql/src/plpgsql.h,v 1.42 2003/09/28 23:37:45 tgl Exp $
  *
  *	  This software is copyrighted by Jan Wieck - Hamburg.
  *
@@ -166,18 +166,22 @@ typedef struct
 }	PLpgSQL_datum;
 
 
-typedef struct
+typedef struct PLpgSQL_expr
 {								/* SQL Query to plan and execute	*/
 	int			dtype;
 	int			exprno;
 	char	   *query;
 	void	   *plan;
-	ExprState  *plan_simple_expr;
-	EState	   *plan_simple_estate;
-	Oid			plan_simple_type;
 	Oid		   *plan_argtypes;
+	/* fields for "simple expression" fast-path execution: */
+	Expr	   *expr_simple_expr;	/* NULL means not a simple expr */
+	Oid			expr_simple_type;
+	/* if expr is simple AND in use in current xact, these fields are set: */
+	ExprState  *expr_simple_state;
+	struct PLpgSQL_expr *expr_simple_next;
+	/* params to pass to expr */
 	int			nparams;
-	int			params[1];
+	int			params[1];		/* VARIABLE SIZE ARRAY ... must be last */
 }	PLpgSQL_expr;
 
 
@@ -636,6 +640,7 @@ extern void plpgsql_HashTableInit(void);
  * Functions in pl_handler.c
  * ----------
  */
+extern void plpgsql_init(void);
 extern Datum plpgsql_call_handler(PG_FUNCTION_ARGS);
 
 /* ----------
@@ -646,7 +651,7 @@ extern Datum plpgsql_exec_function(PLpgSQL_function * func,
 					  FunctionCallInfo fcinfo);
 extern HeapTuple plpgsql_exec_trigger(PLpgSQL_function * func,
 					 TriggerData *trigdata);
-
+extern void plpgsql_eoxact(bool isCommit, void *arg);
 
 /* ----------
  * Functions for the dynamic string handling in pl_funcs.c