I wrote: > Or we could add fields recording the current transaction/subtransaction > IDs, then throw away and regenerate the function cache entry if that > subxact is no longer active. This would be a bit more invasive/riskier > than throwing a "not supported" error, but it would preserve the > functionality.
The attached patch seems fairly reasonable for this. regards, tom lane
diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c index 9a9724574bec2874710316fa74643ae24d2d305e..e62286f9f98eccfd9a30e2e8f4908e8757b48672 100644 *** a/src/backend/access/transam/xact.c --- b/src/backend/access/transam/xact.c *************** GetCurrentSubTransactionId(void) *** 570,575 **** --- 570,596 ---- 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 2782b55e876078cda87bc9dac6b980340be0298d..c908f34cfe4bdd7fb0e059679d8307b2f75c8b23 100644 *** a/src/backend/executor/functions.c --- b/src/backend/executor/functions.c *************** *** 25,34 **** --- 25,36 ---- #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" *************** typedef struct execution_state *** 74,81 **** * 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. */ typedef struct { --- 76,92 ---- * 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 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 { *************** typedef struct *** 106,111 **** --- 117,128 ---- * 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; *************** static void *** 551,556 **** --- 568,575 ---- 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; *************** init_sql_fcache(FmgrInfo *finfo, Oid col *** 562,568 **** --- 581,605 ---- 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; /* *************** init_sql_fcache(FmgrInfo *finfo, Oid col *** 635,640 **** --- 672,682 ---- * 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); *************** init_sql_fcache(FmgrInfo *finfo, Oid col *** 700,706 **** --- 742,754 ---- 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 */ *************** postquel_get_single_result(TupleTableSlo *** 923,931 **** Datum fmgr_sql(PG_FUNCTION_ARGS) { - MemoryContext oldcontext; SQLFunctionCachePtr fcache; ErrorContextCallback sqlerrcontext; bool randomAccess; bool lazyEvalOK; bool is_first; --- 971,979 ---- Datum fmgr_sql(PG_FUNCTION_ARGS) { SQLFunctionCachePtr fcache; ErrorContextCallback sqlerrcontext; + MemoryContext oldcontext; bool randomAccess; bool lazyEvalOK; bool is_first; *************** fmgr_sql(PG_FUNCTION_ARGS) *** 937,949 **** 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() */ sqlerrcontext.callback = sql_exec_error_callback; --- 985,990 ---- *************** fmgr_sql(PG_FUNCTION_ARGS) *** 978,997 **** } /* ! * Initialize fcache (build plans) if first time through. */ fcache = (SQLFunctionCachePtr) fcinfo->flinfo->fn_extra; if (fcache == NULL) { init_sql_fcache(fcinfo->flinfo, PG_GET_COLLATION(), lazyEvalOK); fcache = (SQLFunctionCachePtr) fcinfo->flinfo->fn_extra; } ! eslist = fcache->func_state; /* * Find first unfinished query in function, and note whether it's the * first query. */ es = NULL; is_first = true; foreach(eslc, eslist) --- 1019,1061 ---- } /* ! * 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; } ! ! /* ! * 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 09e6a6842c2039dc2b7c99cad74a6e83a6d9e9e6..835f6acbee0e10ee51e5a2295429efd5141e3b77 100644 *** a/src/include/access/xact.h --- b/src/include/access/xact.h *************** extern TransactionId GetCurrentTransacti *** 215,220 **** --- 215,221 ---- 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);
-- Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-bugs