From b17118b2178cca1855be784e20fb297be2b3c7e4 Mon Sep 17 00:00:00 2001 From: Matheus Alcantara Date: Fri, 5 Jun 2026 10:51:53 -0300 Subject: [PATCH v2] plpython: Use funccache.c infrastructure for procedure caching PL/Python set-returning functions can crash with a use-after-free when CREATE OR REPLACE FUNCTION is executed while the SRF is mid-iteration. The crash occurs because srfstate->savedargs is allocated in proc->mcxt, which gets deleted when the procedure is invalidated, leaving a dangling pointer that PLy_function_restore_args() then dereferences. The fix is to use reference counting to prevent destroying the function state while it's still in use, similar to what PL/pgSQL has done. This commit converts PL/Python to use the funccache.c infrastructure introduced in v18. The main challenge is that PL/Python uses SFRM_ValuePerCall for SRFs, where the handler is called multiple times with use_count potentially returning to zero between calls. SQL functions face the same challenge, so this commit follows the same approach used in functions.c: maintain a per-call-site cache struct (PLyProcedureCache) in fn_extra that holds both the pointer to the long-lived PLyProcedure and the SRF execution state. The use_count is incremented when we first obtain the procedure and is decremented via a MemoryContextCallback registered on fn_mcxt, which runs even during error aborts. Cleaning up the per-call SRF state needs more care: an ExprContextCallback handles the in-query cases, since the iterator is not guaranteed to run to completion (for example a LIMIT or a rescan can abandon it early). But unlike SQL functions, whose resources are released by transaction abort, PL/Python holds Python reference counts on the iterator and saved arguments that abort will not release, and ExprContextCallbacks are not invoked during an error abort. The MemoryContextCallback on fn_mcxt therefore doubles as the backstop that releases those references when a query errors out mid-iteration. Since fn_extra is now used for PLyProcedureCache, this commit removes the SRF macros (SRF_IS_FIRSTCALL, SRF_RETURN_NEXT, etc.) and switches to direct isDone signaling via ReturnSetInfo, matching how SQL functions handle ValuePerCall mode. Author: Matheus Alcantara Reported-by: Andrzej Doros Suggested-by: Tom Lane Reviewed-by: Tom Lane Discussion: https://www.postgresql.org/message-id/19480-f1f9fdce30462fc4%40postgresql.org --- src/pl/plpython/expected/plpython_setof.out | 23 ++ src/pl/plpython/plpy_exec.c | 205 ++++++------ src/pl/plpython/plpy_exec.h | 2 +- src/pl/plpython/plpy_main.c | 75 +++-- src/pl/plpython/plpy_procedure.c | 339 +++++++++++++------- src/pl/plpython/plpy_procedure.h | 52 +-- src/pl/plpython/sql/plpython_setof.sql | 13 + src/tools/pgindent/typedefs.list | 3 +- 8 files changed, 452 insertions(+), 260 deletions(-) diff --git a/src/pl/plpython/expected/plpython_setof.out b/src/pl/plpython/expected/plpython_setof.out index c4461ac2762..68457c8a74b 100644 --- a/src/pl/plpython/expected/plpython_setof.out +++ b/src/pl/plpython/expected/plpython_setof.out @@ -228,3 +228,26 @@ SELECT * FROM get_user_records2(); willem | doe | w_doe | 3 (4 rows) +-- A set-returning function that is invalidated mid-iteration must run to +-- completion using its original definition (bug #19480). +CREATE OR REPLACE FUNCTION self_invalidating_srf(x int) RETURNS SETOF int AS $$ +for i in range(3): + if i == 1: + plpy.execute("CREATE OR REPLACE FUNCTION self_invalidating_srf(x int) " + "RETURNS SETOF int LANGUAGE plpython3u AS 'return [-1]'") + yield x + i +$$ LANGUAGE plpython3u; +SELECT self_invalidating_srf(10); -- expect 10,11,12 (original definition) + self_invalidating_srf +----------------------- + 10 + 11 + 12 +(3 rows) + +SELECT self_invalidating_srf(10); -- expect -1 (replacement now in effect) + self_invalidating_srf +----------------------- + -1 +(1 row) + diff --git a/src/pl/plpython/plpy_exec.c b/src/pl/plpython/plpy_exec.c index de0dad1f533..e3a7497a2e4 100644 --- a/src/pl/plpython/plpy_exec.c +++ b/src/pl/plpython/plpy_exec.c @@ -22,22 +22,13 @@ #include "utils/fmgrprotos.h" #include "utils/rel.h" -/* saved state for a set-returning function */ -typedef struct PLySRFState -{ - PyObject *iter; /* Python iterator producing results */ - PLySavedArgs *savedargs; /* function argument values */ - MemoryContextCallback callback; /* for releasing refcounts when done */ -} PLySRFState; - static PyObject *PLy_function_build_args(FunctionCallInfo fcinfo, PLyProcedure *proc); -static PLySavedArgs *PLy_function_save_args(PLyProcedure *proc); +static PLySavedArgs *PLy_function_save_args(MemoryContext mctx, PLyProcedure *proc); static void PLy_function_restore_args(PLyProcedure *proc, PLySavedArgs *savedargs); -static void PLy_function_drop_args(PLySavedArgs *savedargs); static void PLy_global_args_push(PLyProcedure *proc); static void PLy_global_args_pop(PLyProcedure *proc); -static void plpython_srf_cleanup_callback(void *arg); static void plpython_return_error_callback(void *arg); +static void ShutdownPLyFunction(Datum arg); static PyObject *PLy_trigger_build_args(FunctionCallInfo fcinfo, PLyProcedure *proc, HeapTuple *rv); @@ -51,14 +42,15 @@ static void PLy_abort_open_subtransactions(int save_subxact_level); /* function subhandler */ Datum -PLy_exec_function(FunctionCallInfo fcinfo, PLyProcedure *proc) +PLy_exec_function(FunctionCallInfo fcinfo, PLyProcedureCache *pcache) { + PLyProcedure *proc = pcache->proc; bool is_setof = proc->is_setof; Datum rv; PyObject *volatile plargs = NULL; PyObject *volatile plrv = NULL; - FuncCallContext *volatile funcctx = NULL; PLySRFState *volatile srfstate = NULL; + ReturnSetInfo *rsi = NULL; ErrorContextCallback plerrcontext; /* @@ -72,25 +64,42 @@ PLy_exec_function(FunctionCallInfo fcinfo, PLyProcedure *proc) { if (is_setof) { - /* First Call setup */ - if (SRF_IS_FIRSTCALL()) + rsi = (ReturnSetInfo *) fcinfo->resultinfo; + + /* + * PL/Python returns a set in ValuePerCall mode, so the handler is + * invoked once per result row. Across those calls we keep the + * iterator and saved arguments in the per-call-site cache + * (pcache->srfstate); a NULL srfstate means this is the first + * call of a new iteration, so we set that state up here. + */ + if (pcache->srfstate == NULL) { - funcctx = SRF_FIRSTCALL_INIT(); - srfstate = (PLySRFState *) - MemoryContextAllocZero(funcctx->multi_call_memory_ctx, - sizeof(PLySRFState)); - /* Immediately register cleanup callback */ - srfstate->callback.func = plpython_srf_cleanup_callback; - srfstate->callback.arg = srfstate; - MemoryContextRegisterResetCallback(funcctx->multi_call_memory_ctx, - &srfstate->callback); - funcctx->user_fctx = srfstate; + if (!rsi || !IsA(rsi, ReturnSetInfo) || + (rsi->allowedModes & SFRM_ValuePerCall) == 0) + { + ereport(ERROR, + (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), + errmsg("unsupported set function return mode"), + errdetail("PL/Python set-returning functions only support returning one value per call."))); + } + rsi->returnMode = SFRM_ValuePerCall; + + pcache->srfstate = (PLySRFState *) + MemoryContextAllocZero(pcache->fcontext, sizeof(PLySRFState)); + + /* + * Register a shutdown callback so that the iterator state is + * released if execution is abandoned before the iterator is + * exhausted. We unregister it again on normal completion. + */ + RegisterExprContextCallback(rsi->econtext, + ShutdownPLyFunction, + PointerGetDatum(pcache)); + pcache->shutdown_reg = true; } - /* Every call setup */ - funcctx = SRF_PERCALL_SETUP(); - Assert(funcctx != NULL); - srfstate = (PLySRFState *) funcctx->user_fctx; - Assert(srfstate != NULL); + + srfstate = pcache->srfstate; } if (srfstate == NULL || srfstate->iter == NULL) @@ -127,20 +136,7 @@ PLy_exec_function(FunctionCallInfo fcinfo, PLyProcedure *proc) { if (srfstate->iter == NULL) { - /* first time -- do checks and setup */ - ReturnSetInfo *rsi = (ReturnSetInfo *) fcinfo->resultinfo; - - if (!rsi || !IsA(rsi, ReturnSetInfo) || - (rsi->allowedModes & SFRM_ValuePerCall) == 0) - { - ereport(ERROR, - (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), - errmsg("unsupported set function return mode"), - errdetail("PL/Python set-returning functions only support returning one value per call."))); - } - rsi->returnMode = SFRM_ValuePerCall; - - /* Make iterator out of returned object */ + /* first time -- make iterator out of returned object */ srfstate->iter = PyObject_GetIter(plrv); Py_DECREF(plrv); @@ -177,7 +173,7 @@ PLy_exec_function(FunctionCallInfo fcinfo, PLyProcedure *proc) * this again each time in case the iterator is changing those * values. */ - srfstate->savedargs = PLy_function_save_args(proc); + srfstate->savedargs = PLy_function_save_args(pcache->fcontext, proc); } } @@ -260,21 +256,16 @@ PLy_exec_function(FunctionCallInfo fcinfo, PLyProcedure *proc) Py_XDECREF(plrv); /* - * If there was an error within a SRF, the iterator might not have - * been exhausted yet. Clear it so the next invocation of the - * function will start the iteration again. (This code is probably - * unnecessary now; plpython_srf_cleanup_callback should take care of - * cleanup. But it doesn't hurt anything to do it here.) + * If the error was thrown within a SRF, clean up its state here. This + * is the only cleanup hook that runs for an error thrown during the + * function's own execution: ShutdownPLyFunction is not called on + * abort, and the memory-context callback only fires once the + * FmgrInfo's context is torn down. Releasing the Python references + * promptly avoids leaking them if teardown is delayed, and clearing + * pcache->srfstate ensures a reused cache won't mistake this for an + * iteration still in progress. */ - if (srfstate) - { - Py_XDECREF(srfstate->iter); - srfstate->iter = NULL; - /* And drop any saved args; we won't need them */ - if (srfstate->savedargs) - PLy_function_drop_args(srfstate->savedargs); - srfstate->savedargs = NULL; - } + PLy_function_cleanup_srfstate(pcache); PG_RE_THROW(); } @@ -290,22 +281,59 @@ PLy_exec_function(FunctionCallInfo fcinfo, PLyProcedure *proc) if (srfstate) { - /* We're in a SRF, exit appropriately */ + /* We're in a SRF, signal via rsi->isDone */ if (srfstate->iter == NULL) { - /* Iterator exhausted, so we're done */ - SRF_RETURN_DONE(funcctx); + /* + * Iterator exhausted. Unregister the shutdown callback since + * we're done normally, then clean up srfstate. (srfstate->iter + * is already NULL here, so the cleanup just frees the struct.) + */ + if (pcache->shutdown_reg) + { + UnregisterExprContextCallback(rsi->econtext, + ShutdownPLyFunction, + PointerGetDatum(pcache)); + pcache->shutdown_reg = false; + } + PLy_function_cleanup_srfstate(pcache); + + rsi->isDone = ExprEndResult; + fcinfo->isnull = true; + return (Datum) 0; } - else if (fcinfo->isnull) - SRF_RETURN_NEXT_NULL(funcctx); else - SRF_RETURN_NEXT(funcctx, rv); + { + rsi->isDone = ExprMultipleResult; + return rv; + } } /* Plain function, just return the Datum value (possibly null) */ return rv; } +/* + * ExprContext shutdown callback, invoked when the expression context that ran + * a SRF is rescanned or freed at end of query. This handles in-query + * cancellation, e.g. a LIMIT that stops fetching before the iterator is + * exhausted, or a rescan of the owning plan node. + * + * NB: this is not called during an error abort (see ShutdownExprContext()). + * The companion memory-context callback RemovePLyProcedureCache() covers that + * case, since memory-context reset/delete callbacks do run during abort. + */ +static void +ShutdownPLyFunction(Datum arg) +{ + PLyProcedureCache *pcache = (PLyProcedureCache *) DatumGetPointer(arg); + + /* execUtils.c will deregister the callback after we return */ + pcache->shutdown_reg = false; + + PLy_function_cleanup_srfstate(pcache); +} + /* * trigger subhandler * @@ -536,13 +564,13 @@ PLy_function_build_args(FunctionCallInfo fcinfo, PLyProcedure *proc) * available via the proc's globals :-( ... but we're stuck with that now. */ static PLySavedArgs * -PLy_function_save_args(PLyProcedure *proc) +PLy_function_save_args(MemoryContext mctx, PLyProcedure *proc) { PLySavedArgs *result; /* saved args are always allocated in procedure's context */ result = (PLySavedArgs *) - MemoryContextAllocZero(proc->mcxt, + MemoryContextAllocZero(mctx, offsetof(PLySavedArgs, namedargs) + proc->nargs * sizeof(PyObject *)); result->nargs = proc->nargs; @@ -618,28 +646,6 @@ PLy_function_restore_args(PLyProcedure *proc, PLySavedArgs *savedargs) pfree(savedargs); } -/* - * Free a PLySavedArgs struct without restoring the values. - */ -static void -PLy_function_drop_args(PLySavedArgs *savedargs) -{ - int i; - - /* Drop references for named args */ - for (i = 0; i < savedargs->nargs; i++) - { - Py_XDECREF(savedargs->namedargs[i]); - } - - /* Drop refs to the "args" and "TD" objects, too */ - Py_XDECREF(savedargs->args); - Py_XDECREF(savedargs->td); - - /* And free the PLySavedArgs struct */ - pfree(savedargs); -} - /* * Save away any existing arguments for the given procedure, so that we can * install new values for a recursive call. This should be invoked before @@ -659,7 +665,7 @@ PLy_global_args_push(PLyProcedure *proc) PLySavedArgs *node; /* Build a struct containing current argument values */ - node = PLy_function_save_args(proc); + node = PLy_function_save_args(proc->mcxt, proc); /* * Push the saved argument values into the procedure's stack. Once we @@ -713,25 +719,6 @@ PLy_global_args_pop(PLyProcedure *proc) } } -/* - * Memory context deletion callback for cleaning up a PLySRFState. - * We need this in case execution of the SRF is terminated early, - * due to error or the caller simply not running it to completion. - */ -static void -plpython_srf_cleanup_callback(void *arg) -{ - PLySRFState *srfstate = (PLySRFState *) arg; - - /* Release refcount on the iter, if we still have one */ - Py_XDECREF(srfstate->iter); - srfstate->iter = NULL; - /* And drop any saved args; we won't need them */ - if (srfstate->savedargs) - PLy_function_drop_args(srfstate->savedargs); - srfstate->savedargs = NULL; -} - static void plpython_return_error_callback(void *arg) { diff --git a/src/pl/plpython/plpy_exec.h b/src/pl/plpython/plpy_exec.h index f35eabbd8ee..1ade1bae151 100644 --- a/src/pl/plpython/plpy_exec.h +++ b/src/pl/plpython/plpy_exec.h @@ -7,7 +7,7 @@ #include "plpy_procedure.h" -extern Datum PLy_exec_function(FunctionCallInfo fcinfo, PLyProcedure *proc); +extern Datum PLy_exec_function(FunctionCallInfo fcinfo, PLyProcedureCache *pcache); extern HeapTuple PLy_exec_trigger(FunctionCallInfo fcinfo, PLyProcedure *proc); extern void PLy_exec_event_trigger(FunctionCallInfo fcinfo, PLyProcedure *proc); diff --git a/src/pl/plpython/plpy_main.c b/src/pl/plpython/plpy_main.c index 9f07c115f80..98ec6a28a9b 100644 --- a/src/pl/plpython/plpy_main.c +++ b/src/pl/plpython/plpy_main.c @@ -103,8 +103,6 @@ _PG_init(void) Py_DECREF(main_mod); - init_procedure_caches(); - explicit_subtransactions = NIL; PLy_execution_contexts = NULL; @@ -113,9 +111,13 @@ _PG_init(void) Datum plpython3_validator(PG_FUNCTION_ARGS) { + LOCAL_FCINFO(fake_fcinfo, 0); Oid funcoid = PG_GETARG_OID(0); HeapTuple tuple; Form_pg_proc procStruct; + FmgrInfo flinfo; + TriggerData trigdata; + EventTriggerData etrigdata; PLyTrigType is_trigger; if (!CheckFunctionValidatorAccess(fcinfo->flinfo->fn_oid, funcoid)) @@ -134,8 +136,33 @@ plpython3_validator(PG_FUNCTION_ARGS) ReleaseSysCache(tuple); + /* + * Set up a fake fcinfo with just enough info to satisfy + * PLy_procedure_get(). That function derives the call context (plain + * function, DML trigger, or event trigger) from the fcinfo, so we have to + * fake up the matching context node here. + */ + MemSet(fake_fcinfo, 0, SizeForFunctionCallInfo(0)); + MemSet(&flinfo, 0, sizeof(flinfo)); + fake_fcinfo->flinfo = &flinfo; + flinfo.fn_oid = funcoid; + flinfo.fn_mcxt = CurrentMemoryContext; + + if (is_trigger == PLPY_TRIGGER) + { + MemSet(&trigdata, 0, sizeof(trigdata)); + trigdata.type = T_TriggerData; + fake_fcinfo->context = (Node *) &trigdata; + } + else if (is_trigger == PLPY_EVENT_TRIGGER) + { + MemSet(&etrigdata, 0, sizeof(etrigdata)); + etrigdata.type = T_EventTriggerData; + fake_fcinfo->context = (Node *) &etrigdata; + } + /* We can't validate triggers against any particular table ... */ - (void) PLy_procedure_get(funcoid, InvalidOid, is_trigger); + (void) PLy_procedure_get(fake_fcinfo, true); PG_RETURN_VOID(); } @@ -143,6 +170,7 @@ plpython3_validator(PG_FUNCTION_ARGS) Datum plpython3_call_handler(PG_FUNCTION_ARGS) { + PLyProcedureCache *pcache; bool nonatomic; Datum retval; PLyExecutionContext *exec_ctx; @@ -164,9 +192,6 @@ plpython3_call_handler(PG_FUNCTION_ARGS) PG_TRY(); { - Oid funcoid = fcinfo->flinfo->fn_oid; - PLyProcedure *proc; - /* * Setup error traceback support for ereport(). Note that the PG_TRY * structure pops this for us again at exit, so we needn't do that @@ -178,34 +203,35 @@ plpython3_call_handler(PG_FUNCTION_ARGS) plerrcontext.previous = error_context_stack; error_context_stack = &plerrcontext; + /* + * Look up (and if necessary compile) the procedure. This can throw + * an error, so it must happen inside the PG_TRY so that the execution + * context gets popped on the way out. + */ + pcache = PLy_procedure_get(fcinfo, false); + exec_ctx->curr_proc = pcache->proc; + if (CALLED_AS_TRIGGER(fcinfo)) { - Relation tgrel = ((TriggerData *) fcinfo->context)->tg_relation; HeapTuple trv; - proc = PLy_procedure_get(funcoid, RelationGetRelid(tgrel), PLPY_TRIGGER); - exec_ctx->curr_proc = proc; - trv = PLy_exec_trigger(fcinfo, proc); + trv = PLy_exec_trigger(fcinfo, pcache->proc); retval = PointerGetDatum(trv); } else if (CALLED_AS_EVENT_TRIGGER(fcinfo)) { - proc = PLy_procedure_get(funcoid, InvalidOid, PLPY_EVENT_TRIGGER); - exec_ctx->curr_proc = proc; - PLy_exec_event_trigger(fcinfo, proc); + PLy_exec_event_trigger(fcinfo, pcache->proc); retval = (Datum) 0; } else - { - proc = PLy_procedure_get(funcoid, InvalidOid, PLPY_NOT_TRIGGER); - exec_ctx->curr_proc = proc; - retval = PLy_exec_function(fcinfo, proc); - } + retval = PLy_exec_function(fcinfo, pcache); } PG_CATCH(); { + /* Destroy the execution context */ PLy_pop_execution_context(); PyErr_Clear(); + PG_RE_THROW(); } PG_END_TRY(); @@ -223,6 +249,7 @@ plpython3_inline_handler(PG_FUNCTION_ARGS) InlineCodeBlock *codeblock = (InlineCodeBlock *) DatumGetPointer(PG_GETARG_DATUM(0)); FmgrInfo flinfo; PLyProcedure proc; + PLyProcedureCache pcache; PLyExecutionContext *exec_ctx; ErrorContextCallback plerrcontext; @@ -248,6 +275,11 @@ plpython3_inline_handler(PG_FUNCTION_ARGS) */ proc.result.typoid = VOIDOID; + /* Set up a minimal PLyProcedureCache for the inline block */ + MemSet(&pcache, 0, sizeof(PLyProcedureCache)); + pcache.proc = &proc; + pcache.fcontext = CurrentMemoryContext; + /* * Push execution context onto stack. It is important that this get * popped again, so avoid putting anything that could throw error between @@ -269,7 +301,7 @@ plpython3_inline_handler(PG_FUNCTION_ARGS) PLy_procedure_compile(&proc, codeblock->source_text); exec_ctx->curr_proc = &proc; - PLy_exec_function(fake_fcinfo, &proc); + PLy_exec_function(fake_fcinfo, &pcache); } PG_CATCH(); { @@ -289,6 +321,11 @@ plpython3_inline_handler(PG_FUNCTION_ARGS) PG_RETURN_VOID(); } +/* + * Determine whether a function is a (DML or event) trigger from its pg_proc + * result type. This is used by the validator, which has no call context to + * inspect; the call handler instead relies on the fcinfo's call context. + */ static PLyTrigType PLy_procedure_is_trigger(Form_pg_proc procStruct) { diff --git a/src/pl/plpython/plpy_procedure.c b/src/pl/plpython/plpy_procedure.c index 750ba586e0c..2a6a5c54809 100644 --- a/src/pl/plpython/plpy_procedure.c +++ b/src/pl/plpython/plpy_procedure.c @@ -9,33 +9,32 @@ #include "access/htup_details.h" #include "catalog/pg_proc.h" #include "catalog/pg_type.h" +#include "commands/event_trigger.h" +#include "commands/trigger.h" #include "funcapi.h" #include "plpy_elog.h" #include "plpy_main.h" #include "plpy_procedure.h" #include "plpy_util.h" #include "utils/builtins.h" -#include "utils/hsearch.h" +#include "utils/funccache.h" #include "utils/memutils.h" +#include "utils/rel.h" #include "utils/syscache.h" -static HTAB *PLy_procedure_cache = NULL; - -static PLyProcedure *PLy_procedure_create(HeapTuple procTup, Oid fn_oid, PLyTrigType is_trigger); -static bool PLy_procedure_valid(PLyProcedure *proc, HeapTuple procTup); +static void PLy_procedure_create(PLyProcedure *proc, + HeapTuple procTup, + Oid fn_oid, + PLyTrigType is_trigger); static char *PLy_procedure_munge_source(const char *name, const char *src); - - -void -init_procedure_caches(void) -{ - HASHCTL hash_ctl; - - hash_ctl.keysize = sizeof(PLyProcedureKey); - hash_ctl.entrysize = sizeof(PLyProcedureEntry); - PLy_procedure_cache = hash_create("PL/Python procedures", 32, &hash_ctl, - HASH_ELEM | HASH_BLOBS); -} +static void PLy_compile_callback(FunctionCallInfo fcinfo, + HeapTuple procTup, + const CachedFunctionHashKey *hashkey, + CachedFunction *cfunc, + bool forValidator); +static void PLy_delete_callback(CachedFunction *cfunc); +static void RemovePLyProcedureCache(void *arg); +static void PLy_function_drop_args(PLySavedArgs *savedargs); /* * PLy_procedure_name: get the name of the specified procedure. @@ -51,103 +50,107 @@ PLy_procedure_name(PLyProcedure *proc) } /* - * PLy_procedure_get: returns a cached PLyProcedure, or creates, stores and - * returns a new PLyProcedure. + * PLy_procedure_get: returns a cached PLyProcedureCache for the function. * - * fn_oid is the OID of the function requested - * fn_rel is InvalidOid or the relation this function triggers on - * is_trigger denotes whether the function is a trigger function + * The PLyProcedureCache contains a pointer to the long-lived PLyProcedure + * (managed by funccache.c) and execution-specific state like SRF state. * - * The reason that both fn_rel and is_trigger need to be passed is that when - * trigger functions get validated we don't know which relation(s) they'll - * be used with, so no sensible fn_rel can be passed. Also, in that case - * we can't make a cache entry because we can't construct the right cache key. - * To forestall leakage of the PLyProcedure in such cases, delete it after - * construction and return NULL. That's okay because the only caller that - * would pass that set of values is plpython3_validator, which ignores our - * result anyway. + * For SRFs, if we are resuming execution (srfstate->iter != NULL), we skip + * revalidation and continue using the same PLyProcedure to ensure consistent + * behavior throughout the SRF execution. */ -PLyProcedure * -PLy_procedure_get(Oid fn_oid, Oid fn_rel, PLyTrigType is_trigger) +PLyProcedureCache * +PLy_procedure_get(FunctionCallInfo fcinfo, bool forValidator) { - bool use_cache; - HeapTuple procTup; - PLyProcedureKey key; - PLyProcedureEntry *volatile entry = NULL; - PLyProcedure *volatile proc = NULL; - bool found = false; - - if (is_trigger == PLPY_TRIGGER && fn_rel == InvalidOid) - use_cache = false; - else - use_cache = true; + PLyProcedure *proc; + PLyProcedureCache *pcache; + FmgrInfo *finfo = fcinfo->flinfo; + + /* + * If this is the first execution for this FmgrInfo, set up a cache struct + * (initially containing null pointers). The cache must live as long as + * the FmgrInfo, so it goes in fn_mcxt. Also set up a memory context + * callback that will be invoked when fn_mcxt is deleted. + */ + pcache = finfo->fn_extra; + if (pcache == NULL) + { + pcache = (PLyProcedureCache *) + MemoryContextAllocZero(finfo->fn_mcxt, sizeof(PLyProcedureCache)); - procTup = SearchSysCache1(PROCOID, ObjectIdGetDatum(fn_oid)); - if (!HeapTupleIsValid(procTup)) - elog(ERROR, "cache lookup failed for function %u", fn_oid); + pcache->fcontext = finfo->fn_mcxt; + pcache->mcb.func = RemovePLyProcedureCache; + pcache->mcb.arg = pcache; + + MemoryContextRegisterResetCallback(finfo->fn_mcxt, &pcache->mcb); + + finfo->fn_extra = pcache; + } /* - * Look for the function in the cache, unless we don't have the necessary - * information (e.g. during validation). In that case we just don't cache - * anything. + * If we are resuming execution of a set-returning function, just keep + * using the same cache. We do not ask funccache.c to re-validate the + * PLyProcedure: we want to run to completion using the function's initial + * definition. + * + * A live iterator (srfstate->iter != NULL) reliably means a genuine + * resume: when an iteration ends for any reason, srfstate->iter is reset + * to NULL. Normal exhaustion and in-query cancellation (LIMIT, rescan) + * go through PLy_exec_function()/ShutdownPLyFunction(); an error thrown + * during the function's own execution is handled by the PG_CATCH there; + * and an abort that tears down the FmgrInfo runs + * RemovePLyProcedureCache(). So we can't mistake leftover state from an + * interrupted SRF for a resume here. */ - if (use_cache) + if (pcache->srfstate != NULL && pcache->srfstate->iter != NULL) { - key.fn_oid = fn_oid; - key.fn_rel = fn_rel; - entry = hash_search(PLy_procedure_cache, &key, HASH_ENTER, &found); - proc = entry->proc; + Assert(pcache->proc != NULL); + return pcache; } - PG_TRY(); + /* + * Look up, or re-validate, the long-lived hash entry. + */ + proc = (PLyProcedure *) + cached_function_compile(fcinfo, + (CachedFunction *) pcache->proc, + PLy_compile_callback, + PLy_delete_callback, + sizeof(PLyProcedure), + true, + forValidator); + + /* + * Install the hash pointer in the PLyProcedureCache, and increment its + * use count to reflect that. If cached_function_compile gave us back a + * different hash entry than we were using before, we must decrement that + * one's use count. + */ + if (proc != pcache->proc) { - if (!found) + if (pcache->proc != NULL) { - /* Haven't found it, create a new procedure */ - proc = PLy_procedure_create(procTup, fn_oid, is_trigger); - if (use_cache) - entry->proc = proc; - else - { - /* Delete the proc, otherwise it's a memory leak */ - PLy_procedure_delete(proc); - proc = NULL; - } - } - else if (!PLy_procedure_valid(proc, procTup)) - { - /* Found it, but it's invalid, free and reuse the cache entry */ - entry->proc = NULL; - if (proc) - PLy_procedure_delete(proc); - proc = PLy_procedure_create(procTup, fn_oid, is_trigger); - entry->proc = proc; + Assert(pcache->proc->cfunc.use_count > 0); + pcache->proc->cfunc.use_count--; } - /* Found it and it's valid, it's fine to use it */ + pcache->proc = proc; + proc->cfunc.use_count++; } - PG_CATCH(); - { - /* Do not leave an uninitialized entry in the cache */ - if (use_cache) - hash_search(PLy_procedure_cache, &key, HASH_REMOVE, NULL); - PG_RE_THROW(); - } - PG_END_TRY(); - - ReleaseSysCache(procTup); - return proc; + return pcache; } /* * Create a new PLyProcedure structure */ -static PLyProcedure * -PLy_procedure_create(HeapTuple procTup, Oid fn_oid, PLyTrigType is_trigger) +static void +PLy_procedure_create(PLyProcedure *proc, + HeapTuple procTup, + Oid fn_oid, + PLyTrigType is_trigger) { char procName[NAMEDATALEN + 256]; Form_pg_proc procStruct; - PLyProcedure *volatile proc; MemoryContext cxt; MemoryContext oldcxt; int rv; @@ -177,7 +180,6 @@ PLy_procedure_create(HeapTuple procTup, Oid fn_oid, PLyTrigType is_trigger) oldcxt = MemoryContextSwitchTo(cxt); - proc = palloc0_object(PLyProcedure); proc->mcxt = cxt; PG_TRY(); @@ -191,8 +193,6 @@ PLy_procedure_create(HeapTuple procTup, Oid fn_oid, PLyTrigType is_trigger) proc->proname = pstrdup(NameStr(procStruct->proname)); MemoryContextSetIdentifier(cxt, proc->proname); proc->pyname = pstrdup(procName); - proc->fn_xmin = HeapTupleHeaderGetRawXmin(procTup->t_data); - proc->fn_tid = procTup->t_self; proc->fn_readonly = (procStruct->provolatile != PROVOLATILE_VOLATILE); proc->is_setof = procStruct->proretset; proc->is_procedure = (procStruct->prokind == PROKIND_PROCEDURE); @@ -355,7 +355,6 @@ PLy_procedure_create(HeapTuple procTup, Oid fn_oid, PLyTrigType is_trigger) PG_END_TRY(); MemoryContextSwitchTo(oldcxt); - return proc; } /* @@ -424,23 +423,6 @@ PLy_procedure_delete(PLyProcedure *proc) MemoryContextDelete(proc->mcxt); } -/* - * Decide whether a cached PLyProcedure struct is still valid - */ -static bool -PLy_procedure_valid(PLyProcedure *proc, HeapTuple procTup) -{ - if (proc == NULL) - return false; - - /* If the pg_proc tuple has changed, it's not valid */ - if (!(proc->fn_xmin == HeapTupleHeaderGetRawXmin(procTup->t_data) && - ItemPointerEquals(&proc->fn_tid, &procTup->t_self))) - return false; - - return true; -} - static char * PLy_procedure_munge_source(const char *name, const char *src) { @@ -485,3 +467,138 @@ PLy_procedure_munge_source(const char *name, const char *src) return mrc; } + +/* + * Compile callback for funccache.c. + * + * cached_function_compile() calls this when it needs to (re)compile the + * long-lived PLyProcedure for a function. The CachedFunction handed to us is + * pre-zeroed workspace of size sizeof(PLyProcedure); we just have to fill in + * the PL/Python-specific fields. We derive the trigger type from the call + * context, matching what plpython3_call_handler dispatches on. + */ +static void +PLy_compile_callback(FunctionCallInfo fcinfo, + HeapTuple procTup, + const CachedFunctionHashKey *hashkey, + CachedFunction *cfunc, + bool forValidator) +{ + PLyProcedure *proc = (PLyProcedure *) cfunc; + PLyTrigType is_trigger; + Oid fn_oid = fcinfo->flinfo->fn_oid; + + if (CALLED_AS_TRIGGER(fcinfo)) + is_trigger = PLPY_TRIGGER; + else if (CALLED_AS_EVENT_TRIGGER(fcinfo)) + is_trigger = PLPY_EVENT_TRIGGER; + else + is_trigger = PLPY_NOT_TRIGGER; + + PLy_procedure_create(proc, procTup, fn_oid, is_trigger); +} + +/* + * Deletion callback for funccache.c. + * + * cached_function_compile() calls this when it discards a cache entry, which + * only happens once the entry's use count has dropped to zero. We must free + * the subsidiary data but not the CachedFunction struct itself. + */ +static void +PLy_delete_callback(CachedFunction *cfunc) +{ + PLyProcedure *proc = (PLyProcedure *) cfunc; + + Assert(proc->cfunc.use_count == 0); + Assert(proc->calldepth == 0); + + PLy_procedure_delete(proc); +} + +/* + * MemoryContext callback function + * + * We register this in the memory context that contains a PLyProcedureCache + * struct (that is, the FmgrInfo's fn_mcxt). When the memory context is reset + * or deleted, we release the reference count (if any) that the cache holds on + * the long-lived hash entry. Note that this will happen even during error + * aborts. + * + * This is also our opportunity to release the Python references held by an + * interrupted set-returning function. ShutdownPLyFunction() handles that for + * normal completion and in-query cancellation, but it does not run during an + * error abort; this callback does, so it is the backstop that prevents leaking + * the SRF's iterator and saved arguments when a query errors out mid-iteration. + */ +static void +RemovePLyProcedureCache(void *arg) +{ + PLyProcedureCache *pcache = (PLyProcedureCache *) arg; + + /* Release any Python state left behind by an interrupted SRF */ + PLy_function_cleanup_srfstate(pcache); + + /* Release reference count on PLyProcedure */ + if (pcache->proc != NULL) + { + Assert(pcache->proc->cfunc.use_count > 0); + pcache->proc->cfunc.use_count--; + pcache->proc = NULL; + } +} + +/* + * Free a PLySavedArgs struct without restoring the values. + */ +static void +PLy_function_drop_args(PLySavedArgs *savedargs) +{ + int i; + + /* Drop references for named args */ + for (i = 0; i < savedargs->nargs; i++) + { + Py_XDECREF(savedargs->namedargs[i]); + } + + /* Drop refs to the "args" and "TD" objects, too */ + Py_XDECREF(savedargs->args); + Py_XDECREF(savedargs->td); + + /* And free the PLySavedArgs struct */ + pfree(savedargs); +} + +/* + * Release the Python references held by an in-progress set-returning + * function, and free the SRF state. This is a no-op if there is no active + * SRF state, so it's safe to call more than once. + * + * The Python iterator and the saved argument values own reference counts on + * Python objects, which are not released by transaction abort the way backend + * memory is. We must therefore make sure this runs on every exit path: it is + * called from the SRF paths in PLy_exec_function() and from its companion + * ShutdownPLyFunction() for normal completion and in-query cancellation, and + * from RemovePLyProcedureCache() above as the backstop for error aborts. + */ +void +PLy_function_cleanup_srfstate(PLyProcedureCache *pcache) +{ + PLySRFState *srfstate = pcache->srfstate; + + if (srfstate != NULL) + { + /* Release refcount on the iter, if we still have one */ + Py_XDECREF(srfstate->iter); + srfstate->iter = NULL; + + /* And drop any saved args; we won't need them */ + if (srfstate->savedargs) + PLy_function_drop_args(srfstate->savedargs); + srfstate->savedargs = NULL; + + pfree(srfstate); + pcache->srfstate = NULL; + } +} diff --git a/src/pl/plpython/plpy_procedure.h b/src/pl/plpython/plpy_procedure.h index 3ef22844a9b..6cd61a981fa 100644 --- a/src/pl/plpython/plpy_procedure.h +++ b/src/pl/plpython/plpy_procedure.h @@ -6,9 +6,7 @@ #define PLPY_PROCEDURE_H #include "plpy_typeio.h" - - -extern void init_procedure_caches(void); +#include "utils/funccache.h" /* @@ -31,15 +29,28 @@ typedef struct PLySavedArgs PyObject *namedargs[FLEXIBLE_ARRAY_MEMBER]; /* named args */ } PLySavedArgs; -/* cached procedure data */ +/* saved state for a set-returning function */ +typedef struct PLySRFState +{ + PyObject *iter; /* Python iterator producing results */ + PLySavedArgs *savedargs; /* function argument values */ +} PLySRFState; + +/* + * Long-lived data for a PL/Python function. + * + * This struct is managed by funccache.c and can be shared across multiple + * executions of the same function. It must contain no execution-specific + * state. The CachedFunction struct must be first so we can cast between them. + */ typedef struct PLyProcedure { + CachedFunction cfunc; /* fields managed by funccache.c */ + MemoryContext mcxt; /* context holding this PLyProcedure and its * subsidiary data */ char *proname; /* SQL name of procedure */ char *pyname; /* Python name of procedure */ - TransactionId fn_xmin; - ItemPointerData fn_tid; bool fn_readonly; bool is_setof; /* true, if function returns result set */ bool is_procedure; @@ -59,24 +70,29 @@ typedef struct PLyProcedure PLySavedArgs *argstack; /* stack of outer-level call arguments */ } PLyProcedure; -/* the procedure cache key */ -typedef struct PLyProcedureKey +/* + * Per-call-site cache for a PL/Python function. + * + * This struct is stored in fn_extra and holds execution-specific state, + * including a pointer to the long-lived PLyProcedure. The use_count in + * the PLyProcedure is incremented while we hold a reference. + */ +typedef struct PLyProcedureCache { - Oid fn_oid; /* function OID */ - Oid fn_rel; /* triggered-on relation or InvalidOid */ -} PLyProcedureKey; + PLyProcedure *proc; /* long-lived hash entry */ + MemoryContext fcontext; /* fn_mcxt - context holding this struct */ + PLySRFState *srfstate; /* SRF execution state, NULL if not in SRF */ + bool shutdown_reg; /* true if registered shutdown callback */ -/* the procedure cache entry */ -typedef struct PLyProcedureEntry -{ - PLyProcedureKey key; /* hash key */ - PLyProcedure *proc; -} PLyProcedureEntry; + /* Callback to release use-count when fcontext is deleted */ + MemoryContextCallback mcb; +} PLyProcedureCache; /* PLyProcedure manipulation */ extern char *PLy_procedure_name(PLyProcedure *proc); -extern PLyProcedure *PLy_procedure_get(Oid fn_oid, Oid fn_rel, PLyTrigType is_trigger); +extern PLyProcedureCache *PLy_procedure_get(FunctionCallInfo fcinfo, bool forValidator); extern void PLy_procedure_compile(PLyProcedure *proc, const char *src); extern void PLy_procedure_delete(PLyProcedure *proc); +extern void PLy_function_cleanup_srfstate(PLyProcedureCache *pcache); #endif /* PLPY_PROCEDURE_H */ diff --git a/src/pl/plpython/sql/plpython_setof.sql b/src/pl/plpython/sql/plpython_setof.sql index 1a0472b7a3b..af73155a713 100644 --- a/src/pl/plpython/sql/plpython_setof.sql +++ b/src/pl/plpython/sql/plpython_setof.sql @@ -107,3 +107,16 @@ $$ LANGUAGE plpython3u; SELECT get_user_records2(); SELECT * FROM get_user_records2(); + +-- A set-returning function that is invalidated mid-iteration must run to +-- completion using its original definition (bug #19480). +CREATE OR REPLACE FUNCTION self_invalidating_srf(x int) RETURNS SETOF int AS $$ +for i in range(3): + if i == 1: + plpy.execute("CREATE OR REPLACE FUNCTION self_invalidating_srf(x int) " + "RETURNS SETOF int LANGUAGE plpython3u AS 'return [-1]'") + yield x + i +$$ LANGUAGE plpython3u; + +SELECT self_invalidating_srf(10); -- expect 10,11,12 (original definition) +SELECT self_invalidating_srf(10); -- expect -1 (replacement now in effect) diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list index f9eb23e52c9..6975aaa25c5 100644 --- a/src/tools/pgindent/typedefs.list +++ b/src/tools/pgindent/typedefs.list @@ -2075,8 +2075,7 @@ PLyObToTuple PLyObject_AsString_t PLyPlanObject PLyProcedure -PLyProcedureEntry -PLyProcedureKey +PLyProcedureCache PLyResultObject PLySRFState PLySavedArgs -- 2.50.1 (Apple Git-155)