From 35b039a26ca2cdcfae94c5aacd0ac4a0904b5285 Mon Sep 17 00:00:00 2001
From: Tom Lane <tgl@sss.pgh.pa.us>
Date: Tue, 30 Jan 2007 22:05:13 +0000
Subject: [PATCH] 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.
---
src/pl/plpgsql/src/pl_comp.c | 109 ++++++++++++++++++++++++++------
src/pl/plpgsql/src/pl_handler.c | 33 +++++++---
src/pl/plpgsql/src/plpgsql.h | 4 +-
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 bfb924d66dd..f9eb233e8c4 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.109 2007/01/05 22:20:02 momjian Exp $
+ * $PostgreSQL: pgsql/src/pl/plpgsql/src/pl_comp.c,v 1.110 2007/01/30 22:05:12 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 38e25f940dd..9b02160da4d 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.35 2007/01/28 16:15:49 tgl Exp $
+ * $PostgreSQL: pgsql/src/pl/plpgsql/src/pl_handler.c,v 1.36 2007/01/30 22:05:13 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 e764db40faa..dad5ba5bb4b 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.83 2007/01/28 16:15:49 tgl Exp $
+ * $PostgreSQL: pgsql/src/pl/plpgsql/src/plpgsql.h,v 1.84 2007/01/30 22:05:13 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;
--
GitLab