Repair oversights in the mechanism used to store compiled plpgsql functions.
authorTom Lane <tgl@sss.pgh.pa.us>
Tue, 30 Jan 2007 22:05:13 +0000 (22:05 +0000)
committerTom Lane <tgl@sss.pgh.pa.us>
Tue, 30 Jan 2007 22:05:13 +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 bfb924d66dd0f852d8400d5b88a975d0af166114..f9eb233e8c4516c9a9d837578878678a52169e4f 100644 (file)
@@ -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;
 }
index 38e25f940dd9115ea24d687146020cbb073d819f..9b02160da4ddf89dcf30ef2f73a433a9945ea824 100644 (file)
@@ -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
index e764db40faa02bc7477d94aef547c08c768e5690..dad5ba5bb4b3fae9e582937ca9f5f727b8cbbc7d 100644 (file)
@@ -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;