Fix SQL function execution to be safe with long-lived FmgrInfos.

fmgr_sql had been designed on the assumption that the FmgrInfo it's called
with has only query lifespan.  This is demonstrably unsafe in connection
with range types, as shown in bug #7881 from Andrew Gierth.  Fix things
so that we re-generate the function's cache data if the (sub)transaction
it was made in is no longer active.

Back-patch to 9.2.  This might be needed further back, but it's not clear
whether the case can realistically arise without range types, so for now
I'll desist from back-patching further.

diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
index fcece901cc76c988c0082e0684ffcaca5f70d637..2b103f4255933ca02cd773b3cc10963bbf8c206b 100644 (file)
--- a/src/backend/access/transam/xact.c
+++ b/src/backend/access/transam/xact.c
@@ -570,6 +570,27 @@ GetCurrentSubTransactionId(void)
        return s->subTransactionId;
 }
 
+/*
+ *     SubTransactionIsActive
+ *
+ * Test if the specified subxact ID is still active.  Note caller is
+ * responsible for checking whether this ID is relevant to the current xact.
+ */
+bool
+SubTransactionIsActive(SubTransactionId subxid)
+{
+       TransactionState s;
+
+       for (s = CurrentTransactionState; s != NULL; s = s->parent)
+       {
+               if (s->state == TRANS_ABORT)
+                       continue;
+               if (s->subTransactionId == subxid)
+                       return true;
+       }
+       return false;
+}
+
 
 /*
  *     GetCurrentCommandId

diff --git a/src/backend/executor/functions.c b/src/backend/executor/functions.c
index bf2f5c68829da0e22c04cc896b123a358dee82fd..ef184b69e25a30e006a3ccbe1c45d6adf0b5bdcd 100644 (file)
--- a/src/backend/executor/functions.c
+++ b/src/backend/executor/functions.c
@@ -24,10 +24,12 @@
 #include "nodes/nodeFuncs.h"
 #include "parser/parse_coerce.h"
 #include "parser/parse_func.h"
+#include "storage/proc.h"
 #include "tcop/utility.h"
 #include "utils/builtins.h"
 #include "utils/datum.h"
 #include "utils/lsyscache.h"
+#include "utils/memutils.h"
 #include "utils/snapmgr.h"
 #include "utils/syscache.h"
 
@@ -73,8 +75,17 @@ typedef struct execution_state
  * and linked to from the fn_extra field of the FmgrInfo struct.
  *
  * Note that currently this has only the lifespan of the calling query.
- * Someday we might want to consider caching the parse/plan results longer
- * than that.
+ * Someday we should rewrite this code to use plancache.c to save parse/plan
+ * results for longer than that.
+ *
+ * Physically, though, the data has the lifespan of the FmgrInfo that's used
+ * to call the function, and there are cases (particularly with indexes)
+ * where the FmgrInfo might survive across transactions.  We cannot assume
+ * that the parse/plan trees are good for longer than the (sub)transaction in
+ * which parsing was done, so we must mark the record with the LXID/subxid of
+ * its creation time, and regenerate everything if that's obsolete.  To avoid
+ * memory leakage when we do have to regenerate things, all the data is kept
+ * in a sub-context of the FmgrInfo's fn_mcxt.
  */
 typedef struct
 {
@@ -105,6 +116,12 @@ typedef struct
         * track of where the original query boundaries are.
         */
        List       *func_state;
+
+       MemoryContext fcontext;         /* memory context holding this struct and all
+                                                                * subsidiary data */
+
+       LocalTransactionId lxid;        /* lxid in which cache was made */
+       SubTransactionId subxid;        /* subxid in which cache was made */
 } SQLFunctionCache;
 
 typedef SQLFunctionCache *SQLFunctionCachePtr;
@@ -550,6 +567,8 @@ static void
 init_sql_fcache(FmgrInfo *finfo, Oid collation, bool lazyEvalOK)
 {
        Oid                     foid = finfo->fn_oid;
+       MemoryContext fcontext;
+       MemoryContext oldcontext;
        Oid                     rettype;
        HeapTuple       procedureTuple;
        Form_pg_proc procedureStruct;
@@ -561,7 +580,25 @@ init_sql_fcache(FmgrInfo *finfo, Oid collation, bool lazyEvalOK)
        Datum           tmp;
        bool            isNull;
 
+       /*
+        * Create memory context that holds all the SQLFunctionCache data.      It
+        * must be a child of whatever context holds the FmgrInfo.
+        */
+       fcontext = AllocSetContextCreate(finfo->fn_mcxt,
+                                                                        "SQL function data",
+                                                                        ALLOCSET_DEFAULT_MINSIZE,
+                                                                        ALLOCSET_DEFAULT_INITSIZE,
+                                                                        ALLOCSET_DEFAULT_MAXSIZE);
+
+       oldcontext = MemoryContextSwitchTo(fcontext);
+
+       /*
+        * Create the struct proper, link it to fcontext and fn_extra.  Once this
+        * is done, we'll be able to recover the memory after failure, even if the
+        * FmgrInfo is long-lived.
+        */
        fcache = (SQLFunctionCachePtr) palloc0(sizeof(SQLFunctionCache));
+       fcache->fcontext = fcontext;
        finfo->fn_extra = (void *) fcache;
 
        /*
@@ -634,6 +671,11 @@ init_sql_fcache(FmgrInfo *finfo, Oid collation, bool lazyEvalOK)
         * sublist, for example if the last query rewrites to DO INSTEAD NOTHING.
         * (It might not be unreasonable to throw an error in such a case, but
         * this is the historical behavior and it doesn't seem worth changing.)
+        *
+        * Note: since parsing and planning is done in fcontext, we will generate
+        * a lot of cruft that lives as long as the fcache does.  This is annoying
+        * but we'll not worry about it until the module is rewritten to use
+        * plancache.c.
         */
        raw_parsetree_list = pg_parse_query(fcache->src);
 
@@ -699,7 +741,13 @@ init_sql_fcache(FmgrInfo *finfo, Oid collation, bool lazyEvalOK)
                                                                                          fcache,
                                                                                          lazyEvalOK);
 
