pgsql: Fix plpgsql to release SPI plans when a function or DO block is
Fix plpgsql to release SPI plans when a function or DO block is freed.
This fixes the gripe I made a few months ago about DO blocks getting
slower with repeated use. At least, it fixes it for the case where
the DO block isn't aborted by an error. We could try running
plpgsql_free_function_memory() even during error exit, but that seems
a bit scary since it makes a lot of presumptions about the data
structures being in good shape. It's probably reasonable to assume
that repeated failures of DO blocks isn't a performance-critical case.
Branch
------
master
Details
-------
http://git.postgresql.org/pg/commitdiff/87f2ad1326bff5cd37dde6fbf024137a2243efea
Modified Files
--------------
src/pl/plpgsql/src/pl_comp.c | 7 +-
src/pl/plpgsql/src/pl_funcs.c | 398 +++++++++++++++++++++++++++++++++++++++
src/pl/plpgsql/src/pl_handler.c | 10 +
src/pl/plpgsql/src/plpgsql.h | 1 +
4 files changed, 411 insertions(+), 5 deletions(-)
On Sun, Mar 27, 2011 at 04:51:13PM +0000, Tom Lane wrote:
Fix plpgsql to release SPI plans when a function or DO block is freed.
Do the other PLs we ship need similar fixes?
Cheers,
David.
--
David Fetter <david@fetter.org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david.fetter@gmail.com
iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics
Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate
David Fetter <david@fetter.org> writes:
On Sun, Mar 27, 2011 at 04:51:13PM +0000, Tom Lane wrote:
Fix plpgsql to release SPI plans when a function or DO block is freed.
Do the other PLs we ship need similar fixes?
Offhand I think the other PLs leave management of prepared plans to the
user. If there are any places where they cache plans behind the scenes,
maybe a similar fix would be appropriate.
regards, tom lane
On 27.03.2011 19:51, Tom Lane wrote:
Fix plpgsql to release SPI plans when a function or DO block is freed.
This fixes the gripe I made a few months ago about DO blocks getting
slower with repeated use. At least, it fixes it for the case where
the DO block isn't aborted by an error. We could try running
plpgsql_free_function_memory() even during error exit, but that seems
a bit scary since it makes a lot of presumptions about the data
structures being in good shape. It's probably reasonable to assume
that repeated failures of DO blocks isn't a performance-critical case.
I was quite surprised by the way you did this. Instead of adding all
that code to traverse the PLpgSQL_stmt tree (that we'll have to remember
to keep up-to-date), can't we just have a list of cached plans in
PLpgSQL_function? As attached.
--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com
Attachments:
plpgsql-release-cached-plans.patchtext/x-diff; name=plpgsql-release-cached-plans.patchDownload
diff --git a/src/pl/plpgsql/src/pl_comp.c b/src/pl/plpgsql/src/pl_comp.c
index a928e2f..b2139d6 100644
--- a/src/pl/plpgsql/src/pl_comp.c
+++ b/src/pl/plpgsql/src/pl_comp.c
@@ -2368,12 +2368,9 @@ delete_function(PLpgSQL_function *func)
/* remove function from hash table (might be done already) */
plpgsql_HashTableDelete(func);
- /* release the function's storage if safe and not done already */
- if (func->use_count == 0 && func->fn_cxt)
- {
- MemoryContextDelete(func->fn_cxt);
- func->fn_cxt = NULL;
- }
+ /* release the function's storage if safe */
+ if (func->use_count)
+ plpgsql_free_function_memory(func);
}
/* exported so we can call it from plpgsql_init() */
diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c
index 1f4d5ac..e353374 100644
--- a/src/pl/plpgsql/src/pl_exec.c
+++ b/src/pl/plpgsql/src/pl_exec.c
@@ -2919,6 +2919,7 @@ static void
exec_prepare_plan(PLpgSQL_execstate *estate,
PLpgSQL_expr *expr, int cursorOptions)
{
+ MemoryContext oldcxt;
SPIPlanPtr plan;
/*
@@ -2956,6 +2957,11 @@ exec_prepare_plan(PLpgSQL_execstate *estate,
expr->plan = SPI_saveplan(plan);
SPI_freeplan(plan);
exec_simple_check_plan(expr);
+
+ /* Remember this plan so that it can be freed along with the function */
+ oldcxt = MemoryContextSwitchTo(expr->func->fn_cxt);
+ expr->func->cached_plans = lappend(expr->func->cached_plans, expr->plan);
+ MemoryContextSwitchTo(oldcxt);
}
diff --git a/src/pl/plpgsql/src/pl_funcs.c b/src/pl/plpgsql/src/pl_funcs.c
index f13e4c3..14c5e08 100644
--- a/src/pl/plpgsql/src/pl_funcs.c
+++ b/src/pl/plpgsql/src/pl_funcs.c
@@ -15,6 +15,8 @@
#include "plpgsql.h"
+#include "utils/memutils.h"
+
/* ----------
* Local variables for namespace handling
@@ -264,6 +266,37 @@ plpgsql_stmt_typename(PLpgSQL_stmt *stmt)
}
+/*
+ * Release memory when a PL/pgSQL function is no longer needed
+ */
+void
+plpgsql_free_function_memory(PLpgSQL_function *func)
+{
+ ListCell *lc;
+
+ /* Better not call this on an in-use function */
+ Assert(func->use_count == 0);
+
+ /* Release cached plans belonging to the function */
+ foreach(lc, func->cached_plans)
+ {
+ SPI_freeplan(lfirst(lc));
+ }
+ list_free(func->cached_plans);
+ func->cached_plans = NIL;
+
+ /*
+ * And finally, release all memory except the PLpgSQL_function struct
+ * itself (which has to be kept around because there may be multiple
+ * fn_extra pointers to it).
+ */
+ if (func->fn_cxt)
+ {
+ MemoryContextDelete(func->fn_cxt);
+ func->fn_cxt = NULL;
+ }
+}
+
/**********************************************************************
* Debug functions for analyzing the compiled code
**********************************************************************/
diff --git a/src/pl/plpgsql/src/pl_handler.c b/src/pl/plpgsql/src/pl_handler.c
index 8f7080e..2389849 100644
--- a/src/pl/plpgsql/src/pl_handler.c
+++ b/src/pl/plpgsql/src/pl_handler.c
@@ -172,6 +172,9 @@ plpgsql_inline_handler(PG_FUNCTION_ARGS)
/* Compile the anonymous code block */
func = plpgsql_compile_inline(codeblock->source_text);
+ /* Mark the function as busy, just pro forma */
+ func->use_count++;
+
/*
* Set up a fake fcinfo with just enough info to satisfy
* plpgsql_exec_function(). In particular note that this sets things up
@@ -185,6 +188,13 @@ plpgsql_inline_handler(PG_FUNCTION_ARGS)
retval = plpgsql_exec_function(func, &fake_fcinfo);
+ /* Function should now have no remaining use-counts ... */
+ func->use_count--;
+ Assert(func->use_count == 0);
+
+ /* ... so we can free subsidiary storage */
+ plpgsql_free_function_memory(func);
+
/*
* Disconnect from SPI manager
*/
diff --git a/src/pl/plpgsql/src/plpgsql.h b/src/pl/plpgsql/src/plpgsql.h
index 7015379..9efa982 100644
--- a/src/pl/plpgsql/src/plpgsql.h
+++ b/src/pl/plpgsql/src/plpgsql.h
@@ -693,6 +693,8 @@ typedef struct PLpgSQL_function
/* these fields change when the function is used */
struct PLpgSQL_execstate *cur_estate;
unsigned long use_count;
+
+ List *cached_plans;
} PLpgSQL_function;
@@ -918,6 +920,7 @@ extern PLpgSQL_nsitem *plpgsql_ns_lookup_label(PLpgSQL_nsitem *ns_cur,
* ----------
*/
extern const char *plpgsql_stmt_typename(PLpgSQL_stmt *stmt);
+extern void plpgsql_free_function_memory(PLpgSQL_function *func);
extern void plpgsql_dumptree(PLpgSQL_function *func);
/* ----------
On 28/03/11 04:31, Tom Lane wrote:
David Fetter <david@fetter.org> writes:
On Sun, Mar 27, 2011 at 04:51:13PM +0000, Tom Lane wrote:
Fix plpgsql to release SPI plans when a function or DO block is freed.
Do the other PLs we ship need similar fixes?
Offhand I think the other PLs leave management of prepared plans to the
user. If there are any places where they cache plans behind the scenes,
maybe a similar fix would be appropriate.
FWIW I executed
do $$ plpy.execute("select 1 from pg_class") $$ language plpythonu;
10k times in a session and the backend grew a lot in memory and never
released it. I can't offhand see where the memory went, I'll try to
investigate in the evening.
Cheers,
Jan
Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> writes:
On 27.03.2011 19:51, Tom Lane wrote:
Fix plpgsql to release SPI plans when a function or DO block is freed.
I was quite surprised by the way you did this. Instead of adding all
that code to traverse the PLpgSQL_stmt tree (that we'll have to remember
to keep up-to-date), can't we just have a list of cached plans in
PLpgSQL_function? As attached.
The code I added is a direct rip-off of the adjacent function tree
dumping code, which we've had no trouble keeping up-to-date. (I checked
it as I went through, and it did not appear to be missing any cases.)
I'm unconvinced that adding a separate new mechanism to track the
function's SPI plans would be more easily maintainable or less prone to
oversights. Also, what if we come up with another resource that has to
be explicitly released here? The tree walk won't require additional
mechanism for that.
regards, tom lane
* Jan Urbański (wulczer@wulczer.org) wrote:
On 28/03/11 04:31, Tom Lane wrote:
Do the other PLs we ship need similar fixes?
Offhand I think the other PLs leave management of prepared plans to the
user. If there are any places where they cache plans behind the scenes,
maybe a similar fix would be appropriate.FWIW I executed
do $$ plpy.execute("select 1 from pg_class") $$ language plpythonu;
10k times in a session and the backend grew a lot in memory and never
released it. I can't offhand see where the memory went, I'll try to
investigate in the evening.
I'm about 90% sure that they all have this problem.. I havn't had a
chance to look at how Tom fixed pl/pgsql (I didn't think it'd be easy to
do w/o coming up with a way to explicitly tell the PL to release
something) so perhaps I'm mistaken, but they all share very similar
code..
Thanks,
Stephen
On 28/03/11 17:25, Stephen Frost wrote:
* Jan Urbański (wulczer@wulczer.org) wrote:
On 28/03/11 04:31, Tom Lane wrote:
Do the other PLs we ship need similar fixes?
Offhand I think the other PLs leave management of prepared plans to the
user. If there are any places where they cache plans behind the scenes,
maybe a similar fix would be appropriate.FWIW I executed
do $$ plpy.execute("select 1 from pg_class") $$ language plpythonu;
10k times in a session and the backend grew a lot in memory and never
released it. I can't offhand see where the memory went, I'll try to
investigate in the evening.I'm about 90% sure that they all have this problem.. I havn't had a
chance to look at how Tom fixed pl/pgsql (I didn't think it'd be easy to
do w/o coming up with a way to explicitly tell the PL to release
something) so perhaps I'm mistaken, but they all share very similar
code..
Valgrind showed me the way. PFA a trivial patch to avoid leaking a
PLyProcedure struct in inline blocks.
Prepared plans get cleaned up currectly, the SPI plan is deallocated
when the Python plan object is garbage collected.
Cheers,
Jan
Attachments:
plpython-leak.difftext/x-patch; name=plpython-leak.diffDownload
diff --git a/src/pl/plpython/plpython.c b/src/pl/plpython/plpython.c
index dd2b919..58d070f 100644
--- a/src/pl/plpython/plpython.c
+++ b/src/pl/plpython/plpython.c
@@ -626,6 +626,7 @@ plpython_inline_handler(PG_FUNCTION_ARGS)
PG_CATCH();
{
PLy_procedure_delete(proc);
+ PLy_free(proc);
PLy_curr_procedure = save_curr_proc;
PyErr_Clear();
PG_RE_THROW();
@@ -633,6 +634,7 @@ plpython_inline_handler(PG_FUNCTION_ARGS)
PG_END_TRY();
PLy_procedure_delete(proc);
+ PLy_free(proc);
/* Pop the error context stack */
error_context_stack = plerrcontext.previous;
On 30.03.2011 21:21, Jan Urbański wrote:
Valgrind showed me the way. PFA a trivial patch to avoid leaking a
PLyProcedure struct in inline blocks.
Hmm, any reason the PLyProcedure struct needs to be allocated in
TopMemoryContext in the first place? Could you palloc0 it in a
shorter-lived context, or even better, just allocate it in stack?
PS. I don't think the volatile qualifier in 'proc' is in necessary. The
variable is not changed in PG_TRY-block.
--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com
Attachments:
plpython-leak-2.difftext/x-diff; name=plpython-leak-2.diffDownload
diff --git a/src/pl/plpython/plpython.c b/src/pl/plpython/plpython.c
index dd2b919..f3f5890 100644
--- a/src/pl/plpython/plpython.c
+++ b/src/pl/plpython/plpython.c
@@ -592,7 +592,7 @@ plpython_inline_handler(PG_FUNCTION_ARGS)
FunctionCallInfoData fake_fcinfo;
FmgrInfo flinfo;
PLyProcedure *save_curr_proc;
- PLyProcedure *volatile proc = NULL;
+ PLyProcedure proc;
ErrorContextCallback plerrcontext;
if (SPI_connect() != SPI_OK_CONNECT)
@@ -613,26 +613,26 @@ plpython_inline_handler(PG_FUNCTION_ARGS)
flinfo.fn_oid = InvalidOid;
flinfo.fn_mcxt = CurrentMemoryContext;
- proc = PLy_malloc0(sizeof(PLyProcedure));
- proc->pyname = PLy_strdup("__plpython_inline_block");
- proc->result.out.d.typoid = VOIDOID;
+ MemSet(&proc, 0, sizeof(PLyProcedure));
+ proc.pyname = PLy_strdup("__plpython_inline_block");
+ proc.result.out.d.typoid = VOIDOID;
PG_TRY();
{
- PLy_procedure_compile(proc, codeblock->source_text);
- PLy_curr_procedure = proc;
- PLy_function_handler(&fake_fcinfo, proc);
+ PLy_procedure_compile(&proc, codeblock->source_text);
+ PLy_curr_procedure = &proc;
+ PLy_function_handler(&fake_fcinfo, &proc);
}
PG_CATCH();
{
- PLy_procedure_delete(proc);
+ PLy_procedure_delete(&proc);
PLy_curr_procedure = save_curr_proc;
PyErr_Clear();
PG_RE_THROW();
}
PG_END_TRY();
- PLy_procedure_delete(proc);
+ PLy_procedure_delete(&proc);
/* Pop the error context stack */
error_context_stack = plerrcontext.previous;
On 31/03/11 07:35, Heikki Linnakangas wrote:
On 30.03.2011 21:21, Jan Urbański wrote:
Valgrind showed me the way. PFA a trivial patch to avoid leaking a
PLyProcedure struct in inline blocks.Hmm, any reason the PLyProcedure struct needs to be allocated in
TopMemoryContext in the first place? Could you palloc0 it in a
shorter-lived context, or even better, just allocate it in stack?
Yeah, you're right, you can keep it on the stack.
PS. I don't think the volatile qualifier in 'proc' is in necessary. The
variable is not changed in PG_TRY-block.
That always confuses me, but I guess you're right, the variable does not
change, only the memory it points to.
Cheers,
Jan
On 31.03.2011 12:30, Jan Urbański wrote:
On 31/03/11 07:35, Heikki Linnakangas wrote:
On 30.03.2011 21:21, Jan Urbański wrote:
Valgrind showed me the way. PFA a trivial patch to avoid leaking a
PLyProcedure struct in inline blocks.Hmm, any reason the PLyProcedure struct needs to be allocated in
TopMemoryContext in the first place? Could you palloc0 it in a
shorter-lived context, or even better, just allocate it in stack?Yeah, you're right, you can keep it on the stack.
PS. I don't think the volatile qualifier in 'proc' is in necessary. The
variable is not changed in PG_TRY-block.That always confuses me, but I guess you're right, the variable does not
change, only the memory it points to.
Ok then, committed.
--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com