Thread: Performance issues with v18 SQL-language-function changes
In the thread that led up to commit 0dca5d68d [1], we'd convinced
ourselves that the new implementation was faster than the old.
So I was sad to discover that there are common cases where it's
a good bit slower. We'd focused too much on test methods like
do $$
begin
for i in 1..10000000 loop
perform fx((random()*100)::int);
end loop;
end;
$$;
The thing about this test case is that the SQL function under
test is executed only once per calling query (i.e., per PERFORM).
That doesn't allow the old implementation to amortize anything.
If you instead test cases like
create function fx(p_summa bigint) returns text immutable strict
return ltrim(to_char(p_summa, '999 999 999 999 999 999 999 999'));
explain analyze select fx(i) from generate_series(1,1000000) as i(i);
you arrive at the rude discovery that 0dca5d68d is about 50% slower
than 0dca5d68d^, because the old implementation builds a plan for fx()
only once and then re-uses it throughout the query. So does the new
implementation, but it has added GetCachedPlan overhead. Moreover,
I made the unforced error of deciding that we could tear down and
rebuild the SQLFunctionCache and subsidiary data structures for
each call. That overhead is relatively minor in comparison to the
cost of parsing and planning the function; but when comparing cases
where there's no repeated parsing and planning, it becomes
significant.
Hence, the attached patch series reverts that decision and goes back
to the old method of having the SQLFunctionCache and some associated
objects live as long as the FmgrInfo does (without, however, the
poorly-controlled memory leaks of the old code). In my testing
this gets us from a 50% penalty down to about 5%, which I think is
acceptable given the other benefits 0dca5d68d brought us.
I'm inclined to argue that, seeing that 0dca5d68d was mainly intended
as a performance feature, this performance loss is a bug that we
need to do something about even though we're post-feature-freeze.
We could either revert 0dca5d68d or apply the attached. I'd prefer
the latter.
(There are other things we could do to try to reduce the overhead
further, such as trying to not build a Tuplestore or JunkFilter in
simple cases. But that seems like new work not a fix for a bad
decision in existing work, so I think it's out of scope for v18.)
Comments?
regards, tom lane
[1] https://www.postgresql.org/message-id/flat/8216639.NyiUUSuA9g%40aivenlaptop
From c87a37d00b847e86e5286c9c46668410019b0043 Mon Sep 17 00:00:00 2001
From: Tom Lane <tgl@sss.pgh.pa.us>
Date: Sun, 13 Apr 2025 13:37:46 -0400
Subject: [PATCH v1 1/5] Minor performance improvement for SQL-language
functions.
Late in the development of commit 0dca5d68d, I added a step to copy
the result tlist we extract from the cached final query, because
I was afraid that that might not last as long as the JunkFilter that
we're passing it off to. However, that turns out to cost a noticeable
number of cycles, and it's really quite unnecessary because the
JunkFilter will not examine that tlist after it's been created.
(ExecFindJunkAttribute would use it, but we don't use that function
on this JunkFilter.) Hence, remove the copy step. For safety,
reset the might-become-dangling jf_targetList pointer to NIL.
In passing, remove DR_sqlfunction.cxt, which we don't use anymore;
it's confusing because it's not entirely clear which context it
ought to point at.
---
src/backend/executor/functions.c | 19 +++++++++++--------
1 file changed, 11 insertions(+), 8 deletions(-)
diff --git a/src/backend/executor/functions.c b/src/backend/executor/functions.c
index 53ff614d87b..d3f05c7d2c7 100644
--- a/src/backend/executor/functions.c
+++ b/src/backend/executor/functions.c
@@ -45,7 +45,6 @@ typedef struct
{
DestReceiver pub; /* publicly-known function pointers */
Tuplestorestate *tstore; /* where to put result tuples */
- MemoryContext cxt; /* context containing tstore */
JunkFilter *filter; /* filter to convert tuple type */
} DR_sqlfunction;
@@ -787,12 +786,6 @@ init_execution_state(SQLFunctionCachePtr fcache)
*/
resulttlist = get_sql_fn_result_tlist(plansource->query_list);
- /*
- * We need to make a copy to ensure that it doesn't disappear
- * underneath us due to plancache invalidation.
- */
- resulttlist = copyObject(resulttlist);
-
/*
* If the result is composite, *and* we are returning the whole tuple
* result, we need to insert nulls for any dropped columns. In the
@@ -807,6 +800,17 @@ init_execution_state(SQLFunctionCachePtr fcache)
slot);
else
fcache->junkFilter = ExecInitJunkFilter(resulttlist, slot);
+
+ /*
+ * The resulttlist tree belongs to the plancache and might disappear
+ * underneath us due to plancache invalidation. While we could
+ * forestall that by copying it, that'd just be a waste of cycles,
+ * because the junkfilter doesn't need it anymore. (It'd only be used
+ * by ExecFindJunkAttribute(), which we don't use here.) To ensure
+ * there's not a dangling pointer laying about, clear the junkFilter's
+ * pointer.
+ */
+ fcache->junkFilter->jf_targetList = NIL;
}
if (fcache->func->returnsTuple)
@@ -1245,7 +1249,6 @@ postquel_start(execution_state *es, SQLFunctionCachePtr fcache)
myState = (DR_sqlfunction *) dest;
Assert(myState->pub.mydest == DestSQLFunction);
myState->tstore = fcache->tstore;
- myState->cxt = CurrentMemoryContext;
myState->filter = fcache->junkFilter;
}
else
--
2.43.5
From 7d427b796978a45ca668aede83e0705a81df7f3f Mon Sep 17 00:00:00 2001
From: Tom Lane <tgl@sss.pgh.pa.us>
Date: Sun, 13 Apr 2025 13:57:16 -0400
Subject: [PATCH v1 2/5] Make functions.c mostly run in a short-lived memory
context.
Previously, much of this code ran with CurrentMemoryContext set
to be the function's fcontext, so that we tended to leak a lot of
stuff there. Commit 0dca5d68d dealt with that by releasing the
fcontext at the completion of each SQL function call, but we'd
like to go back to the previous approach of allowing the fcontext
to be query-lifespan. To control the leakage problem, rearrange
the code so that we mostly run in the memory context that fmgr_sql
is called in (which we expect to be short-lived). Notably, this
means that parsing/planning is all done in the short-lived context
and doesn't leak cruft into fcontext.
This patch also fixes the allocation of execution_state records
so that we don't leak them across executions. I set that up
with a re-usable array that contains at least as many
execution_state structs as we need for the current querytree.
The chain structure is still there, but it's not really doing
much for us, and maybe somebody will be motivated to get rid
of it. I'm not though.
This incidentally also moves the call of BlessTupleDesc to be
with the code that creates the JunkFilter. That doesn't make
much difference now, but a later patch will reduce the number
of times the JunkFilter gets made, and we needn't bless the
results any more often than that.
We still leak a fair amount in fcontext, particularly when
executing utility statements, but that's material for a
separate patch step; the point here is only to get rid of
unintentional allocations in fcontext.
---
src/backend/executor/functions.c | 153 +++++++++++++++++++------------
1 file changed, 95 insertions(+), 58 deletions(-)
diff --git a/src/backend/executor/functions.c b/src/backend/executor/functions.c
index d3f05c7d2c7..b264060d33d 100644
--- a/src/backend/executor/functions.c
+++ b/src/backend/executor/functions.c
@@ -55,7 +55,9 @@ typedef struct
*
* The "next" fields chain together all the execution_state records generated
* from a single original parsetree. (There will only be more than one in
- * case of rule expansion of the original parsetree.)
+ * case of rule expansion of the original parsetree.) The chain structure is
+ * quite vestigial at this point, because we allocate the records in an array
+ * for ease of memory management. But we'll get rid of it some other day.
*/
typedef enum
{
@@ -158,17 +160,20 @@ typedef struct SQLFunctionCache
/*
* While executing a particular query within the function, cplan is the
- * CachedPlan we've obtained for that query, and eslist is a list of
+ * CachedPlan we've obtained for that query, and eslist is a chain of
* execution_state records for the individual plans within the CachedPlan.
* next_query_index is the 0-based index of the next CachedPlanSource to
* get a CachedPlan from.
*/
CachedPlan *cplan; /* Plan for current query, if any */
ResourceOwner cowner; /* CachedPlan is registered with this owner */
- execution_state *eslist; /* execution_state records */
int next_query_index; /* index of next CachedPlanSource to run */
- /* if positive, this is the index of the query we're processing */
+ execution_state *eslist; /* chain of execution_state records */
+ execution_state *esarray; /* storage for eslist */
+ int esarray_len; /* allocated length of esarray[] */
+
+ /* if positive, this is the 1-based index of the query we're processing */
int error_query_index;
MemoryContext fcontext; /* memory context holding this struct and all
@@ -212,13 +217,12 @@ static void sql_delete_callback(CachedFunction *cfunc);
static void sql_postrewrite_callback(List *querytree_list, void *arg);
static void postquel_start(execution_state *es, SQLFunctionCachePtr fcache);
static bool postquel_getnext(execution_state *es, SQLFunctionCachePtr fcache);
-static void postquel_end(execution_state *es);
+static void postquel_end(execution_state *es, SQLFunctionCachePtr fcache);
static void postquel_sub_params(SQLFunctionCachePtr fcache,
FunctionCallInfo fcinfo);
static Datum postquel_get_single_result(TupleTableSlot *slot,
FunctionCallInfo fcinfo,
- SQLFunctionCachePtr fcache,
- MemoryContext resultcontext);
+ SQLFunctionCachePtr fcache);
static void sql_compile_error_callback(void *arg);
static void sql_exec_error_callback(void *arg);
static void ShutdownSQLFunction(Datum arg);
@@ -658,12 +662,11 @@ init_execution_state(SQLFunctionCachePtr fcache)
CachedPlanSource *plansource;
execution_state *preves = NULL;
execution_state *lasttages = NULL;
+ int nstmts;
ListCell *lc;
/*
- * Clean up after previous query, if there was one. Note that we just
- * leak the old execution_state records until end of function execution;
- * there aren't likely to be enough of them to matter.
+ * Clean up after previous query, if there was one.
*/
if (fcache->cplan)
{
@@ -704,6 +707,22 @@ init_execution_state(SQLFunctionCachePtr fcache)
fcache->cowner,
NULL);
+ /*
+ * If necessary, make esarray[] bigger to hold the needed state.
+ */
+ nstmts = list_length(fcache->cplan->stmt_list);
+ if (nstmts > fcache->esarray_len)
+ {
+ if (fcache->esarray == NULL)
+ fcache->esarray = (execution_state *)
+ MemoryContextAlloc(fcache->fcontext,
+ sizeof(execution_state) * nstmts);
+ else
+ fcache->esarray = repalloc_array(fcache->esarray,
+ execution_state, nstmts);
+ fcache->esarray_len = nstmts;
+ }
+
/*
* Build execution_state list to match the number of contained plans.
*/
@@ -740,7 +759,7 @@ init_execution_state(SQLFunctionCachePtr fcache)
CreateCommandName((Node *) stmt))));
/* OK, build the execution_state for this query */
- newes = (execution_state *) palloc(sizeof(execution_state));
+ newes = &fcache->esarray[foreach_current_index(lc)];
if (preves)
preves->next = newes;
else
@@ -775,9 +794,14 @@ init_execution_state(SQLFunctionCachePtr fcache)
*/
if (fcache->func->rettype != VOIDOID)
{
- TupleTableSlot *slot = MakeSingleTupleTableSlot(NULL,
- &TTSOpsMinimalTuple);
+ TupleTableSlot *slot;
List *resulttlist;
+ MemoryContext oldcontext;
+
+ /* The result slot and JunkFilter must be in the fcontext */
+ oldcontext = MemoryContextSwitchTo(fcache->fcontext);
+
+ slot = MakeSingleTupleTableSlot(NULL, &TTSOpsMinimalTuple);
/*
* Re-fetch the (possibly modified) output tlist of the final
@@ -811,14 +835,17 @@ init_execution_state(SQLFunctionCachePtr fcache)
* pointer.
*/
fcache->junkFilter->jf_targetList = NIL;
- }
- if (fcache->func->returnsTuple)
- {
/* Make sure output rowtype is properly blessed */
- BlessTupleDesc(fcache->junkFilter->jf_resultSlot->tts_tupleDescriptor);
+ if (fcache->func->returnsTuple)
+ BlessTupleDesc(fcache->junkFilter->jf_resultSlot->tts_tupleDescriptor);
+
+ MemoryContextSwitchTo(oldcontext);
}
- else if (fcache->func->returnsSet && type_is_rowtype(fcache->func->rettype))
+
+ if (fcache->func->returnsSet &&
+ !fcache->func->returnsTuple &&
+ type_is_rowtype(fcache->func->rettype))
{
/*
* Returning rowtype as if it were scalar --- materialize won't work.
@@ -1230,12 +1257,23 @@ static void
postquel_start(execution_state *es, SQLFunctionCachePtr fcache)
{
DestReceiver *dest;
+ MemoryContext oldcontext;
Assert(es->qd == NULL);
/* Caller should have ensured a suitable snapshot is active */
Assert(ActiveSnapshotSet());
+ /*
+ * Run the sub-executor in a child of fcontext. The sub-executor is
+ * responsible for deleting per-tuple information. (XXX in the case of a
+ * long-lived FmgrInfo, this policy potentially causes memory leakage, but
+ * it's not very clear where we could keep stuff instead. Fortunately,
+ * there are few if any cases where set-returning functions are invoked
+ * via FmgrInfos that would outlive the calling query.)
+ */
+ oldcontext = MemoryContextSwitchTo(fcache->fcontext);
+
/*
* If this query produces the function result, send its output to the
* tuplestore; else discard any output.
@@ -1285,6 +1323,8 @@ postquel_start(execution_state *es, SQLFunctionCachePtr fcache)
}
es->status = F_EXEC_RUN;
+
+ MemoryContextSwitchTo(oldcontext);
}
/* Run one execution_state; either to completion or to first result row */
@@ -1293,6 +1333,10 @@ static bool
postquel_getnext(execution_state *es, SQLFunctionCachePtr fcache)
{
bool result;
+ MemoryContext oldcontext;
+
+ /* Run the sub-executor in a child of fcontext */
+ oldcontext = MemoryContextSwitchTo(fcache->fcontext);
if (es->qd->operation == CMD_UTILITY)
{
@@ -1320,13 +1364,20 @@ postquel_getnext(execution_state *es, SQLFunctionCachePtr fcache)
result = (count == 0 || es->qd->estate->es_processed == 0);
}
+ MemoryContextSwitchTo(oldcontext);
+
return result;
}
/* Shut down execution of one execution_state node */
static void
-postquel_end(execution_state *es)
+postquel_end(execution_state *es, SQLFunctionCachePtr fcache)
{
+ MemoryContext oldcontext;
+
+ /* Run the sub-executor in a child of fcontext */
+ oldcontext = MemoryContextSwitchTo(fcache->fcontext);
+
/* mark status done to ensure we don't do ExecutorEnd twice */
es->status = F_EXEC_DONE;
@@ -1341,9 +1392,16 @@ postquel_end(execution_state *es)
FreeQueryDesc(es->qd);
es->qd = NULL;
+
+ MemoryContextSwitchTo(oldcontext);
}
-/* Build ParamListInfo array representing current arguments */
+/*
+ * Build ParamListInfo array representing current arguments.
+ *
+ * This must be called in the fcontext so that the results have adequate
+ * lifespan.
+ */
static void
postquel_sub_params(SQLFunctionCachePtr fcache,
FunctionCallInfo fcinfo)
@@ -1398,25 +1456,22 @@ postquel_sub_params(SQLFunctionCachePtr fcache,
/*
* Extract the SQL function's value from a single result row. This is used
* both for scalar (non-set) functions and for each row of a lazy-eval set
- * result.
+ * result. We expect the current memory context to be that of the caller
+ * of fmgr_sql.
*/
static Datum
postquel_get_single_result(TupleTableSlot *slot,
FunctionCallInfo fcinfo,
- SQLFunctionCachePtr fcache,
- MemoryContext resultcontext)
+ SQLFunctionCachePtr fcache)
{
Datum value;
- MemoryContext oldcontext;
/*
* Set up to return the function value. For pass-by-reference datatypes,
- * be sure to allocate the result in resultcontext, not the current memory
- * context (which has query lifespan). We can't leave the data in the
- * TupleTableSlot because we intend to clear the slot before returning.
+ * be sure to copy the result into the current context. We can't leave
+ * the data in the TupleTableSlot because we intend to clear the slot
+ * before returning.
*/
- oldcontext = MemoryContextSwitchTo(resultcontext);
-
if (fcache->func->returnsTuple)
{
/* We must return the whole tuple as a Datum. */
@@ -1427,7 +1482,7 @@ postquel_get_single_result(TupleTableSlot *slot,
{
/*
* Returning a scalar, which we have to extract from the first column
- * of the SELECT result, and then copy into result context if needed.
+ * of the SELECT result, and then copy into current context if needed.
*/
value = slot_getattr(slot, 1, &(fcinfo->isnull));
@@ -1435,8 +1490,6 @@ postquel_get_single_result(TupleTableSlot *slot,
value = datumCopy(value, fcache->func->typbyval, fcache->func->typlen);
}
- MemoryContextSwitchTo(oldcontext);
-
return value;
}
@@ -1450,7 +1503,6 @@ fmgr_sql(PG_FUNCTION_ARGS)
SQLFunctionLink *flink;
ErrorContextCallback sqlerrcontext;
MemoryContext tscontext;
- MemoryContext oldcontext;
bool randomAccess;
bool lazyEvalOK;
bool pushed_snapshot;
@@ -1507,20 +1559,14 @@ fmgr_sql(PG_FUNCTION_ARGS)
* Build tuplestore to hold results, if we don't have one already. Make
* sure it's in a suitable context.
*/
- oldcontext = MemoryContextSwitchTo(tscontext);
-
if (!fcache->tstore)
- fcache->tstore = tuplestore_begin_heap(randomAccess, false, work_mem);
+ {
+ MemoryContext oldcontext;
- /*
- * Switch to context in which the fcache lives. The sub-executor is
- * responsible for deleting per-tuple information. (XXX in the case of a
- * long-lived FmgrInfo, this policy potentially causes memory leakage, but
- * it's not very clear where we could keep stuff instead. Fortunately,
- * there are few if any cases where set-returning functions are invoked
- * via FmgrInfos that would outlive the calling query.)
- */
- MemoryContextSwitchTo(fcache->fcontext);
+ oldcontext = MemoryContextSwitchTo(tscontext);
+ fcache->tstore = tuplestore_begin_heap(randomAccess, false, work_mem);
+ MemoryContextSwitchTo(oldcontext);
+ }
/*
* Find first unfinished execution_state. If none, advance to the next
@@ -1598,7 +1644,7 @@ fmgr_sql(PG_FUNCTION_ARGS)
* don't care about fetching any more result rows.
*/
if (completed || !fcache->func->returnsSet)
- postquel_end(es);
+ postquel_end(es, fcache);
/*
* Break from loop if we didn't shut down (implying we got a
@@ -1657,8 +1703,7 @@ fmgr_sql(PG_FUNCTION_ARGS)
if (!tuplestore_gettupleslot(fcache->tstore, true, false, slot))
elog(ERROR, "failed to fetch lazy-eval tuple");
/* Extract the result as a datum, and copy out from the slot */
- result = postquel_get_single_result(slot, fcinfo,
- fcache, oldcontext);
+ result = postquel_get_single_result(slot, fcinfo, fcache);
/* Clear the tuplestore, but keep it for next time */
/* NB: this might delete the slot's content, but we don't care */
tuplestore_clear(fcache->tstore);
@@ -1716,12 +1761,7 @@ fmgr_sql(PG_FUNCTION_ARGS)
fcache->tstore = NULL;
/* must copy desc because execSRF.c will free it */
if (fcache->junkFilter)
- {
- /* setDesc must be allocated in suitable context */
- MemoryContextSwitchTo(tscontext);
rsi->setDesc = CreateTupleDescCopy(fcache->junkFilter->jf_cleanTupType);
- MemoryContextSwitchTo(fcache->fcontext);
- }
fcinfo->isnull = true;
result = (Datum) 0;
@@ -1746,8 +1786,7 @@ fmgr_sql(PG_FUNCTION_ARGS)
/* Re-use the junkfilter's output slot to fetch back the tuple */
slot = fcache->junkFilter->jf_resultSlot;
if (tuplestore_gettupleslot(fcache->tstore, true, false, slot))
- result = postquel_get_single_result(slot, fcinfo,
- fcache, oldcontext);
+ result = postquel_get_single_result(slot, fcinfo, fcache);
else
{
fcinfo->isnull = true;
@@ -1770,8 +1809,6 @@ fmgr_sql(PG_FUNCTION_ARGS)
if (pushed_snapshot)
PopActiveSnapshot();
- MemoryContextSwitchTo(oldcontext);
-
/*
* If we've gone through every command in the function, we are done.
* Release the cache to start over again on next call.
@@ -1889,7 +1926,7 @@ ShutdownSQLFunction(Datum arg)
if (!fcache->func->readonly_func)
PushActiveSnapshot(es->qd->snapshot);
- postquel_end(es);
+ postquel_end(es, fcache);
if (!fcache->func->readonly_func)
PopActiveSnapshot();
--
2.43.5
From 68fe63f11e10a6ececd1056747b6a7c8d6969c7b Mon Sep 17 00:00:00 2001
From: Tom Lane <tgl@sss.pgh.pa.us>
Date: Sun, 13 Apr 2025 13:59:19 -0400
Subject: [PATCH v1 3/5] Split some storage out to separate subcontexts of
fcontext.
Put the JunkFilter and its result slot (and thence also
some subsidiary data such as the result tupledesc) into a
separate subcontext "jfcontext". This doesn't accomplish
a lot at this point, because we make a new JunkFilter each
time through the SQL function. However, the plan is to make
the fcontext live longer, and that raises the possibility
that we'll need a new JunkFilter because the plan for the
result-generating query changes. A separate context makes
it easy to free the obsoleted data when that happens.
Also, instead of always running the sub-executor in fcontext,
make a separate context for it if we're doing lazy eval of
a SRF, and otherwise just run it inside CurrentMemoryContext.
The combination of these steps reduces the expected size of
fcontext enough that we can use ALLOCSET_SMALL_SIZES.
---
src/backend/executor/functions.c | 82 ++++++++++++++++++++++++++------
1 file changed, 67 insertions(+), 15 deletions(-)
diff --git a/src/backend/executor/functions.c b/src/backend/executor/functions.c
index b264060d33d..12121ad74e7 100644
--- a/src/backend/executor/functions.c
+++ b/src/backend/executor/functions.c
@@ -107,6 +107,8 @@ typedef struct execution_state
* which we free at completion. In non-returnsSet mode, this is just a child
* of the call-time context. In returnsSet mode, it is made a child of the
* FmgrInfo's fn_mcxt so that it will survive between fmgr_sql calls.
+ * The fcontext may have subsidiary contexts jfcontext and/or subcontext,
+ * which have somewhat shorter lifespans.
*
* 3. SQLFunctionLink is a tiny struct that just holds pointers to
* the SQLFunctionHashEntry and the current SQLFunctionCache (if any).
@@ -151,6 +153,7 @@ typedef struct SQLFunctionCache
bool lazyEvalOK; /* true if lazyEval is safe */
bool shutdown_reg; /* true if registered shutdown callback */
bool lazyEval; /* true if using lazyEval for result query */
+ bool ownSubcontext; /* is subcontext really a separate context? */
ParamListInfo paramLI; /* Param list representing current args */
@@ -178,6 +181,9 @@ typedef struct SQLFunctionCache
MemoryContext fcontext; /* memory context holding this struct and all
* subsidiary data */
+ MemoryContext jfcontext; /* subsidiary memory context holding
+ * junkFilter, result slot, and related data */
+ MemoryContext subcontext; /* subsidiary memory context for sub-executor */
} SQLFunctionCache;
typedef SQLFunctionCache *SQLFunctionCachePtr;
@@ -617,8 +623,8 @@ init_sql_fcache(FunctionCallInfo fcinfo, bool lazyEvalOK)
*/
pcontext = func->returnsSet ? finfo->fn_mcxt : CurrentMemoryContext;
fcontext = AllocSetContextCreate(pcontext,
- "SQL function execution",
- ALLOCSET_DEFAULT_SIZES);
+ "SQL function cache",
+ ALLOCSET_SMALL_SIZES);
oldcontext = MemoryContextSwitchTo(fcontext);
@@ -791,6 +797,9 @@ init_execution_state(SQLFunctionCachePtr fcache)
* nothing to coerce to. (XXX Frequently, the JunkFilter isn't doing
* anything very interesting, but much of this module expects it to be
* there anyway.)
+ *
+ * The JunkFilter, its result slot, and its tupledesc are kept in a
+ * subsidiary memory context so that we can free them easily when needed.
*/
if (fcache->func->rettype != VOIDOID)
{
@@ -798,8 +807,14 @@ init_execution_state(SQLFunctionCachePtr fcache)
List *resulttlist;
MemoryContext oldcontext;
- /* The result slot and JunkFilter must be in the fcontext */
- oldcontext = MemoryContextSwitchTo(fcache->fcontext);
+ /* Create or reset the jfcontext */
+ if (fcache->jfcontext == NULL)
+ fcache->jfcontext = AllocSetContextCreate(fcache->fcontext,
+ "SQL function junkfilter",
+ ALLOCSET_SMALL_SIZES);
+ else
+ MemoryContextReset(fcache->jfcontext);
+ oldcontext = MemoryContextSwitchTo(fcache->jfcontext);
slot = MakeSingleTupleTableSlot(NULL, &TTSOpsMinimalTuple);
@@ -1265,14 +1280,46 @@ postquel_start(execution_state *es, SQLFunctionCachePtr fcache)
Assert(ActiveSnapshotSet());
/*
- * Run the sub-executor in a child of fcontext. The sub-executor is
- * responsible for deleting per-tuple information. (XXX in the case of a
- * long-lived FmgrInfo, this policy potentially causes memory leakage, but
- * it's not very clear where we could keep stuff instead. Fortunately,
- * there are few if any cases where set-returning functions are invoked
- * via FmgrInfos that would outlive the calling query.)
+ * In lazyEval mode for a SRF, we must run the sub-executor in a child of
+ * fcontext, so that it can survive across multiple calls to fmgr_sql.
+ * (XXX in the case of a long-lived FmgrInfo, this policy potentially
+ * causes memory leakage, but it's not very clear where we could keep
+ * stuff instead. Fortunately, there are few if any cases where
+ * set-returning functions are invoked via FmgrInfos that would outlive
+ * the calling query.) Otherwise, we're going to run it to completion
+ * before exiting fmgr_sql, so it can perfectly well run in the caller's
+ * context.
*/
- oldcontext = MemoryContextSwitchTo(fcache->fcontext);
+ if (es->lazyEval && fcache->func->returnsSet)
+ {
+ fcache->subcontext = AllocSetContextCreate(fcache->fcontext,
+ "SQL function execution",
+ ALLOCSET_DEFAULT_SIZES);
+ fcache->ownSubcontext = true;
+ }
+ else if (es->stmt->commandType == CMD_UTILITY)
+ {
+ /*
+ * The code path using a sub-executor is pretty good about cleaning up
+ * cruft, since the executor will make its own sub-context. We don't
+ * really need an additional layer of sub-context in that case.
+ * However, if this is a utility statement, it won't make its own
+ * sub-context, so it seems advisable to make one that we can free on
+ * completion.
+ */
+ fcache->subcontext = AllocSetContextCreate(CurrentMemoryContext,
+ "SQL function execution",
+ ALLOCSET_DEFAULT_SIZES);
+ fcache->ownSubcontext = true;
+ }
+ else
+ {
+ fcache->subcontext = CurrentMemoryContext;
+ fcache->ownSubcontext = false;
+ }
+
+ /* Switch into the selected subcontext (might be a no-op) */
+ oldcontext = MemoryContextSwitchTo(fcache->subcontext);
/*
* If this query produces the function result, send its output to the
@@ -1335,8 +1382,8 @@ postquel_getnext(execution_state *es, SQLFunctionCachePtr fcache)
bool result;
MemoryContext oldcontext;
- /* Run the sub-executor in a child of fcontext */
- oldcontext = MemoryContextSwitchTo(fcache->fcontext);
+ /* Run the sub-executor in subcontext */
+ oldcontext = MemoryContextSwitchTo(fcache->subcontext);
if (es->qd->operation == CMD_UTILITY)
{
@@ -1375,8 +1422,8 @@ postquel_end(execution_state *es, SQLFunctionCachePtr fcache)
{
MemoryContext oldcontext;
- /* Run the sub-executor in a child of fcontext */
- oldcontext = MemoryContextSwitchTo(fcache->fcontext);
+ /* Run the sub-executor in subcontext */
+ oldcontext = MemoryContextSwitchTo(fcache->subcontext);
/* mark status done to ensure we don't do ExecutorEnd twice */
es->status = F_EXEC_DONE;
@@ -1394,6 +1441,11 @@ postquel_end(execution_state *es, SQLFunctionCachePtr fcache)
es->qd = NULL;
MemoryContextSwitchTo(oldcontext);
+
+ /* Delete the subcontext, if it's actually a separate context */
+ if (fcache->ownSubcontext)
+ MemoryContextDelete(fcache->subcontext);
+ fcache->subcontext = NULL;
}
/*
--
2.43.5
From 55fff2b9ec8358897dd777b315fae15ecc0d7dbe Mon Sep 17 00:00:00 2001
From: Tom Lane <tgl@sss.pgh.pa.us>
Date: Sun, 13 Apr 2025 14:09:34 -0400
Subject: [PATCH v1 4/5] Make SQLFunctionCache long-lived again.
At this point, the only data structures we allocate directly in
fcontext are the SQLFunctionCache struct itself, the ParamListInfo
struct, and the execution_state array, all of which are small and
perfectly capable of being re-used across executions of the same
FmgrInfo. Hence, let's give them the same lifespan as the FmgrInfo.
This step gets rid of the separate SQLFunctionLink struct and makes
fn_extra point to SQLFunctionCache again. We also get rid of the
separate fcontext memory context and allocate these items directly
in fn_mcxt.
For notational simplicity, SQLFunctionCache still has an fcontext
field, but it's just a copy of fn_mcxt.
The motivation for this is to allow these structures to live as
long as the FmgrInfo and be re-used across calls, restoring the
original design without its propensity for memory leaks. This
gets rid of some per-call overhead that we added in 0dca5d68d.
We also make an effort to re-use the JunkFilter and result slot.
Those might need to change if the function definition changes,
so we compromise by rebuilding them if the cached plan changes.
This also moves the tuplestore into fn_mcxt so that it can be
re-used across calls, again undoing a change made in 0dca5d68d.
---
src/backend/executor/functions.c | 260 +++++++++++++------------------
1 file changed, 105 insertions(+), 155 deletions(-)
diff --git a/src/backend/executor/functions.c b/src/backend/executor/functions.c
index 12121ad74e7..135fddda3fc 100644
--- a/src/backend/executor/functions.c
+++ b/src/backend/executor/functions.c
@@ -76,7 +76,7 @@ typedef struct execution_state
/*
- * Data associated with a SQL-language function is kept in three main
+ * Data associated with a SQL-language function is kept in two main
* data structures:
*
* 1. SQLFunctionHashEntry is a long-lived (potentially session-lifespan)
@@ -85,7 +85,7 @@ typedef struct execution_state
* of plans for the query(s) within the function. A SQLFunctionHashEntry is
* potentially shared across multiple concurrent executions of the function,
* so it must contain no execution-specific state; but its use_count must
- * reflect the number of SQLFunctionLink structs pointing at it.
+ * reflect the number of SQLFunctionCache structs pointing at it.
* If the function's pg_proc row is updated, we throw away and regenerate
* the SQLFunctionHashEntry and subsidiary data. (Also note that if the
* function is polymorphic or used as a trigger, there is a separate
@@ -99,22 +99,13 @@ typedef struct execution_state
* * hcontext ("hash context") holds everything else belonging to the
* SQLFunctionHashEntry.
*
- * 2. SQLFunctionCache lasts for the duration of a single execution of
- * the SQL function. (In "lazyEval" mode, this might span multiple calls of
- * fmgr_sql.) It holds a reference to the CachedPlan for the current query,
- * and other data that is execution-specific. The SQLFunctionCache itself
- * as well as its subsidiary data are kept in fcontext ("function context"),
- * which we free at completion. In non-returnsSet mode, this is just a child
- * of the call-time context. In returnsSet mode, it is made a child of the
- * FmgrInfo's fn_mcxt so that it will survive between fmgr_sql calls.
- * The fcontext may have subsidiary contexts jfcontext and/or subcontext,
- * which have somewhat shorter lifespans.
- *
- * 3. SQLFunctionLink is a tiny struct that just holds pointers to
- * the SQLFunctionHashEntry and the current SQLFunctionCache (if any).
+ * 2. SQLFunctionCache is subsidiary data for a single FmgrInfo struct.
* It is pointed to by the fn_extra field of the FmgrInfo struct, and is
- * always allocated in the FmgrInfo's fn_mcxt. Its purpose is to reduce
- * the cost of repeat lookups of the SQLFunctionHashEntry.
+ * always allocated in the FmgrInfo's fn_mcxt. It holds a reference to
+ * the CachedPlan for the current query, and other execution-specific data.
+ * A few subsidiary items such as the ParamListInfo object are also kept
+ * directly in fn_mcxt (which is also called fcontext here). But most
+ * subsidiary data is in jfcontext or subcontext.
*/
typedef struct SQLFunctionHashEntry
@@ -160,11 +151,15 @@ typedef struct SQLFunctionCache
Tuplestorestate *tstore; /* where we accumulate result tuples */
JunkFilter *junkFilter; /* will be NULL if function returns VOID */
+ int jf_generation; /* tracks whether junkFilter is up-to-date */
/*
* While executing a particular query within the function, cplan is the
* CachedPlan we've obtained for that query, and eslist is a chain of
* execution_state records for the individual plans within the CachedPlan.
+ * If eslist is not NULL at entry to fmgr_sql, then we are resuming
+ * execution of a lazyEval-mode set-returning function.
+ *
* next_query_index is the 0-based index of the next CachedPlanSource to
* get a CachedPlan from.
*/
@@ -184,22 +179,12 @@ typedef struct SQLFunctionCache
MemoryContext jfcontext; /* subsidiary memory context holding
* junkFilter, result slot, and related data */
MemoryContext subcontext; /* subsidiary memory context for sub-executor */
-} SQLFunctionCache;
-
-typedef SQLFunctionCache *SQLFunctionCachePtr;
-
-/* Struct pointed to by FmgrInfo.fn_extra for a SQL function */
-typedef struct SQLFunctionLink
-{
- /* Permanent pointer to associated SQLFunctionHashEntry */
- SQLFunctionHashEntry *func;
-
- /* Transient pointer to SQLFunctionCache, used only if returnsSet */
- SQLFunctionCache *fcache;
/* Callback to release our use-count on the SQLFunctionHashEntry */
MemoryContextCallback mcb;
-} SQLFunctionLink;
+} SQLFunctionCache;
+
+typedef SQLFunctionCache *SQLFunctionCachePtr;
/* non-export function prototypes */
@@ -232,7 +217,7 @@ static Datum postquel_get_single_result(TupleTableSlot *slot,
static void sql_compile_error_callback(void *arg);
static void sql_exec_error_callback(void *arg);
static void ShutdownSQLFunction(Datum arg);
-static void RemoveSQLFunctionLink(void *arg);
+static void RemoveSQLFunctionCache(void *arg);
static void check_sql_fn_statement(List *queryTreeList);
static bool check_sql_stmt_retval(List *queryTreeList,
Oid rettype, TupleDesc rettupdesc,
@@ -548,26 +533,23 @@ init_sql_fcache(FunctionCallInfo fcinfo, bool lazyEvalOK)
FmgrInfo *finfo = fcinfo->flinfo;
SQLFunctionHashEntry *func;
SQLFunctionCache *fcache;
- SQLFunctionLink *flink;
- MemoryContext pcontext;
- MemoryContext fcontext;
- MemoryContext oldcontext;
/*
- * If this is the first execution for this FmgrInfo, set up a link struct
- * (initially containing null pointers). The link must live as long as
+ * 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.
*/
- flink = finfo->fn_extra;
- if (flink == NULL)
+ fcache = finfo->fn_extra;
+ if (fcache == NULL)
{
- flink = (SQLFunctionLink *)
- MemoryContextAllocZero(finfo->fn_mcxt, sizeof(SQLFunctionLink));
- flink->mcb.func = RemoveSQLFunctionLink;
- flink->mcb.arg = flink;
- MemoryContextRegisterResetCallback(finfo->fn_mcxt, &flink->mcb);
- finfo->fn_extra = flink;
+ fcache = (SQLFunctionCache *)
+ MemoryContextAllocZero(finfo->fn_mcxt, sizeof(SQLFunctionCache));
+ fcache->fcontext = finfo->fn_mcxt;
+ fcache->mcb.func = RemoveSQLFunctionCache;
+ fcache->mcb.arg = fcache;
+ MemoryContextRegisterResetCallback(finfo->fn_mcxt, &fcache->mcb);
+ finfo->fn_extra = fcache;
}
/*
@@ -576,10 +558,10 @@ init_sql_fcache(FunctionCallInfo fcinfo, bool lazyEvalOK)
* SQLFunctionHashEntry: we want to run to completion using the function's
* initial definition.
*/
- if (flink->fcache != NULL)
+ if (fcache->eslist != NULL)
{
- Assert(flink->fcache->func == flink->func);
- return flink->fcache;
+ Assert(fcache->func != NULL);
+ return fcache;
}
/*
@@ -590,7 +572,7 @@ init_sql_fcache(FunctionCallInfo fcinfo, bool lazyEvalOK)
*/
func = (SQLFunctionHashEntry *)
cached_function_compile(fcinfo,
- (CachedFunction *) flink->func,
+ (CachedFunction *) fcache->func,
sql_compile_callback,
sql_delete_callback,
sizeof(SQLFunctionHashEntry),
@@ -598,60 +580,38 @@ init_sql_fcache(FunctionCallInfo fcinfo, bool lazyEvalOK)
false);
/*
- * Install the hash pointer in the SQLFunctionLink, and increment its use
+ * Install the hash pointer in the SQLFunctionCache, 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 (func != flink->func)
+ if (func != fcache->func)
{
- if (flink->func != NULL)
+ if (fcache->func != NULL)
{
- Assert(flink->func->cfunc.use_count > 0);
- flink->func->cfunc.use_count--;
+ Assert(fcache->func->cfunc.use_count > 0);
+ fcache->func->cfunc.use_count--;
}
- flink->func = func;
+ fcache->func = func;
func->cfunc.use_count++;
+ /* Assume we need to rebuild the junkFilter */
+ fcache->junkFilter = NULL;
}
- /*
- * Create memory context that holds all the SQLFunctionCache data. If we
- * return a set, we must keep this in whatever context holds the FmgrInfo
- * (anything shorter-lived risks leaving a dangling pointer in flink). But
- * in a non-SRF we'll delete it before returning, and there's no need for
- * it to outlive the caller's context.
- */
- pcontext = func->returnsSet ? finfo->fn_mcxt : CurrentMemoryContext;
- fcontext = AllocSetContextCreate(pcontext,
- "SQL function cache",
- ALLOCSET_SMALL_SIZES);
-
- oldcontext = MemoryContextSwitchTo(fcontext);
-
- /*
- * Create the struct proper, link it to func and fcontext.
- */
- fcache = (SQLFunctionCache *) palloc0(sizeof(SQLFunctionCache));
- fcache->func = func;
- fcache->fcontext = fcontext;
- fcache->lazyEvalOK = lazyEvalOK;
-
- /*
- * If we return a set, we must link the fcache into fn_extra so that we
- * can find it again during future calls. But in a non-SRF there is no
- * need to link it into fn_extra at all. Not doing so removes the risk of
- * having a dangling pointer in a long-lived FmgrInfo.
- */
- if (func->returnsSet)
- flink->fcache = fcache;
-
/*
* We're beginning a new execution of the function, so convert params to
* appropriate format.
*/
postquel_sub_params(fcache, fcinfo);
- MemoryContextSwitchTo(oldcontext);
+ /* Also reset lazyEval state for the new execution. */
+ fcache->lazyEvalOK = lazyEvalOK;
+ fcache->lazyEval = false;
+
+ /* Also reset data about where we are in the function. */
+ fcache->eslist = NULL;
+ fcache->next_query_index = 0;
+ fcache->error_query_index = 0;
return fcache;
}
@@ -798,10 +758,15 @@ init_execution_state(SQLFunctionCachePtr fcache)
* anything very interesting, but much of this module expects it to be
* there anyway.)
*
+ * Normally we can re-use the JunkFilter across executions, but if the
+ * plan for the last CachedPlanSource changed, we'd better rebuild it.
+ *
* The JunkFilter, its result slot, and its tupledesc are kept in a
* subsidiary memory context so that we can free them easily when needed.
*/
- if (fcache->func->rettype != VOIDOID)
+ if (fcache->func->rettype != VOIDOID &&
+ (fcache->junkFilter == NULL ||
+ fcache->jf_generation != fcache->cplan->generation))
{
TupleTableSlot *slot;
List *resulttlist;
@@ -855,6 +820,9 @@ init_execution_state(SQLFunctionCachePtr fcache)
if (fcache->func->returnsTuple)
BlessTupleDesc(fcache->junkFilter->jf_resultSlot->tts_tupleDescriptor);
+ /* Mark the JunkFilter as up-to-date */
+ fcache->jf_generation = fcache->cplan->generation;
+
MemoryContextSwitchTo(oldcontext);
}
@@ -1448,12 +1416,7 @@ postquel_end(execution_state *es, SQLFunctionCachePtr fcache)
fcache->subcontext = NULL;
}
-/*
- * Build ParamListInfo array representing current arguments.
- *
- * This must be called in the fcontext so that the results have adequate
- * lifespan.
- */
+/* Build ParamListInfo array representing current arguments */
static void
postquel_sub_params(SQLFunctionCachePtr fcache,
FunctionCallInfo fcinfo)
@@ -1467,8 +1430,13 @@ postquel_sub_params(SQLFunctionCachePtr fcache,
if (fcache->paramLI == NULL)
{
+ /* First time through: build a persistent ParamListInfo struct */
+ MemoryContext oldcontext;
+
+ oldcontext = MemoryContextSwitchTo(fcache->fcontext);
paramLI = makeParamList(nargs);
fcache->paramLI = paramLI;
+ MemoryContextSwitchTo(oldcontext);
}
else
{
@@ -1552,9 +1520,7 @@ Datum
fmgr_sql(PG_FUNCTION_ARGS)
{
SQLFunctionCachePtr fcache;
- SQLFunctionLink *flink;
ErrorContextCallback sqlerrcontext;
- MemoryContext tscontext;
bool randomAccess;
bool lazyEvalOK;
bool pushed_snapshot;
@@ -1581,23 +1547,17 @@ fmgr_sql(PG_FUNCTION_ARGS)
errmsg("set-valued function called in context that cannot accept a set")));
randomAccess = rsi->allowedModes & SFRM_Materialize_Random;
lazyEvalOK = !(rsi->allowedModes & SFRM_Materialize_Preferred);
- /* tuplestore must have query lifespan */
- tscontext = rsi->econtext->ecxt_per_query_memory;
}
else
{
randomAccess = false;
lazyEvalOK = true;
- /* tuplestore needn't outlive caller context */
- tscontext = CurrentMemoryContext;
}
/*
* Initialize fcache if starting a fresh execution.
*/
fcache = init_sql_fcache(fcinfo, lazyEvalOK);
- /* init_sql_fcache also ensures we have a SQLFunctionLink */
- flink = fcinfo->flinfo->fn_extra;
/*
* Now we can set up error traceback support for ereport()
@@ -1608,14 +1568,15 @@ fmgr_sql(PG_FUNCTION_ARGS)
error_context_stack = &sqlerrcontext;
/*
- * Build tuplestore to hold results, if we don't have one already. Make
- * sure it's in a suitable context.
+ * Build tuplestore to hold results, if we don't have one already. We
+ * want to re-use the tuplestore across calls, so it needs to live in
+ * fcontext.
*/
if (!fcache->tstore)
{
MemoryContext oldcontext;
- oldcontext = MemoryContextSwitchTo(tscontext);
+ oldcontext = MemoryContextSwitchTo(fcache->fcontext);
fcache->tstore = tuplestore_begin_heap(randomAccess, false, work_mem);
MemoryContextSwitchTo(oldcontext);
}
@@ -1773,7 +1734,7 @@ fmgr_sql(PG_FUNCTION_ARGS)
{
RegisterExprContextCallback(rsi->econtext,
ShutdownSQLFunction,
- PointerGetDatum(flink));
+ PointerGetDatum(fcache));
fcache->shutdown_reg = true;
}
}
@@ -1797,7 +1758,7 @@ fmgr_sql(PG_FUNCTION_ARGS)
{
UnregisterExprContextCallback(rsi->econtext,
ShutdownSQLFunction,
- PointerGetDatum(flink));
+ PointerGetDatum(fcache));
fcache->shutdown_reg = false;
}
}
@@ -1823,7 +1784,7 @@ fmgr_sql(PG_FUNCTION_ARGS)
{
UnregisterExprContextCallback(rsi->econtext,
ShutdownSQLFunction,
- PointerGetDatum(flink));
+ PointerGetDatum(fcache));
fcache->shutdown_reg = false;
}
}
@@ -1862,17 +1823,11 @@ fmgr_sql(PG_FUNCTION_ARGS)
PopActiveSnapshot();
/*
- * If we've gone through every command in the function, we are done.
- * Release the cache to start over again on next call.
+ * If we've gone through every command in the function, we are done. Reset
+ * state to start over again on next call.
*/
if (es == NULL)
- {
- if (fcache->tstore)
- tuplestore_end(fcache->tstore);
- Assert(fcache->cplan == NULL);
- flink->fcache = NULL;
- MemoryContextDelete(fcache->fcontext);
- }
+ fcache->eslist = NULL;
error_context_stack = sqlerrcontext.previous;
@@ -1958,67 +1913,62 @@ sql_exec_error_callback(void *arg)
static void
ShutdownSQLFunction(Datum arg)
{
- SQLFunctionLink *flink = (SQLFunctionLink *) DatumGetPointer(arg);
- SQLFunctionCachePtr fcache = flink->fcache;
+ SQLFunctionCachePtr fcache = (SQLFunctionCachePtr) DatumGetPointer(arg);
+ execution_state *es;
- if (fcache != NULL)
+ es = fcache->eslist;
+ while (es)
{
- execution_state *es;
-
- /* Make sure we don't somehow try to do this twice */
- flink->fcache = NULL;
-
- es = fcache->eslist;
- while (es)
+ /* Shut down anything still running */
+ if (es->status == F_EXEC_RUN)
{
- /* Shut down anything still running */
- if (es->status == F_EXEC_RUN)
- {
- /* Re-establish active snapshot for any called functions */
- if (!fcache->func->readonly_func)
- PushActiveSnapshot(es->qd->snapshot);
+ /* Re-establish active snapshot for any called functions */
+ if (!fcache->func->readonly_func)
+ PushActiveSnapshot(es->qd->snapshot);
- postquel_end(es, fcache);
+ postquel_end(es, fcache);
- if (!fcache->func->readonly_func)
- PopActiveSnapshot();
- }
- es = es->next;
+ if (!fcache->func->readonly_func)
+ PopActiveSnapshot();
}
+ es = es->next;
+ }
+ fcache->eslist = NULL;
- /* Release tuplestore if we have one */
- if (fcache->tstore)
- tuplestore_end(fcache->tstore);
+ /* Release tuplestore if we have one */
+ if (fcache->tstore)
+ tuplestore_end(fcache->tstore);
+ fcache->tstore = NULL;
- /* Release CachedPlan if we have one */
- if (fcache->cplan)
- ReleaseCachedPlan(fcache->cplan, fcache->cowner);
+ /* Release CachedPlan if we have one */
+ if (fcache->cplan)
+ ReleaseCachedPlan(fcache->cplan, fcache->cowner);
+ fcache->cplan = NULL;
- /* Release the cache */
- MemoryContextDelete(fcache->fcontext);
- }
/* execUtils will deregister the callback... */
+ fcache->shutdown_reg = false;
}
/*
* MemoryContext callback function
*
- * We register this in the memory context that contains a SQLFunctionLink
+ * We register this in the memory context that contains a SQLFunctionCache
* struct. When the memory context is reset or deleted, we release the
- * reference count (if any) that the link holds on the long-lived hash entry.
+ * reference count (if any) that the cache holds on the long-lived hash entry.
* Note that this will happen even during error aborts.
*/
static void
-RemoveSQLFunctionLink(void *arg)
+RemoveSQLFunctionCache(void *arg)
{
- SQLFunctionLink *flink = (SQLFunctionLink *) arg;
+ SQLFunctionCache *fcache = (SQLFunctionCache *) arg;
- if (flink->func != NULL)
+ /* Release reference count on SQLFunctionHashEntry */
+ if (fcache->func != NULL)
{
- Assert(flink->func->cfunc.use_count > 0);
- flink->func->cfunc.use_count--;
+ Assert(fcache->func->cfunc.use_count > 0);
+ fcache->func->cfunc.use_count--;
/* This should be unnecessary, but let's just be sure: */
- flink->func = NULL;
+ fcache->func = NULL;
}
}
--
2.43.5
From 5704b126123a71a41062b9ab502564842fc3ddc9 Mon Sep 17 00:00:00 2001
From: Tom Lane <tgl@sss.pgh.pa.us>
Date: Sun, 13 Apr 2025 14:10:28 -0400
Subject: [PATCH v1 5/5] Cache typlens of a SQL function's input arguments.
This gets rid of repetitive get_typlen() calls in postquel_sub_params,
which show up as costing 1% or so of the runtime in simple test cases.
---
src/backend/executor/functions.c | 13 ++++++++++++-
1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/src/backend/executor/functions.c b/src/backend/executor/functions.c
index 135fddda3fc..e0bca7cb81c 100644
--- a/src/backend/executor/functions.c
+++ b/src/backend/executor/functions.c
@@ -116,6 +116,7 @@ typedef struct SQLFunctionHashEntry
char *src; /* function body text (for error msgs) */
SQLFunctionParseInfoPtr pinfo; /* data for parser callback hooks */
+ int16 *argtyplen; /* lengths of the input argument types */
Oid rettype; /* actual return type */
int16 typlen; /* length of the return type */
@@ -1100,6 +1101,15 @@ sql_compile_callback(FunctionCallInfo fcinfo,
PG_GET_COLLATION());
MemoryContextSwitchTo(oldcontext);
+ /*
+ * Now that we have the resolved argument types, collect their typlens for
+ * use in postquel_sub_params.
+ */
+ func->argtyplen = (int16 *)
+ MemoryContextAlloc(hcontext, func->pinfo->nargs * sizeof(int16));
+ for (int i = 0; i < func->pinfo->nargs; i++)
+ func->argtyplen[i] = get_typlen(func->pinfo->argtypes[i]);
+
/*
* And of course we need the function body text.
*/
@@ -1427,6 +1437,7 @@ postquel_sub_params(SQLFunctionCachePtr fcache,
{
ParamListInfo paramLI;
Oid *argtypes = fcache->func->pinfo->argtypes;
+ int16 *argtyplen = fcache->func->argtyplen;
if (fcache->paramLI == NULL)
{
@@ -1463,7 +1474,7 @@ postquel_sub_params(SQLFunctionCachePtr fcache,
prm->isnull = fcinfo->args[i].isnull;
prm->value = MakeExpandedObjectReadOnly(fcinfo->args[i].value,
prm->isnull,
- get_typlen(argtypes[i]));
+ argtyplen[i]);
/* Allow the value to be substituted into custom plans */
prm->pflags = PARAM_FLAG_CONST;
prm->ptype = argtypes[i];
--
2.43.5
On Sun, Apr 13, 2025 at 3:23 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > create function fx(p_summa bigint) returns text immutable strict > return ltrim(to_char(p_summa, '999 999 999 999 999 999 999 999')); > > explain analyze select fx(i) from generate_series(1,1000000) as i(i); > > you arrive at the rude discovery that 0dca5d68d is about 50% slower > than 0dca5d68d^, because the old implementation builds a plan for fx() > only once and then re-uses it throughout the query. I agree that we should do something about this. I haven't reviewed your patches but the approach sounds broadly reasonable. -- Robert Haas EDB: http://www.enterprisedb.com
Hi
po 14. 4. 2025 v 16:38 odesílatel Robert Haas <robertmhaas@gmail.com> napsal:
On Sun, Apr 13, 2025 at 3:23 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> create function fx(p_summa bigint) returns text immutable strict
> return ltrim(to_char(p_summa, '999 999 999 999 999 999 999 999'));
>
> explain analyze select fx(i) from generate_series(1,1000000) as i(i);
>
> you arrive at the rude discovery that 0dca5d68d is about 50% slower
> than 0dca5d68d^, because the old implementation builds a plan for fx()
> only once and then re-uses it throughout the query.
I agree that we should do something about this. I haven't reviewed
your patches but the approach sounds broadly reasonable.
I can confirm that all tests passed, and patched code is about 5% faster than the current master (tested on my slower notebook). So it should to fix performance regression where it was it against pg17 (it was about 2%) (tested without assertions)
Regards
Pavel
--
Robert Haas
EDB: http://www.enterprisedb.com