Repair oversights in the mechanism used to store compiled plpgsql functions.

The original coding failed (tried to access deallocated memory) if there were
two active call sites (fn_extra pointers) for the same function and the
function definition was updated.  Also, if an update of a recursive function
was detected upon nested entry to the function, the existing compiled version
was summarily deallocated, resulting in crash upon return to the outer
instance.  Problem observed while studying a bug report from Sergiy
Vyshnevetskiy.

Bug does not exist before 8.1 since older versions just leaked the memory of
obsoleted compiled functions, rather than trying to reclaim it.

3 files changed, 115 insertions, 31 deletions

diff --git a/src/pl/plpgsql/src/pl_comp.c b/src/pl/plpgsql/src/pl_comp.c
index 654e8ccf43..15cdab7698 100644
--- a/src/pl/plpgsql/src/pl_comp.c
+++ b/src/pl/plpgsql/src/pl_comp.c
@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *	  $PostgreSQL: pgsql/src/pl/plpgsql/src/pl_comp.c,v 1.108 2006/10/04 00:30:13 momjian Exp $
+ *	  $PostgreSQL: pgsql/src/pl/plpgsql/src/pl_comp.c,v 1.108.2.1 2007/01/30 22:05:20 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -93,6 +93,7 @@ static const ExceptionLabelMap exception_label_map[] = {
  */
 static PLpgSQL_function *do_compile(FunctionCallInfo fcinfo,
 		   HeapTuple procTup,
+		   PLpgSQL_function *function,
 		   PLpgSQL_func_hashkey *hashkey,
 		   bool forValidator);
 static PLpgSQL_row *build_row_from_class(Oid classOid);
@@ -130,6 +131,7 @@ plpgsql_compile(FunctionCallInfo fcinfo, bool forValidator)
 	Form_pg_proc procStruct;
 	PLpgSQL_function *function;
 	PLpgSQL_func_hashkey hashkey;
+	bool		function_valid = false;
 	bool		hashkey_valid = false;
 
 	/*
@@ -148,6 +150,7 @@ plpgsql_compile(FunctionCallInfo fcinfo, bool forValidator)
 	 */
 	function = (PLpgSQL_function *) fcinfo->flinfo->fn_extra;
 
+recheck:
 	if (!function)
 	{
 		/* Compute hashkey using function signature and actual arg types */
@@ -161,19 +164,43 @@ plpgsql_compile(FunctionCallInfo fcinfo, bool forValidator)
 	if (function)
 	{
 		/* We have a compiled function, but is it still valid? */
-		if (!(function->fn_xmin == HeapTupleHeaderGetXmin(procTup->t_data) &&
-			  function->fn_cmin == HeapTupleHeaderGetCmin(procTup->t_data)))
+		if (function->fn_xmin == HeapTupleHeaderGetXmin(procTup->t_data) &&
+			function->fn_cmin == HeapTupleHeaderGetCmin(procTup->t_data))
+			function_valid = true;
+		else
 		{
-			/* Nope, drop the function and associated storage */
+			/*
+			 * Nope, so remove it from hashtable and try to drop associated
+			 * storage (if not done already).
+			 */
 			delete_function(function);
-			function = NULL;
+			/*
+			 * If the function isn't in active use then we can overwrite the
+			 * func struct with new data, allowing any other existing fn_extra
+			 * pointers to make use of the new definition on their next use.
+			 * If it is in use then just leave it alone and make a new one.
+			 * (The active invocations will run to completion using the
+			 * previous definition, and then the cache entry will just be
+			 * leaked; doesn't seem worth adding code to clean it up, given
+			 * what a corner case this is.)
+			 *
+			 * If we found the function struct via fn_extra then it's possible
+			 * a replacement has already been made, so go back and recheck
+			 * the hashtable.
+			 */
+			if (function->use_count != 0)
+			{
+				function = NULL;
+				if (!hashkey_valid)
+					goto recheck;
+			}
 		}
 	}
 
 	/*
 	 * If the function wasn't found or was out-of-date, we have to compile it
 	 */
-	if (!function)
+	if (!function_valid)
 	{
 		/*
 		 * Calculate hashkey if we didn't already; we'll need it to store the
@@ -186,7 +213,8 @@ plpgsql_compile(FunctionCallInfo fcinfo, bool forValidator)
 		/*
 		 * Do the hard part.
 		 */
-		function = do_compile(fcinfo, procTup, &hashkey, forValidator);
+		function = do_compile(fcinfo, procTup, function,
+							  &hashkey, forValidator);
 	}
 
 	ReleaseSysCache(procTup);
@@ -205,6 +233,9 @@ plpgsql_compile(FunctionCallInfo fcinfo, bool forValidator)
 /*
  * This is the slow part of plpgsql_compile().
  *
+ * The passed-in "function" pointer is either NULL or an already-allocated
+ * function struct to overwrite.
+ *
  * While compiling a function, the CurrentMemoryContext is the
  * per-function memory context of the function we are compiling. That
  * means a palloc() will allocate storage with the same lifetime as
@@ -217,16 +248,19 @@ plpgsql_compile(FunctionCallInfo fcinfo, bool forValidator)
  * switch into a short-term context before calling into the
  * backend. An appropriate context for performing short-term
  * allocations is the compile_tmp_cxt.
+ *
+ * NB: this code is not re-entrant.  We assume that nothing we do here could
+ * result in the invocation of another plpgsql function.
  */
 static PLpgSQL_function *
 do_compile(FunctionCallInfo fcinfo,
 		   HeapTuple procTup,
+		   PLpgSQL_function *function,
 		   PLpgSQL_func_hashkey *hashkey,
 		   bool forValidator)
 {
 	Form_pg_proc procStruct = (Form_pg_proc) GETSTRUCT(procTup);
 	int			functype = CALLED_AS_TRIGGER(fcinfo) ? T_TRIGGER : T_FUNCTION;
-	PLpgSQL_function *function;
 	Datum		prosrcdatum;
 	bool		isnull;
 	char	   *proc_source;
@@ -292,9 +326,24 @@ do_compile(FunctionCallInfo fcinfo,
 	plpgsql_check_syntax = forValidator;
 
 	/*
-	 * Create the new function node. We allocate the function and all of its
-	 * compile-time storage (e.g. parse tree) in its own memory context. This
-	 * allows us to reclaim the function's storage cleanly.
+	 * Create the new function struct, if not done already.  The function
+	 * structs are never thrown away, so keep them in TopMemoryContext.
+	 */
+	if (function == NULL)
+	{
+		function = (PLpgSQL_function *)
+			MemoryContextAllocZero(TopMemoryContext, sizeof(PLpgSQL_function));
+	}
+	else
+	{
+		/* re-using a previously existing struct, so clear it out */
+		memset(function, 0, sizeof(PLpgSQL_function));
+	}
+	plpgsql_curr_compile = function;
+
+	/*
+	 * All the rest of the compile-time storage (e.g. parse tree) is kept in
+	 * its own memory context, so it can be reclaimed easily.
 	 */
 	func_cxt = AllocSetContextCreate(TopMemoryContext,
 									 "PL/PgSQL function context",
@@ -302,8 +351,6 @@ do_compile(FunctionCallInfo fcinfo,
 									 ALLOCSET_DEFAULT_INITSIZE,
 									 ALLOCSET_DEFAULT_MAXSIZE);
 	compile_tmp_cxt = MemoryContextSwitchTo(func_cxt);
-	function = palloc0(sizeof(*function));
-	plpgsql_curr_compile = function;
 
 	function->fn_name = pstrdup(NameStr(procStruct->proname));
 	function->fn_oid = fcinfo->flinfo->fn_oid;
@@ -1990,19 +2037,32 @@ plpgsql_resolve_polymorphic_argtypes(int numargs,
 	}
 }
 
+/*
+ * delete_function - clean up as much as possible of a stale function cache
+ *
+ * We can't release the PLpgSQL_function struct itself, because of the
+ * possibility that there are fn_extra pointers to it.  We can release
+ * the subsidiary storage, but only if there are no active evaluations
+ * in progress.  Otherwise we'll just leak that storage.  Since the
+ * case would only occur if a pg_proc update is detected during a nested
+ * recursive call on the function, a leak seems acceptable.
+ *
+ * Note that this can be called more than once if there are multiple fn_extra
+ * pointers to the same function cache.  Hence be careful not to do things
+ * twice.
+ */
 static void
 delete_function(PLpgSQL_function *func)
 {
-	/* remove function from hash table */
+	/* remove function from hash table (might be done already) */
 	plpgsql_HashTableDelete(func);
 
-	/* release the function's storage */
-	MemoryContextDelete(func->fn_cxt);
-
-	/*
-	 * Caller should be sure not to use passed-in pointer, as it now points to
-	 * pfree'd storage
-	 */
+	/* release the function's storage if safe and not done already */
+	if (func->use_count == 0 && func->fn_cxt)
+	{
+		MemoryContextDelete(func->fn_cxt);
+		func->fn_cxt = NULL;
+	}
 }
 
 /* exported so we can call it from plpgsql_init() */
@@ -2063,10 +2123,17 @@ plpgsql_HashTableDelete(PLpgSQL_function *function)
 {
 	plpgsql_HashEnt *hentry;
 
+	/* do nothing if not in table */
+	if (function->fn_hashkey == NULL)
+		return;
+
 	hentry = (plpgsql_HashEnt *) hash_search(plpgsql_HashTable,
 											 (void *) function->fn_hashkey,
 											 HASH_REMOVE,
 											 NULL);
 	if (hentry == NULL)
 		elog(WARNING, "trying to delete function that does not exist");
+
+	/* remove back link, which no longer points to allocated storage */
+	function->fn_hashkey = NULL;
 }
diff --git a/src/pl/plpgsql/src/pl_handler.c b/src/pl/plpgsql/src/pl_handler.c
index 9fbce30ea6..993eee4fb4 100644
--- a/src/pl/plpgsql/src/pl_handler.c
+++ b/src/pl/plpgsql/src/pl_handler.c
@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *	  $PostgreSQL: pgsql/src/pl/plpgsql/src/pl_handler.c,v 1.33.2.1 2007/01/28 16:15:58 tgl Exp $
+ *	  $PostgreSQL: pgsql/src/pl/plpgsql/src/pl_handler.c,v 1.33.2.2 2007/01/30 22:05:20 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -79,15 +79,30 @@ plpgsql_call_handler(PG_FUNCTION_ARGS)
 	/* Find or compile the function */
 	func = plpgsql_compile(fcinfo, false);
 
-	/*
-	 * Determine if called as function or trigger and call appropriate
-	 * subhandler
-	 */
-	if (CALLED_AS_TRIGGER(fcinfo))
-		retval = PointerGetDatum(plpgsql_exec_trigger(func,
+	/* Mark the function as busy, so it can't be deleted from under us */
+	func->use_count++;
+
+	PG_TRY();
+	{
+		/*
+		 * Determine if called as function or trigger and call appropriate
+		 * subhandler
+		 */
+		if (CALLED_AS_TRIGGER(fcinfo))
+			retval = PointerGetDatum(plpgsql_exec_trigger(func,
 										   (TriggerData *) fcinfo->context));
-	else
-		retval = plpgsql_exec_function(func, fcinfo);
+		else
+			retval = plpgsql_exec_function(func, fcinfo);
+	}
+	PG_CATCH();
+	{
+		/* Decrement use-count and propagate error */
+		func->use_count--;
+		PG_RE_THROW();
+	}
+	PG_END_TRY();
+
+	func->use_count--;
 
 	/*
 	 * Disconnect from SPI manager
diff --git a/src/pl/plpgsql/src/plpgsql.h b/src/pl/plpgsql/src/plpgsql.h
index 4a68dcc0a8..b8bad2b1de 100644
--- a/src/pl/plpgsql/src/plpgsql.h
+++ b/src/pl/plpgsql/src/plpgsql.h
@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *	  $PostgreSQL: pgsql/src/pl/plpgsql/src/plpgsql.h,v 1.81.2.1 2007/01/28 16:15:58 tgl Exp $
+ *	  $PostgreSQL: pgsql/src/pl/plpgsql/src/plpgsql.h,v 1.81.2.2 2007/01/30 22:05:20 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -580,6 +580,8 @@ typedef struct PLpgSQL_function
 	int			ndatums;
 	PLpgSQL_datum **datums;
 	PLpgSQL_stmt_block *action;
+
+	unsigned long use_count;
 } PLpgSQL_function;