Re: [HACKERS] Transaction-lifespan memory leak with plpgsql DO blocks
On 13 November 2013 03:17 David Johnston wrote, Having had this same thought WRT the FOR UPDATE in LOOP bug posting the lack of a listing of outstanding bugs does leave some gaps. I would imagine people would appreciate something like: Frequency: Rare Severity: Low Fix Complexity: Moderate Work Around: Easy - create an actual function; create some form of loop Status: Confirmed - Awaiting Volunteers to Fix This problem is fixed as explained below.. 1. Created own simple_eval_estate for every Do block in plpgsql_inline_handler and Freed it after the execution is finished. 2. While executing the simple expression if func-simple_eval_estate is not null (means its Do block) then use this otherwise use globle one. Patch is attached in the mail. Please let me know whether this approach is fine or not ? Regards, Dilip transaction-lifespan_memory_leak_fix.patch Description: transaction-lifespan_memory_leak_fix.patch -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Transaction-lifespan memory leak with plpgsql DO blocks
On 2013-11-14 22:23, Tom Lane wrote: So after some experimentation I came up with version 2, attached. Thanks for looking into this! I currently do not have access to a setup to try the patch. A colleague of mine will look into this next week. Thanks again, Yeb Havinga -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Transaction-lifespan memory leak with plpgsql DO blocks
I wrote:
Robert Haas robertmh...@gmail.com writes:
I'm not volunteering to spend time fixing this, but I disagree with
the premise that it isn't worth fixing, because I think it's a POLA
violation.
Yeah, I'm not terribly comfortable with letting it go either. Attached
is a proposed patch. I couldn't see any nice way to do it without adding
a field to PLpgSQL_execstate, so this isn't a feasible solution for
back-patching (it'd break the plpgsql debugger). However, given the
infrequency of complaints, I think fixing it in 9.4 and up is good enough.
This patch crashed and burned when I tried it on a case where a DO block
traps an exception :-(. I had thought that a private econtext stack was
the right thing to do, but it isn't because we need plpgsql_subxact_cb
to be able to clean up dead econtexts in either functions or DO blocks.
So after some experimentation I came up with version 2, attached.
The additional test case I was using looks like
do $outer$
begin
for i in 1..10 loop
begin
execute $e$
do $$
declare x int = 0;
begin
x := 1 / x;
end;
$$;
$e$;
exception when division_by_zero then null;
end;
end loop;
end;
$outer$;
If you try this with the patch, you'll notice that there's still a slow
leak, but that's not the fault of DO blocks: the same leak exists if you
transpose this code into a regular function. That leak is the fault of
exec_stmt_dynexecute, which copies the querystring into the function's
main execution context, from which it won't get freed if an error is
thrown out of SPI_execute. (If we were using EXECUTE USING, the ppd
structure would get leaked too.) There are some other similar error-
case leaks in other plpgsql statements, I think. I'm not excited
about trying to clean those up as part of this patch.
regards, tom lane
diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c
index bc31fe9..f5f1892 100644
*** a/src/pl/plpgsql/src/pl_exec.c
--- b/src/pl/plpgsql/src/pl_exec.c
*** typedef struct
*** 66,71
--- 66,80
* so that we don't have to re-prepare simple expressions on each trip through
* a function. (We assume the case to optimize is many repetitions of a
* function within a transaction.)
+ *
+ * However, there's no value in trying to amortize simple expression setup
+ * across multiple executions of a DO block (inline code block), since there
+ * can never be any. If we use the shared EState for a DO block, the expr
+ * state trees are effectively leaked till end of transaction, and that can
+ * add up if the user keeps on submitting DO blocks. Therefore, each DO block
+ * has its own simple-expression EState, which is cleaned up at exit from
+ * plpgsql_inline_handler(). DO blocks still use the simple_econtext_stack,
+ * though, so that subxact abort cleanup does the right thing.
*/
typedef struct SimpleEcontextStackEntry
{
*** typedef struct SimpleEcontextStackEntry
*** 74,80
struct SimpleEcontextStackEntry *next; /* next stack entry up */
} SimpleEcontextStackEntry;
! static EState *simple_eval_estate = NULL;
static SimpleEcontextStackEntry *simple_econtext_stack = NULL;
/
--- 83,89
struct SimpleEcontextStackEntry *next; /* next stack entry up */
} SimpleEcontextStackEntry;
! static EState *shared_simple_eval_estate = NULL;
static SimpleEcontextStackEntry *simple_econtext_stack = NULL;
/
*** static int exec_stmt_dynfors(PLpgSQL_exe
*** 136,142
static void plpgsql_estate_setup(PLpgSQL_execstate *estate,
PLpgSQL_function *func,
! ReturnSetInfo *rsi);
static void exec_eval_cleanup(PLpgSQL_execstate *estate);
static void exec_prepare_plan(PLpgSQL_execstate *estate,
--- 145,152
static void plpgsql_estate_setup(PLpgSQL_execstate *estate,
PLpgSQL_function *func,
! ReturnSetInfo *rsi,
! EState *simple_eval_estate);
static void exec_eval_cleanup(PLpgSQL_execstate *estate);
static void exec_prepare_plan(PLpgSQL_execstate *estate,
*** static char *format_preparedparamsdata(P
*** 230,239
/* --
* plpgsql_exec_function Called by the call handler for
*function execution.
* --
*/
Datum
! plpgsql_exec_function(PLpgSQL_function *func, FunctionCallInfo fcinfo)
{
PLpgSQL_execstate estate;
ErrorContextCallback plerrcontext;
--- 240,256
/* --
* plpgsql_exec_function Called by the call handler for
*function execution.
+ *
+ * This is also used to execute inline code blocks (DO blocks). The only
+ * difference that this code is aware of is that for a DO block, we want
+ * to use a private simple_eval_estate, which is created and passed in by
+ * the caller. For regular functions, pass NULL,
Re: [HACKERS] Transaction-lifespan memory leak with plpgsql DO blocks
Robert Haas robertmh...@gmail.com writes:
On Tue, Nov 12, 2013 at 11:18 AM, Tom Lane t...@sss.pgh.pa.us wrote:
Or we could say what the heck are you doing executing tens of
thousands of DO blocks? Make it into a real live function;
you'll save a lot of cycles on parsing costs. I'm not sure that
this is a usage pattern we ought to be optimizing for.
I'm not volunteering to spend time fixing this, but I disagree with
the premise that it isn't worth fixing, because I think it's a POLA
violation.
Yeah, I'm not terribly comfortable with letting it go either. Attached
is a proposed patch. I couldn't see any nice way to do it without adding
a field to PLpgSQL_execstate, so this isn't a feasible solution for
back-patching (it'd break the plpgsql debugger). However, given the
infrequency of complaints, I think fixing it in 9.4 and up is good enough.
I checked that this eliminates the memory leak using this test case:
do $outer$
begin
for i in 1..100 loop
execute $e$
do $$
declare x int = 0;
begin
x := x + 1;
end;
$$;
$e$;
end loop;
end;
$outer$;
which eats a couple GB in HEAD and nothing with the patch.
The run time seems to be the same or a bit less, too.
Any objections to applying this to HEAD?
regards, tom lane
diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c
index bc31fe9..76da842 100644
*** a/src/pl/plpgsql/src/pl_exec.c
--- b/src/pl/plpgsql/src/pl_exec.c
*** typedef struct
*** 50,81
bool *freevals; /* which arguments are pfree-able */
} PreparedParamsData;
! /*
! * All plpgsql function executions within a single transaction share the same
! * executor EState for evaluating simple expressions. Each function call
! * creates its own eval_econtext ExprContext within this estate for
! * per-evaluation workspace. eval_econtext is freed at normal function exit,
! * and the EState is freed at transaction end (in case of error, we assume
! * that the abort mechanisms clean it all up). Furthermore, any exception
! * block within a function has to have its own eval_econtext separate from
! * the containing function's, so that we can clean up ExprContext callbacks
! * properly at subtransaction exit. We maintain a stack that tracks the
! * individual econtexts so that we can clean up correctly at subxact exit.
! *
! * This arrangement is a bit tedious to maintain, but it's worth the trouble
! * so that we don't have to re-prepare simple expressions on each trip through
! * a function. (We assume the case to optimize is many repetitions of a
! * function within a transaction.)
! */
! typedef struct SimpleEcontextStackEntry
! {
! ExprContext *stack_econtext; /* a stacked econtext */
! SubTransactionId xact_subxid; /* ID for current subxact */
! struct SimpleEcontextStackEntry *next; /* next stack entry up */
! } SimpleEcontextStackEntry;
!
! static EState *simple_eval_estate = NULL;
! static SimpleEcontextStackEntry *simple_econtext_stack = NULL;
/
* Local function forward declarations
--- 50,57
bool *freevals; /* which arguments are pfree-able */
} PreparedParamsData;
! /* Shared simple-expression eval context for regular plpgsql functions */
! static SimpleEvalContext simple_eval_context = {NULL, NULL};
/
* Local function forward declarations
*** static int exec_stmt_dynfors(PLpgSQL_exe
*** 136,142
static void plpgsql_estate_setup(PLpgSQL_execstate *estate,
PLpgSQL_function *func,
! ReturnSetInfo *rsi);
static void exec_eval_cleanup(PLpgSQL_execstate *estate);
static void exec_prepare_plan(PLpgSQL_execstate *estate,
--- 112,119
static void plpgsql_estate_setup(PLpgSQL_execstate *estate,
PLpgSQL_function *func,
! ReturnSetInfo *rsi,
! SimpleEvalContext *simple_context);
static void exec_eval_cleanup(PLpgSQL_execstate *estate);
static void exec_prepare_plan(PLpgSQL_execstate *estate,
*** static char *format_preparedparamsdata(P
*** 230,239
/* --
* plpgsql_exec_function Called by the call handler for
*function execution.
* --
*/
Datum
! plpgsql_exec_function(PLpgSQL_function *func, FunctionCallInfo fcinfo)
{
PLpgSQL_execstate estate;
ErrorContextCallback plerrcontext;
--- 207,222
/* --
* plpgsql_exec_function Called by the call handler for
*function execution.
+ *
+ * This is also used to execute inline code blocks (DO blocks). The only
+ * difference that this code is aware of is that for a DO block, we want
+ * to use a private SimpleEvalContext, whose address must be passed as
+ * simple_context. For regular functions, pass NULL.
* --
*/
Datum
! plpgsql_exec_function(PLpgSQL_function *func, FunctionCallInfo
Re: [HACKERS] Transaction-lifespan memory leak with plpgsql DO blocks
On 2013-11-12 11:18:32 -0500, Tom Lane wrote: Or we could say what the heck are you doing executing tens of thousands of DO blocks? Make it into a real live function; you'll save a lot of cycles on parsing costs. I'm not sure that this is a usage pattern we ought to be optimizing for. Exactly, I think it's not worth spending much code/complexity on that issue. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Transaction-lifespan memory leak with plpgsql DO blocks
On Tue, Nov 12, 2013 at 11:18 AM, Tom Lane t...@sss.pgh.pa.us wrote: Or we could say what the heck are you doing executing tens of thousands of DO blocks? Make it into a real live function; you'll save a lot of cycles on parsing costs. I'm not sure that this is a usage pattern we ought to be optimizing for. I'm not volunteering to spend time fixing this, but I disagree with the premise that it isn't worth fixing, because I think it's a POLA violation. From the user's point of view, there are plausible reasons for this to be slow, but there's really no reason to expect it to leak memory. That's a sufficiently astonishing result that it wouldn't be surprising for this to get reported as a bug where a simple performance gap wouldn't be, and I think if we don't fix it the perception will be that we've left that bug unfixed. Now, there are lots of things we don't fix just because there is not an infinitely large army of trained PostgreSQL hackers who love to fix other people's bugs for free, so I'm not going to say we HAVE to fix this or whatever - but neither do I think fixing it is useless and worthless. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Transaction-lifespan memory leak with plpgsql DO blocks
Robert Haas wrote That's a sufficiently astonishing result that it wouldn't be surprising for this to get reported as a bug where a simple performance gap wouldn't be, and I think if we don't fix it the perception will be that we've left that bug unfixed. Now, there are lots of things we don't fix just because there is not an infinitely large army of trained PostgreSQL hackers who love to fix other people's bugs for free, so I'm not going to say we HAVE to fix this or whatever - but neither do I think fixing it is useless and worthless. Having had this same thought WRT the FOR UPDATE in LOOP bug posting the lack of a listing of outstanding bugs does leave some gaps. I would imagine people would appreciate something like: Frequency: Rare Severity: Low Fix Complexity: Moderate Work Around: Easy - create an actual function; create some form of loop Status: Confirmed - Awaiting Volunteers to Fix Even without a formal system it may not hurt for bug threads to have a posting with this kind of information summarizing the thread. As Tom is apt to do - for the sake of the archives - though mostly I see those once something has been fixed and not for items that are being left open. Ideally these could also be migrated to the wiki, with links back to the main thread, to provide a basic known open items interface - something that I imagine would make corporate acceptance of PostgreSQL more likely. I don't see where there are a considerably large number of these unresolved items - most things do indeed get fixed or explained away as normal user learning. Sorry for the digression but it seems relevant. David J. -- View this message in context: http://postgresql.1045698.n5.nabble.com/Transaction-lifespan-memory-leak-with-plpgsql-DO-blocks-tp5777942p5778001.html Sent from the PostgreSQL - hackers mailing list archive at Nabble.com. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
