diff --git a/src/pl/plpgsql/src/pl_comp.c b/src/pl/plpgsql/src/pl_comp.c index bfb924d66d..f9eb233e8c 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 38e25f940d..9b02160da4 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 e764db40fa..dad5ba5bb4 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;