From: Tom Lane Date: Sun, 3 Mar 2013 22:40:04 +0000 (-0500) Subject: Fix SQL function execution to be safe with long-lived FmgrInfos. X-Git-Tag: REL9_2_4~29 X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=d4f4bdf23a47dc2b6122195bbff4b09c80e7e2f5;p=postgresql 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 fcece901cc..2b103f4255 100644 --- 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 bf2f5c6882..ef184b69e2 100644 --- 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 b12d2a0068..86dd6db777 100644 --- 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);