]> git.ipfire.org Git - thirdparty/postgresql.git/commitdiff
Repair oversights in the mechanism used to store compiled plpgsql functions.
authorTom Lane <tgl@sss.pgh.pa.us>
Tue, 30 Jan 2007 22:05:25 +0000 (22:05 +0000)
committerTom Lane <tgl@sss.pgh.pa.us>
Tue, 30 Jan 2007 22:05:25 +0000 (22:05 +0000)
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
src/pl/plpgsql/src/pl_handler.c
src/pl/plpgsql/src/plpgsql.h

index 10b5a200a89fdeb44a58dbfeabf95ac49ba1a95a..c20a2b31837ddbc34ea8d4fc839616e163f47855 100644 (file)
@@ -3,7 +3,7 @@
  *                       procedural language
  *
  * IDENTIFICATION
- *       $PostgreSQL: pgsql/src/pl/plpgsql/src/pl_comp.c,v 1.94.2.2 2005/12/09 17:09:00 tgl Exp $
+ *       $PostgreSQL: pgsql/src/pl/plpgsql/src/pl_comp.c,v 1.94.2.3 2007/01/30 22:05:25 tgl Exp $
  *
  *       This software is copyrighted by Jan Wieck - Hamburg.
  *
@@ -121,6 +121,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 int fetchArgInfo(HeapTuple procTup,
@@ -161,6 +162,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;
 
        /*
@@ -179,6 +181,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 */
@@ -192,19 +195,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
@@ -217,7 +244,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);
@@ -236,6 +264,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
@@ -248,16 +279,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;
@@ -323,9 +357,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",
@@ -333,8 +382,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;
@@ -2100,19 +2147,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() */
@@ -2173,10 +2233,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;
 }
index 69d128e8698316a812423c2160816914fbba0558..43446f0a8af17bbf9131e19943e5285a24b69a6a 100644 (file)
@@ -3,7 +3,7 @@
  *                       procedural language
  *
  * IDENTIFICATION
- *       $PostgreSQL: pgsql/src/pl/plpgsql/src/pl_handler.c,v 1.26 2005/10/15 02:49:50 momjian Exp $
+ *       $PostgreSQL: pgsql/src/pl/plpgsql/src/pl_handler.c,v 1.26.2.1 2007/01/30 22:05:25 tgl Exp $
  *
  *       This software is copyrighted by Jan Wieck - Hamburg.
  *
@@ -112,15 +112,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
index ba1a818412030102b2c70c54688df31516bb3d50..d27ea9eab5395358f4dda437f18310899fdf6c99 100644 (file)
@@ -3,7 +3,7 @@
  *                       procedural language
  *
  * IDENTIFICATION
- *       $PostgreSQL: pgsql/src/pl/plpgsql/src/plpgsql.h,v 1.65.2.2 2006/03/02 05:34:17 tgl Exp $
+ *       $PostgreSQL: pgsql/src/pl/plpgsql/src/plpgsql.h,v 1.65.2.3 2007/01/30 22:05:25 tgl Exp $
  *
  *       This software is copyrighted by Jan Wieck - Hamburg.
  *
@@ -598,6 +598,8 @@ typedef struct PLpgSQL_function
        int                     ndatums;
        PLpgSQL_datum **datums;
        PLpgSQL_stmt_block *action;
+
+       unsigned long use_count;
 } PLpgSQL_function;