+       /* Mark fcache with time of creation to show it's valid */
+       fcache->lxid = MyProc->lxid;
+       fcache->subxid = GetCurrentSubTransactionId();
+
        ReleaseSysCache(procedureTuple);
+
+       MemoryContextSwitchTo(oldcontext);
 }
 
 /* Start up execution of one execution_state node */
@@ -922,9 +970,9 @@ postquel_get_single_result(TupleTableSlot *slot,
 Datum
 fmgr_sql(PG_FUNCTION_ARGS)
 {
-       MemoryContext oldcontext;
        SQLFunctionCachePtr fcache;
        ErrorContextCallback sqlerrcontext;
+       MemoryContext oldcontext;
        bool            randomAccess;
        bool            lazyEvalOK;
        bool            is_first;
@@ -935,13 +983,6 @@ fmgr_sql(PG_FUNCTION_ARGS)
        List       *eslist;
        ListCell   *eslc;
 
-       /*
-        * Switch to context in which the fcache lives.  This ensures that
-        * parsetrees, plans, etc, will have sufficient lifetime.  The
-        * sub-executor is responsible for deleting per-tuple information.
-        */
-       oldcontext = MemoryContextSwitchTo(fcinfo->flinfo->fn_mcxt);
-
        /*
         * Setup error traceback support for ereport()
         */
@@ -977,20 +1018,43 @@ fmgr_sql(PG_FUNCTION_ARGS)
        }
 
        /*
-        * Initialize fcache (build plans) if first time through.
+        * Initialize fcache (build plans) if first time through; or re-initialize
+        * if the cache is stale.
         */
        fcache = (SQLFunctionCachePtr) fcinfo->flinfo->fn_extra;
+
+       if (fcache != NULL)
+       {
+               if (fcache->lxid != MyProc->lxid ||
+                       !SubTransactionIsActive(fcache->subxid))
+               {
+                       /* It's stale; unlink and delete */
+                       fcinfo->flinfo->fn_extra = NULL;
+                       MemoryContextDelete(fcache->fcontext);
+                       fcache = NULL;
+               }
+       }
+
        if (fcache == NULL)
        {
                init_sql_fcache(fcinfo->flinfo, PG_GET_COLLATION(), lazyEvalOK);
                fcache = (SQLFunctionCachePtr) fcinfo->flinfo->fn_extra;
        }
-       eslist = fcache->func_state;
+
+       /*
+        * Switch to context in which the fcache lives.  This ensures that our
+        * tuplestore etc will have sufficient lifetime.  The sub-executor is
+        * responsible for deleting per-tuple information.      (XXX in the case of a
+        * long-lived FmgrInfo, this policy represents more memory leakage, but
+        * it's not entirely clear where to keep stuff instead.)
+        */
+       oldcontext = MemoryContextSwitchTo(fcache->fcontext);
 
        /*
         * Find first unfinished query in function, and note whether it's the
         * first query.
         */
+       eslist = fcache->func_state;
        es = NULL;
        is_first = true;
        foreach(eslc, eslist)

diff --git a/src/include/access/xact.h b/src/include/access/xact.h
index b12d2a0068531d311137ed5b18c8cd17e015cd0f..86dd6db777f518d59d722ea8dceb1275308aafb7 100644 (file)
--- a/src/include/access/xact.h
+++ b/src/include/access/xact.h
@@ -212,6 +212,7 @@ extern TransactionId GetCurrentTransactionId(void);
 extern TransactionId GetCurrentTransactionIdIfAny(void);
 extern TransactionId GetStableLatestTransactionId(void);
 extern SubTransactionId GetCurrentSubTransactionId(void);
+extern bool SubTransactionIsActive(SubTransactionId subxid);
 extern CommandId GetCurrentCommandId(bool used);
 extern TimestampTz GetCurrentTransactionStartTimestamp(void);
 extern TimestampTz GetCurrentStatementStartTimestamp(void);