Обсуждение: Curing plpgsql's memory leaks for statement-lifespan values
In https://www.postgresql.org/message-id/tencent_5C738ECA65BAD6861AA43E8F@qq.com it was pointed out that you could get an intra-function-call memory leak from something like LOOP BEGIN EXECUTE 'bogus command'; EXCEPTION WHEN OTHERS THEN END;END LOOP; The problem is that exec_stmt_dynexecute() loses control to the error thrown from the bogus command, and therefore leaks its "querystr" local variable --- which is not much of a leak, but it adds up if the loop iterates enough times. There are similar problems in many other places in plpgsql. Basically the issue is that while running a plpgsql function, CurrentMemoryContext points to a function-lifespan context (the same as the SPI procCxt the function is using). We also store things such as values of the function's variables there, so just resetting that context is not an option. plpgsql does have an expression-evaluation-lifespan context for short-lived stuff, but anything that needs to live for more or less the duration of a statement is put into the procedure-lifespan context, where it risks becoming a leak. (That design was fine originally, because any error would result in abandoning function execution and thereby cleaning up that context. But once we invented plpgsql exception blocks, it's not so good.) One way we could resolve the problem is to require all plpgsql code to use PG_TRY/PG_CATCH blocks to ensure that statement-lifespan variables are explicitly released. That's undesirable on pretty much every front though: it'd be notationally ugly, prone to omissions, and not very speedy. Another answer is to invent a third per-function memory context intended to hold statement-lifespan variables. We could say that statements are still expected to clean up their mess in normal cases, and this context is only cleared when BEGIN/EXCEPT catches an error. Each BEGIN/EXCEPT would need to create and install a new such context, since otherwise it might be clobbering statement-lifespan variables from a surrounding statement. A more aggressive variant is to arrange things so we can automatically reset that context after each statement, avoiding the need for explicit pfree's of statement-lifespan variables. I think that would mean that not only BEGINs but also looping statements would need to install new contexts for their controlled statements, unless they had no pass-by-reference local values. The traffic for creating and deleting such contexts might be too expensive, even though it'd buy back some retail pfree's. I looked into whether we could actually install such a context as CurrentMemoryContext, allowing palloc's to go there by default. This seems not workable: a lot of the existing palloc's really need to be creating function-lifespan values, and there's also a problem that SPI expects CurrentMemoryContext to be the same as its procCxt, because many SPI functions will just set CurrentMemoryContext to equal procCxt at exit. So in any case, use of such a context would require explicit MemoryContextAllocs or MemoryContextSwitchTos, which is a bit annoying notationally, but there seems little help for it. Thoughts, better ideas? regards, tom lane
On 22 July 2016 at 07:02, Tom Lane <tgl@sss.pgh.pa.us> wrote:
...
In
https://www.postgresql.org/message-id/tencent_5C738ECA65BAD6861AA43E8F@qq.com
it was pointed out that you could get an intra-function-call memory leak
from something like
LOOP
BEGIN
EXECUTE 'bogus command';
EXCEPTION WHEN OTHERS THEN
END;
END LOOP;
...
Another answer is to invent a third per-function memory context intended
to hold statement-lifespan variables.
I've wanted this in the past and been surprised we don't have it, so +1 from me.
I don't think a few MemoryContextAlloc's are too ugly.
I suggest that in cassert builds, or maybe just CACHE_CLOBBER_ALWAYS, the context is reset after each call even when not cleaning up after an error. That'll help catch mistakes and leaks.
On 22 July 2016 at 13:24, Craig Ringer <craig@2ndquadrant.com> wrote:
On 22 July 2016 at 07:02, Tom Lane <tgl@sss.pgh.pa.us> wrote:In
https://www.postgresql.org/message-id/tencent_5C738ECA65BAD6861AA43E8F@qq.com
it was pointed out that you could get an intra-function-call memory leak
from something like
LOOP
BEGIN
EXECUTE 'bogus command';
EXCEPTION WHEN OTHERS THEN
END;
END LOOP;
...
Another answer is to invent a third per-function memory context intended
to hold statement-lifespan variables.I've wanted this in the past and been surprised we don't have it, so +1 from me.I don't think a few MemoryContextAlloc's are too ugly.I suggest that in cassert builds, or maybe just CACHE_CLOBBER_ALWAYS, the context is reset after each call even when not cleaning up after an error. That'll help catch mistakes and leaks.
Craig Ringer <craig@2ndquadrant.com> writes:
> On 22 July 2016 at 13:24, Craig Ringer <craig@2ndquadrant.com> wrote:
>> I suggest that in cassert builds, or maybe just CACHE_CLOBBER_ALWAYS, the
>> context is reset after each call even when not cleaning up after an error.
>> That'll help catch mistakes and leaks.
> That is, after each statement.
Well, the issue is with nested statements: if some outer statement has
data in such a context, you can't just clobber it because of a failure in
an inner statement. To make that work, you need a new context for each
nesting level. Once we've gone to that much trouble, I'm inclined to
think we should just do the resets always (ie, not only in debug builds).
That will let us get rid of retail pfree's and buy back at least some of
the added cost.
I've not done any detail work on this idea yet, but at least some of
plpgsql's loop control statements don't have any pass-by-ref local data;
exec_stmt_fori, for example. So it seems possible for that function not
to bother with creating a new context for its child statements. That
would save some more cycles, at the cost of a possibly-confusing-and-
fragile difference in the way different kinds of loops work.
In any case, I'm thinking this is too invasive, relative to its value,
for a back-patch; but it might be okay to sneak it into beta4 if I can
get it done soon. Or we could let it slide till 9.7^H^H^H10. Comments?
regards, tom lane
On Fri, Jul 22, 2016 at 4:32 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > In > https://www.postgresql.org/message-id/tencent_5C738ECA65BAD6861AA43E8F@qq.com > it was pointed out that you could get an intra-function-call memory leak > from something like > > LOOP > BEGIN > EXECUTE 'bogus command'; > EXCEPTION WHEN OTHERS THEN > END; > END LOOP; > > The problem is that exec_stmt_dynexecute() loses control to the error > thrown from the bogus command, and therefore leaks its "querystr" local > variable --- which is not much of a leak, but it adds up if the loop > iterates enough times. There are similar problems in many other places in > plpgsql. Basically the issue is that while running a plpgsql function, > CurrentMemoryContext points to a function-lifespan context (the same as > the SPI procCxt the function is using). We also store things such as > values of the function's variables there, so just resetting that context > is not an option. plpgsql does have an expression-evaluation-lifespan > context for short-lived stuff, but anything that needs to live for more > or less the duration of a statement is put into the procedure-lifespan > context, where it risks becoming a leak. (That design was fine > originally, because any error would result in abandoning function > execution and thereby cleaning up that context. But once we invented > plpgsql exception blocks, it's not so good.) > Wouldn't it be better, if each nested sub-block (which is having an exception) has a separate "SPI Proc", "SPI Exec" contexts which would be destroyed at sub-block end (either commit or rollback)? I think one difficulty could be that we need some way to propagate the information that is required by outer blocks, if there exists any such information. > One way we could resolve the problem is to require all plpgsql code to > use PG_TRY/PG_CATCH blocks to ensure that statement-lifespan variables > are explicitly released. That's undesirable on pretty much every front > though: it'd be notationally ugly, prone to omissions, and not very > speedy. > > Another answer is to invent a third per-function memory context intended > to hold statement-lifespan variables. > This sounds better than spreading PG_TRY/PG_CATCH everywhere. I think if this allocation would have been done in executor context "SPI Exec", then it wouldn't have leaked. One way could have been that by default all exec_stmt* functions execute in "SPI Exec" context and we then switch to "SPI Proc" for the memory that is required for longer duration. I think that might not be good, if we have to switch at many places, but OTOH the same will be required for a new statement-level execution context as well. In short, why do you think it is better to create a new context rather than using "SPI Exec"? -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
<div dir="ltr"><div class="gmail_extra"><br /><div class="gmail_quote">On Sun, Jul 24, 2016 at 12:40 PM, Amit Kapila
<spandir="ltr"><<a href="mailto:amit.kapila16@gmail.com" target="_blank">amit.kapila16@gmail.com</a>></span>
wrote:<br/><blockquote class="gmail_quote" style="margin:0px 0px 0px
0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex"><divclass=""
id=":5jg">Inshort, why do you think<br /> it is better to create a new context rather than using "SPI Exec"?<div
class=""><divclass="" id=":42q" tabindex="0"></div></div></div></blockquote></div><br />I think life span of the memory
allocatedfrom <span style="font-size:12.8px">"SPI Exec" is only within "Executor", and after that
SPI_Exec</span></div><divclass="gmail_extra"><span style="font-size:12.8px">will be reset. But many places we need such
memorybeyond "Executor"</span><span style="font-size:12.8px">(including one which is reported in above
issue).</span></div><divclass="gmail_extra"><span style="font-size:12.8px"><br /></span></div><div
class="gmail_extra"><spanstyle="font-size:12.8px">If we see below example of </span>exec_stmt_dynexecute.</div><div
class="gmail_extra">exec_stmt_dynexecute<br/></div><div class="gmail_extra"><p class="">{<p class="">....<p class=""><p
class="">querystr= pstrdup(querystr);<p class="">SPI_Execute --> inside this SPI_Exec context will be reset.<p
class="">Afterthis querystr is being used in this function.<p class="">}</div><div class="gmail_extra">-- <br /><div
class="gmail_signature"data-smartmail="gmail_signature"><div dir="ltr"><span
style="color:rgb(80,0,80);font-size:12.8px">Regards,</span><brstyle="color:rgb(80,0,80);font-size:12.8px" /><span
style="color:rgb(80,0,80);font-size:12.8px">DilipKumar</span><br style="color:rgb(80,0,80);font-size:12.8px" /><span
style="color:rgb(80,0,80);font-size:12.8px">EnterpriseDB: </span><ahref="http://www.enterprisedb.com/"
style="color:rgb(17,85,204);font-size:12.8px"target="_blank">http://www.enterprisedb.com</a><br
/></div></div></div></div>
Amit Kapila <amit.kapila16@gmail.com> writes:
> On Fri, Jul 22, 2016 at 4:32 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> The problem is that exec_stmt_dynexecute() loses control to the error
>> thrown from the bogus command, and therefore leaks its "querystr" local
>> variable --- which is not much of a leak, but it adds up if the loop
>> iterates enough times. There are similar problems in many other places in
>> plpgsql. Basically the issue is that while running a plpgsql function,
>> CurrentMemoryContext points to a function-lifespan context (the same as
>> the SPI procCxt the function is using). We also store things such as
>> values of the function's variables there, so just resetting that context
>> is not an option. plpgsql does have an expression-evaluation-lifespan
>> context for short-lived stuff, but anything that needs to live for more
>> or less the duration of a statement is put into the procedure-lifespan
>> context, where it risks becoming a leak.
> Wouldn't it be better, if each nested sub-block (which is having an
> exception) has a separate "SPI Proc", "SPI Exec" contexts which would
> be destroyed at sub-block end (either commit or rollback)?
AFAICS that would just confuse matters. In the first place, plpgsql
variable values are not subtransaction-local, so they'd have to live in
the outermost SPI Proc context anyway. In the second place, spi.c
contains a whole lot of assumptions that actions like saving a plan are
tied to the current SPI Proc/Exec contexts, so SPI-using plpgsql
statements that were nested inside a BEGIN/EXCEPT would probably break:
state they expect to remain valid from one execution to the next would
disappear.
> In short, why do you think it is better to create a new context rather
> than using "SPI Exec"?
"SPI Exec" has the same problem as the eval_econtext: there are already
points at which it will be reset, and those can't necessarily be delayed
till end of statement. In particular, _SPI_end_call will delete whatever
is in that context. Also, spi.c does not consider the execCxt to be
an exported part of its abstraction, and I'm pretty loath to punch
another hole in that API.
Also, as I've been working through this, I've found that only a rather
small minority of plpgsql statements actually need statement-lifetime
storage. So I'm thinking that it will be faster to create such a context
only on-demand, not unconditionally; which knocks out any thought of
changing plpgsql's coding conventions so much that statement-lifespan
storage would become the normal place for CurrentMemoryContext to point.
regards, tom lane
On Sun, Jul 24, 2016 at 9:17 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Amit Kapila <amit.kapila16@gmail.com> writes: > >> Wouldn't it be better, if each nested sub-block (which is having an >> exception) has a separate "SPI Proc", "SPI Exec" contexts which would >> be destroyed at sub-block end (either commit or rollback)? > > AFAICS that would just confuse matters. In the first place, plpgsql > variable values are not subtransaction-local, so they'd have to live in > the outermost SPI Proc context anyway. In the second place, spi.c > contains a whole lot of assumptions that actions like saving a plan are > tied to the current SPI Proc/Exec contexts, so SPI-using plpgsql > statements that were nested inside a BEGIN/EXCEPT would probably break: > state they expect to remain valid from one execution to the next would > disappear. > I think for all such usage, we can always switch to top level SPI Proc/Exec context. To do so, we might need to invent a notion of something like Sub SPI Proc/Exec contexts and that sounds quite tricky. >> In short, why do you think it is better to create a new context rather >> than using "SPI Exec"? > > "SPI Exec" has the same problem as the eval_econtext: there are already > points at which it will be reset, and those can't necessarily be delayed > till end of statement. In particular, _SPI_end_call will delete whatever > is in that context. > That's right and I think it might not be even feasible to do so, mainly because that is done in exposed SPI calls. I have checked some other usage of SPI, it seems plperl is handling some similar problems either via creating temporary work context or by using PG_TRY/PG_CATCH. I think it might be better if, whatever we are trying here could be of help for other similar usage. -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
Attached is a draft patch for creating statement-level temporary memory
contexts in plpgsql. It creates such contexts only as needed, and there
are a lot of simpler plpgsql statements that don't need them, so I think
the performance impact should be pretty minimal --- though I've not tried
to measure it, since I don't have a credible benchmark for overall plpgsql
performance.
This fixes the originally complained-of leak and several others besides,
including at least one path that leaked function-lifespan memory even
without an error :-(.
In addition to the patch proper, I attach for amusement's sake some
additional hacking I did for testing purposes, to make sure I'd found and
accounted for all the places that were allocating memory in the SPI Proc
context. There's a glibc-dependent hack in aset.c that reports any
plpgsql-driven palloc or pfree against a context named "SPI Proc", as
well as changes in pl_comp.c so that transient junk created during initial
parsing of a plpgsql function body doesn't end up in the SPI Proc context.
(I did that just to cut the amount of noise I had to chase down. I suppose
in large functions it might be worth adopting such a change so that that
junk can be released immediately after parsing; but I suspect for most
functions it'd just be an extra context without much gain.)
Although this is in principle a bug fix, it's invasive enough that I'd
be pretty hesitant to back-patch it, or even to stick it into HEAD
post-beta. I'm inclined to sign it up for the next commitfest instead.
regards, tom lane
diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c
index 586ff1f..e23ca96 100644
*** a/src/pl/plpgsql/src/pl_exec.c
--- b/src/pl/plpgsql/src/pl_exec.c
*************** typedef struct
*** 48,54 ****
Oid *types; /* types of arguments */
Datum *values; /* evaluated argument values */
char *nulls; /* null markers (' '/'n' style) */
- bool *freevals; /* which arguments are pfree-able */
} PreparedParamsData;
/*
--- 48,53 ----
*************** static EState *shared_simple_eval_estate
*** 88,93 ****
--- 87,122 ----
static SimpleEcontextStackEntry *simple_econtext_stack = NULL;
/*
+ * Memory management within a plpgsql function generally works with three
+ * contexts:
+ *
+ * 1. Function-call-lifespan data, such as variable values, is kept in the
+ * "main" context, a/k/a the "SPI Proc" context established by SPI_connect().
+ * This is usually the CurrentMemoryContext while running code in this module
+ * (which is not good, because careless coding can easily cause
+ * function-lifespan memory leaks, but we live with it for now).
+ *
+ * 2. Some statement-execution routines need statement-lifespan workspace.
+ * A suitable context is created on-demand by get_stmt_mcontext(), and must
+ * be reset at the end of the requesting routine. Error recovery will clean
+ * it up automatically. Nested statements requiring statement-lifespan
+ * workspace will result in a stack of such contexts, see push_stmt_mcontext().
+ *
+ * 3. We use the eval_econtext's per-tuple memory context for expression
+ * evaluation, and as a general-purpose workspace for short-lived allocations.
+ * Such allocations usually aren't explicitly freed, but are left to be
+ * cleaned up by a context reset, typically done by exec_eval_cleanup().
+ *
+ * These macros are for use in making short-lived allocations:
+ */
+ #define get_eval_mcontext(estate) \
+ ((estate)->eval_econtext->ecxt_per_tuple_memory)
+ #define eval_mcontext_alloc(estate, sz) \
+ MemoryContextAlloc(get_eval_mcontext(estate), sz)
+ #define eval_mcontext_alloc0(estate, sz) \
+ MemoryContextAllocZero(get_eval_mcontext(estate), sz)
+
+ /*
* We use a session-wide hash table for caching cast information.
*
* Once built, the compiled expression trees (cast_expr fields) survive for
*************** static HTAB *shared_cast_hash = NULL;
*** 128,133 ****
--- 157,165 ----
************************************************************/
static void plpgsql_exec_error_callback(void *arg);
static PLpgSQL_datum *copy_plpgsql_datum(PLpgSQL_datum *datum);
+ static MemoryContext get_stmt_mcontext(PLpgSQL_execstate *estate);
+ static void push_stmt_mcontext(PLpgSQL_execstate *estate);
+ static void pop_stmt_mcontext(PLpgSQL_execstate *estate);
static int exec_stmt_block(PLpgSQL_execstate *estate,
PLpgSQL_stmt_block *block);
*************** static void exec_eval_cleanup(PLpgSQL_ex
*** 191,197 ****
static void exec_prepare_plan(PLpgSQL_execstate *estate,
PLpgSQL_expr *expr, int cursorOptions);
static bool exec_simple_check_node(Node *node);
! static void exec_simple_check_plan(PLpgSQL_expr *expr);
static void exec_simple_recheck_plan(PLpgSQL_expr *expr, CachedPlan *cplan);
static void exec_check_rw_parameter(PLpgSQL_expr *expr, int target_dno);
static bool contains_target_param(Node *node, int *target_dno);
--- 223,229 ----
static void exec_prepare_plan(PLpgSQL_execstate *estate,
PLpgSQL_expr *expr, int cursorOptions);
static bool exec_simple_check_node(Node *node);
! static void exec_simple_check_plan(PLpgSQL_execstate *estate, PLpgSQL_expr *expr);
static void exec_simple_recheck_plan(PLpgSQL_expr *expr, CachedPlan *cplan);
static void exec_check_rw_parameter(PLpgSQL_expr *expr, int target_dno);
static bool contains_target_param(Node *node, int *target_dno);
*************** static void assign_text_var(PLpgSQL_exec
*** 271,281 ****
const char *str);
static PreparedParamsData *exec_eval_using_params(PLpgSQL_execstate *estate,
List *params);
- static void free_params_data(PreparedParamsData *ppd);
static Portal exec_dynquery_with_params(PLpgSQL_execstate *estate,
PLpgSQL_expr *dynquery, List *params,
const char *portalname, int cursorOptions);
-
static char *format_expr_params(PLpgSQL_execstate *estate,
const PLpgSQL_expr *expr);
static char *format_preparedparamsdata(PLpgSQL_execstate *estate,
--- 303,311 ----
*************** plpgsql_exec_function(PLpgSQL_function *
*** 562,567 ****
--- 592,598 ----
/* Clean up any leftover temporary memory */
plpgsql_destroy_econtext(&estate);
exec_eval_cleanup(&estate);
+ /* stmt_mcontext will be destroyed when function's main context is */
/*
* Pop the error context stack
*************** plpgsql_exec_trigger(PLpgSQL_function *f
*** 832,837 ****
--- 863,869 ----
/* Clean up any leftover temporary memory */
plpgsql_destroy_econtext(&estate);
exec_eval_cleanup(&estate);
+ /* stmt_mcontext will be destroyed when function's main context is */
/*
* Pop the error context stack
*************** plpgsql_exec_trigger(PLpgSQL_function *f
*** 844,849 ****
--- 876,886 ----
return rettup;
}
+ /* ----------
+ * plpgsql_exec_event_trigger Called by the call handler for
+ * event trigger execution.
+ * ----------
+ */
void
plpgsql_exec_event_trigger(PLpgSQL_function *func, EventTriggerData *trigdata)
{
*************** plpgsql_exec_event_trigger(PLpgSQL_funct
*** 915,920 ****
--- 952,958 ----
/* Clean up any leftover temporary memory */
plpgsql_destroy_econtext(&estate);
exec_eval_cleanup(&estate);
+ /* stmt_mcontext will be destroyed when function's main context is */
/*
* Pop the error context stack
*************** copy_plpgsql_datum(PLpgSQL_datum *datum)
*** 1041,1047 ****
--- 1079,1142 ----
return result;
}
+ /*
+ * Create a memory context for statement-lifespan variables, if we don't
+ * have one already. It will be a child of stmt_mcontext_parent, which is
+ * either the function's main context or a pushed-down outer stmt_mcontext.
+ */
+ static MemoryContext
+ get_stmt_mcontext(PLpgSQL_execstate *estate)
+ {
+ if (estate->stmt_mcontext == NULL)
+ {
+ estate->stmt_mcontext =
+ AllocSetContextCreate(estate->stmt_mcontext_parent,
+ "PLpgSQL per-statement data",
+ ALLOCSET_DEFAULT_MINSIZE,
+ ALLOCSET_DEFAULT_INITSIZE,
+ ALLOCSET_DEFAULT_MAXSIZE);
+ }
+ return estate->stmt_mcontext;
+ }
+
+ /*
+ * Push down the current stmt_mcontext so that called statements won't use it.
+ * This is needed by statements that have statement-lifespan data and need to
+ * preserve it across some inner statements. The caller should eventually do
+ * pop_stmt_mcontext().
+ */
+ static void
+ push_stmt_mcontext(PLpgSQL_execstate *estate)
+ {
+ /* Should have done get_stmt_mcontext() first */
+ Assert(estate->stmt_mcontext != NULL);
+ /* Assert we've not messed up the stack linkage */
+ Assert(MemoryContextGetParent(estate->stmt_mcontext) == estate->stmt_mcontext_parent);
+ /* Push it down to become the parent of any nested stmt mcontext */
+ estate->stmt_mcontext_parent = estate->stmt_mcontext;
+ /* And make it not available for use directly */
+ estate->stmt_mcontext = NULL;
+ }
+
+ /*
+ * Undo push_stmt_mcontext(). We assume this is done just before or after
+ * resetting the caller's stmt_mcontext; since that action will also delete
+ * any child contexts, there's no need to explicitly delete whatever context
+ * might currently be estate->stmt_mcontext.
+ */
+ static void
+ pop_stmt_mcontext(PLpgSQL_execstate *estate)
+ {
+ /* We need only pop the stack */
+ estate->stmt_mcontext = estate->stmt_mcontext_parent;
+ estate->stmt_mcontext_parent = MemoryContextGetParent(estate->stmt_mcontext);
+ }
+
+ /*
+ * Subroutine for exec_stmt_block: does any condition in the condition list
+ * match the current exception?
+ */
static bool
exception_matches_conditions(ErrorData *edata, PLpgSQL_condition *cond)
{
*************** exec_stmt_block(PLpgSQL_execstate *estat
*** 1174,1182 ****
--- 1269,1289 ----
ResourceOwner oldowner = CurrentResourceOwner;
ExprContext *old_eval_econtext = estate->eval_econtext;
ErrorData *save_cur_error = estate->cur_error;
+ MemoryContext stmt_mcontext;
estate->err_text = gettext_noop("during statement block entry");
+ /*
+ * We will need a stmt_mcontext to hold the error data if an error
+ * occurs. It seems best to force it to exist before entering the
+ * subtransaction, so that we reduce the risk of out-of-memory during
+ * error recovery, and because this greatly simplifies restoring the
+ * stmt_mcontext stack to the correct state after an error. We can
+ * ameliorate the cost of this by allowing the called statements to
+ * use this mcontext too; so we don't push it down here.
+ */
+ stmt_mcontext = get_stmt_mcontext(estate);
+
BeginInternalSubTransaction(NULL);
/* Want to run statements inside function's memory context */
MemoryContextSwitchTo(oldcontext);
*************** exec_stmt_block(PLpgSQL_execstate *estat
*** 1202,1208 ****
* If the block ended with RETURN, we may need to copy the return
* value out of the subtransaction eval_context. This is
* currently only needed for scalar result types --- rowtype
! * values will always exist in the function's own memory context.
*/
if (rc == PLPGSQL_RC_RETURN &&
!estate->retisset &&
--- 1309,1317 ----
* If the block ended with RETURN, we may need to copy the return
* value out of the subtransaction eval_context. This is
* currently only needed for scalar result types --- rowtype
! * values will always exist in the function's main memory context,
! * cf. exec_stmt_return(). We can avoid a physical copy if the
! * value happens to be a R/W expanded object.
*/
if (rc == PLPGSQL_RC_RETURN &&
!estate->retisset &&
*************** exec_stmt_block(PLpgSQL_execstate *estat
*** 1213,1220 ****
bool resTypByVal;
get_typlenbyval(estate->rettype, &resTypLen, &resTypByVal);
! estate->retval = datumCopy(estate->retval,
! resTypByVal, resTypLen);
}
/* Commit the inner transaction, return to outer xact context */
--- 1322,1329 ----
bool resTypByVal;
get_typlenbyval(estate->rettype, &resTypLen, &resTypByVal);
! estate->retval = datumTransfer(estate->retval,
! resTypByVal, resTypLen);
}
/* Commit the inner transaction, return to outer xact context */
*************** exec_stmt_block(PLpgSQL_execstate *estat
*** 1222,1227 ****
--- 1331,1339 ----
MemoryContextSwitchTo(oldcontext);
CurrentResourceOwner = oldowner;
+ /* Assert that the stmt_mcontext stack is unchanged */
+ Assert(stmt_mcontext == estate->stmt_mcontext);
+
/*
* Revert to outer eval_econtext. (The inner one was
* automatically cleaned up during subxact exit.)
*************** exec_stmt_block(PLpgSQL_execstate *estat
*** 1241,1248 ****
estate->err_text = gettext_noop("during exception cleanup");
! /* Save error info */
! MemoryContextSwitchTo(oldcontext);
edata = CopyErrorData();
FlushErrorState();
--- 1353,1360 ----
estate->err_text = gettext_noop("during exception cleanup");
! /* Save error info in our stmt_mcontext */
! MemoryContextSwitchTo(stmt_mcontext);
edata = CopyErrorData();
FlushErrorState();
*************** exec_stmt_block(PLpgSQL_execstate *estat
*** 1251,1256 ****
--- 1363,1388 ----
MemoryContextSwitchTo(oldcontext);
CurrentResourceOwner = oldowner;
+ /*
+ * Set up the stmt_mcontext stack as though we had restored our
+ * previous state and then done push_stmt_mcontext(). The push is
+ * needed so that statements in the exception handler won't
+ * clobber the error data that's in our stmt_mcontext.
+ */
+ estate->stmt_mcontext_parent = stmt_mcontext;
+ estate->stmt_mcontext = NULL;
+
+ /*
+ * Now we can delete any nested stmt_mcontexts that might have
+ * been created as children of ours. (Note: we do not immediately
+ * release any statement-lifespan data that might have been left
+ * behind in stmt_mcontext itself. We could attempt that by doing
+ * a MemoryContextReset on it before collecting the error data
+ * above, but it seems too risky to do any significant amount of
+ * work before collecting the error.)
+ */
+ MemoryContextDeleteChildren(stmt_mcontext);
+
/* Revert to outer eval_econtext */
estate->eval_econtext = old_eval_econtext;
*************** exec_stmt_block(PLpgSQL_execstate *estat
*** 1319,1326 ****
/* If no match found, re-throw the error */
if (e == NULL)
ReThrowError(edata);
! else
! FreeErrorData(edata);
}
PG_END_TRY();
--- 1451,1460 ----
/* If no match found, re-throw the error */
if (e == NULL)
ReThrowError(edata);
!
! /* Restore stmt_mcontext stack and release the error data */
! pop_stmt_mcontext(estate);
! MemoryContextReset(stmt_mcontext);
}
PG_END_TRY();
*************** exec_stmt_getdiag(PLpgSQL_execstate *est
*** 1663,1673 ****
case PLPGSQL_GETDIAG_CONTEXT:
{
! char *contextstackstr = GetErrorContextStack();
! exec_assign_c_string(estate, var, contextstackstr);
! pfree(contextstackstr);
}
break;
--- 1797,1811 ----
case PLPGSQL_GETDIAG_CONTEXT:
{
! char *contextstackstr;
! MemoryContext oldcontext;
! /* Use eval_mcontext for short-lived string */
! oldcontext = MemoryContextSwitchTo(get_eval_mcontext(estate));
! contextstackstr = GetErrorContextStack();
! MemoryContextSwitchTo(oldcontext);
! exec_assign_c_string(estate, var, contextstackstr);
}
break;
*************** exec_stmt_getdiag(PLpgSQL_execstate *est
*** 1677,1682 ****
--- 1815,1822 ----
}
}
+ exec_eval_cleanup(estate);
+
return PLPGSQL_RC_OK;
}
*************** exec_stmt_case(PLpgSQL_execstate *estate
*** 1738,1744 ****
/*
* When expected datatype is different from real, change it. Note that
* what we're modifying here is an execution copy of the datum, so
! * this doesn't affect the originally stored function parse tree.
*/
if (t_var->datatype->typoid != t_typoid ||
t_var->datatype->atttypmod != t_typmod)
--- 1878,1887 ----
/*
* When expected datatype is different from real, change it. Note that
* what we're modifying here is an execution copy of the datum, so
! * this doesn't affect the originally stored function parse tree. (In
! * theory, if the expression datatype keeps changing during execution,
! * this could cause a function-lifespan memory leak. Doesn't seem
! * worth worrying about though.)
*/
if (t_var->datatype->typoid != t_typoid ||
t_var->datatype->atttypmod != t_typmod)
*************** static int
*** 2132,2137 ****
--- 2275,2281 ----
exec_stmt_forc(PLpgSQL_execstate *estate, PLpgSQL_stmt_forc *stmt)
{
PLpgSQL_var *curvar;
+ MemoryContext stmt_mcontext = NULL;
char *curname = NULL;
PLpgSQL_expr *query;
ParamListInfo paramLI;
*************** exec_stmt_forc(PLpgSQL_execstate *estate
*** 2146,2152 ****
--- 2290,2303 ----
curvar = (PLpgSQL_var *) (estate->datums[stmt->curvar]);
if (!curvar->isnull)
{
+ MemoryContext oldcontext;
+
+ /* We only need stmt_mcontext to hold the cursor name string */
+ stmt_mcontext = get_stmt_mcontext(estate);
+ oldcontext = MemoryContextSwitchTo(stmt_mcontext);
curname = TextDatumGetCString(curvar->value);
+ MemoryContextSwitchTo(oldcontext);
+
if (SPI_cursor_find(curname) != NULL)
ereport(ERROR,
(errcode(ERRCODE_DUPLICATE_CURSOR),
*************** exec_stmt_forc(PLpgSQL_execstate *estate
*** 2216,2225 ****
elog(ERROR, "could not open cursor: %s",
SPI_result_code_string(SPI_result));
- /* don't need paramlist any more */
- if (paramLI)
- pfree(paramLI);
-
/*
* If cursor variable was NULL, store the generated portal name in it
*/
--- 2367,2372 ----
*************** exec_stmt_forc(PLpgSQL_execstate *estate
*** 2227,2232 ****
--- 2374,2386 ----
assign_text_var(estate, curvar, portal->name);
/*
+ * Clean up before entering exec_for_query
+ */
+ exec_eval_cleanup(estate);
+ if (stmt_mcontext)
+ MemoryContextReset(stmt_mcontext);
+
+ /*
* Execute the loop. We can't prefetch because the cursor is accessible
* to the user, for instance via UPDATE WHERE CURRENT OF within the loop.
*/
*************** exec_stmt_forc(PLpgSQL_execstate *estate
*** 2241,2249 ****
if (curname == NULL)
assign_simple_var(estate, curvar, (Datum) 0, true, false);
- if (curname)
- pfree(curname);
-
return rc;
}
--- 2395,2400 ----
*************** exec_stmt_foreach_a(PLpgSQL_execstate *e
*** 2266,2271 ****
--- 2417,2424 ----
Oid loop_var_elem_type;
bool found = false;
int rc = PLPGSQL_RC_OK;
+ MemoryContext stmt_mcontext;
+ MemoryContext oldcontext;
ArrayIterator array_iterator;
Oid iterator_result_type;
int32 iterator_result_typmod;
*************** exec_stmt_foreach_a(PLpgSQL_execstate *e
*** 2279,2284 ****
--- 2432,2446 ----
(errcode(ERRCODE_NULL_VALUE_NOT_ALLOWED),
errmsg("FOREACH expression must not be null")));
+ /*
+ * Do as much as possible of the code below in stmt_mcontext, to avoid any
+ * leaks from called subroutines. We need a private stmt_mcontext since
+ * we'll be calling arbitrary statement code.
+ */
+ stmt_mcontext = get_stmt_mcontext(estate);
+ push_stmt_mcontext(estate);
+ oldcontext = MemoryContextSwitchTo(stmt_mcontext);
+
/* check the type of the expression - must be an array */
if (!OidIsValid(get_element_type(arrtype)))
ereport(ERROR,
*************** exec_stmt_foreach_a(PLpgSQL_execstate *e
*** 2287,2295 ****
format_type_be(arrtype))));
/*
! * We must copy the array, else it will disappear in exec_eval_cleanup.
! * This is annoying, but cleanup will certainly happen while running the
! * loop body, so we have little choice.
*/
arr = DatumGetArrayTypePCopy(value);
--- 2449,2457 ----
format_type_be(arrtype))));
/*
! * We must copy the array into stmt_mcontext, else it will disappear in
! * exec_eval_cleanup. This is annoying, but cleanup will certainly happen
! * while running the loop body, so we have little choice.
*/
arr = DatumGetArrayTypePCopy(value);
*************** exec_stmt_foreach_a(PLpgSQL_execstate *e
*** 2355,2360 ****
--- 2517,2525 ----
{
found = true; /* looped at least once */
+ /* exec_assign_value and exec_stmts must run in the main context */
+ MemoryContextSwitchTo(oldcontext);
+
/* Assign current element/slice to the loop variable */
exec_assign_value(estate, loop_var, value, isnull,
iterator_result_type, iterator_result_typmod);
*************** exec_stmt_foreach_a(PLpgSQL_execstate *e
*** 2413,2423 ****
break;
}
}
}
/* Release temporary memory, including the array value */
! array_free_iterator(array_iterator);
! pfree(arr);
/*
* Set the FOUND variable to indicate the result of executing the loop
--- 2578,2593 ----
break;
}
}
+
+ MemoryContextSwitchTo(stmt_mcontext);
}
+ /* Restore memory context state */
+ MemoryContextSwitchTo(oldcontext);
+ pop_stmt_mcontext(estate);
+
/* Release temporary memory, including the array value */
! MemoryContextReset(stmt_mcontext);
/*
* Set the FOUND variable to indicate the result of executing the loop
*************** exec_stmt_exit(PLpgSQL_execstate *estate
*** 2465,2470 ****
--- 2635,2647 ----
/* ----------
* exec_stmt_return Evaluate an expression and start
* returning from the function.
+ *
+ * Note: in the retistuple code paths, the returned tuple is always in the
+ * function's main context, whereas for non-tuple data types the result may
+ * be in the eval_mcontext. The former case is not a memory leak since we're
+ * about to exit the function anyway. (If you want to change it, note that
+ * exec_stmt_block() knows about this behavior.) The latter case means that
+ * we must not do exec_eval_cleanup while unwinding the control stack.
* ----------
*/
static int
*************** exec_stmt_return_next(PLpgSQL_execstate
*** 2639,2646 ****
{
TupleDesc tupdesc;
int natts;
! HeapTuple tuple = NULL;
! bool free_tuple = false;
if (!estate->retisset)
ereport(ERROR,
--- 2816,2823 ----
{
TupleDesc tupdesc;
int natts;
! HeapTuple tuple;
! MemoryContext oldcontext;
if (!estate->retisset)
ereport(ERROR,
*************** exec_stmt_return_next(PLpgSQL_execstate
*** 2712,2728 ****
rec->refname),
errdetail("The tuple structure of a not-yet-assigned"
" record is indeterminate.")));
tupmap = convert_tuples_by_position(rec->tupdesc,
tupdesc,
gettext_noop("wrong record type supplied in RETURN NEXT"));
tuple = rec->tup;
- /* it might need conversion */
if (tupmap)
- {
tuple = do_convert_tuple(tuple, tupmap);
! free_conversion_map(tupmap);
! free_tuple = true;
! }
}
break;
--- 2889,2905 ----
rec->refname),
errdetail("The tuple structure of a not-yet-assigned"
" record is indeterminate.")));
+
+ /* Use eval_mcontext for tuple conversion work */
+ oldcontext = MemoryContextSwitchTo(get_eval_mcontext(estate));
tupmap = convert_tuples_by_position(rec->tupdesc,
tupdesc,
gettext_noop("wrong record type supplied in RETURN NEXT"));
tuple = rec->tup;
if (tupmap)
tuple = do_convert_tuple(tuple, tupmap);
! tuplestore_puttuple(estate->tuple_store, tuple);
! MemoryContextSwitchTo(oldcontext);
}
break;
*************** exec_stmt_return_next(PLpgSQL_execstate
*** 2730,2741 ****
{
PLpgSQL_row *row = (PLpgSQL_row *) retvar;
tuple = make_tuple_from_row(estate, row, tupdesc);
if (tuple == NULL)
ereport(ERROR,
(errcode(ERRCODE_DATATYPE_MISMATCH),
errmsg("wrong record type supplied in RETURN NEXT")));
! free_tuple = true;
}
break;
--- 2907,2921 ----
{
PLpgSQL_row *row = (PLpgSQL_row *) retvar;
+ /* Use eval_mcontext for tuple conversion work */
+ oldcontext = MemoryContextSwitchTo(get_eval_mcontext(estate));
tuple = make_tuple_from_row(estate, row, tupdesc);
if (tuple == NULL)
ereport(ERROR,
(errcode(ERRCODE_DATATYPE_MISMATCH),
errmsg("wrong record type supplied in RETURN NEXT")));
! tuplestore_puttuple(estate->tuple_store, tuple);
! MemoryContextSwitchTo(oldcontext);
}
break;
*************** exec_stmt_return_next(PLpgSQL_execstate
*** 2770,2793 ****
(errcode(ERRCODE_DATATYPE_MISMATCH),
errmsg("cannot return non-composite value from function returning composite type")));
tuple = get_tuple_from_datum(retval);
- free_tuple = true; /* tuple is always freshly palloc'd */
-
- /* it might need conversion */
retvaldesc = get_tupdesc_from_datum(retval);
tupmap = convert_tuples_by_position(retvaldesc, tupdesc,
gettext_noop("returned record type does not match expected record
type"));
if (tupmap)
! {
! HeapTuple newtuple;
!
! newtuple = do_convert_tuple(tuple, tupmap);
! free_conversion_map(tupmap);
! heap_freetuple(tuple);
! tuple = newtuple;
! }
ReleaseTupleDesc(retvaldesc);
! /* tuple will be stored into tuplestore below */
}
else
{
--- 2950,2966 ----
(errcode(ERRCODE_DATATYPE_MISMATCH),
errmsg("cannot return non-composite value from function returning composite type")));
+ /* Use eval_mcontext for tuple conversion work */
+ oldcontext = MemoryContextSwitchTo(get_eval_mcontext(estate));
tuple = get_tuple_from_datum(retval);
retvaldesc = get_tupdesc_from_datum(retval);
tupmap = convert_tuples_by_position(retvaldesc, tupdesc,
gettext_noop("returned record type does not match expected record
type"));
if (tupmap)
! tuple = do_convert_tuple(tuple, tupmap);
! tuplestore_puttuple(estate->tuple_store, tuple);
ReleaseTupleDesc(retvaldesc);
! MemoryContextSwitchTo(oldcontext);
}
else
{
*************** exec_stmt_return_next(PLpgSQL_execstate
*** 2795,2807 ****
Datum *nulldatums;
bool *nullflags;
! nulldatums = (Datum *) palloc0(natts * sizeof(Datum));
! nullflags = (bool *) palloc(natts * sizeof(bool));
memset(nullflags, true, natts * sizeof(bool));
tuplestore_putvalues(estate->tuple_store, tupdesc,
nulldatums, nullflags);
- pfree(nulldatums);
- pfree(nullflags);
}
}
else
--- 2968,2980 ----
Datum *nulldatums;
bool *nullflags;
! nulldatums = (Datum *)
! eval_mcontext_alloc0(estate, natts * sizeof(Datum));
! nullflags = (bool *)
! eval_mcontext_alloc(estate, natts * sizeof(bool));
memset(nullflags, true, natts * sizeof(bool));
tuplestore_putvalues(estate->tuple_store, tupdesc,
nulldatums, nullflags);
}
}
else
*************** exec_stmt_return_next(PLpgSQL_execstate
*** 2832,2845 ****
errmsg("RETURN NEXT must have a parameter")));
}
- if (HeapTupleIsValid(tuple))
- {
- tuplestore_puttuple(estate->tuple_store, tuple);
-
- if (free_tuple)
- heap_freetuple(tuple);
- }
-
exec_eval_cleanup(estate);
return PLPGSQL_RC_OK;
--- 3005,3010 ----
*************** exec_stmt_return_query(PLpgSQL_execstate
*** 2858,2863 ****
--- 3023,3029 ----
Portal portal;
uint64 processed = 0;
TupleConversionMap *tupmap;
+ MemoryContext oldcontext;
if (!estate->retisset)
ereport(ERROR,
*************** exec_stmt_return_query(PLpgSQL_execstate
*** 2881,2886 ****
--- 3047,3055 ----
CURSOR_OPT_PARALLEL_OK);
}
+ /* Use eval_mcontext for tuple conversion work */
+ oldcontext = MemoryContextSwitchTo(get_eval_mcontext(estate));
+
tupmap = convert_tuples_by_position(portal->tupDesc,
estate->rettupdesc,
gettext_noop("structure of query does not match function result type"));
*************** exec_stmt_return_query(PLpgSQL_execstate
*** 2890,2895 ****
--- 3059,3068 ----
uint64 i;
SPI_cursor_fetch(portal, true, 50);
+
+ /* SPI will have reset CurrentMemoryContext */
+ MemoryContextSwitchTo(get_eval_mcontext(estate));
+
if (SPI_processed == 0)
break;
*************** exec_stmt_return_query(PLpgSQL_execstate
*** 2908,2919 ****
SPI_freetuptable(SPI_tuptable);
}
- if (tupmap)
- free_conversion_map(tupmap);
-
SPI_freetuptable(SPI_tuptable);
SPI_cursor_close(portal);
estate->eval_processed = processed;
exec_set_found(estate, processed != 0);
--- 3081,3092 ----
SPI_freetuptable(SPI_tuptable);
}
SPI_freetuptable(SPI_tuptable);
SPI_cursor_close(portal);
+ MemoryContextSwitchTo(oldcontext);
+ exec_eval_cleanup(estate);
+
estate->eval_processed = processed;
exec_set_found(estate, processed != 0);
*************** do { \
*** 2965,2971 ****
(errcode(ERRCODE_SYNTAX_ERROR), \
errmsg("RAISE option already specified: %s", \
name))); \
! opt = pstrdup(extval); \
} while (0)
/* ----------
--- 3138,3144 ----
(errcode(ERRCODE_SYNTAX_ERROR), \
errmsg("RAISE option already specified: %s", \
name))); \
! opt = MemoryContextStrdup(stmt_mcontext, extval); \
} while (0)
/* ----------
*************** exec_stmt_raise(PLpgSQL_execstate *estat
*** 2985,2990 ****
--- 3158,3164 ----
char *err_datatype = NULL;
char *err_table = NULL;
char *err_schema = NULL;
+ MemoryContext stmt_mcontext;
ListCell *lc;
/* RAISE with no parameters: re-throw current exception */
*************** exec_stmt_raise(PLpgSQL_execstate *estat
*** 2999,3008 ****
errmsg("RAISE without parameters cannot be used outside an exception handler")));
}
if (stmt->condname)
{
err_code = plpgsql_recognize_err_condition(stmt->condname, true);
! condname = pstrdup(stmt->condname);
}
if (stmt->message)
--- 3173,3185 ----
errmsg("RAISE without parameters cannot be used outside an exception handler")));
}
+ /* We'll need to accumulate the various strings in stmt_mcontext */
+ stmt_mcontext = get_stmt_mcontext(estate);
+
if (stmt->condname)
{
err_code = plpgsql_recognize_err_condition(stmt->condname, true);
! condname = MemoryContextStrdup(stmt_mcontext, stmt->condname);
}
if (stmt->message)
*************** exec_stmt_raise(PLpgSQL_execstate *estat
*** 3010,3017 ****
--- 3187,3199 ----
StringInfoData ds;
ListCell *current_param;
char *cp;
+ MemoryContext oldcontext;
+ /* build string in stmt_mcontext */
+ oldcontext = MemoryContextSwitchTo(stmt_mcontext);
initStringInfo(&ds);
+ MemoryContextSwitchTo(oldcontext);
+
current_param = list_head(stmt->params);
for (cp = stmt->message; *cp; cp++)
*************** exec_stmt_raise(PLpgSQL_execstate *estat
*** 3064,3070 ****
elog(ERROR, "unexpected RAISE parameter list length");
err_message = ds.data;
- /* No pfree(ds.data), the pfree(err_message) does it */
}
foreach(lc, stmt->options)
--- 3246,3251 ----
*************** exec_stmt_raise(PLpgSQL_execstate *estat
*** 3096,3102 ****
errmsg("RAISE option already specified: %s",
"ERRCODE")));
err_code = plpgsql_recognize_err_condition(extval, true);
! condname = pstrdup(extval);
break;
case PLPGSQL_RAISEOPTION_MESSAGE:
SET_RAISE_OPTION_TEXT(err_message, "MESSAGE");
--- 3277,3283 ----
errmsg("RAISE option already specified: %s",
"ERRCODE")));
err_code = plpgsql_recognize_err_condition(extval, true);
! condname = MemoryContextStrdup(stmt_mcontext, extval);
break;
case PLPGSQL_RAISEOPTION_MESSAGE:
SET_RAISE_OPTION_TEXT(err_message, "MESSAGE");
*************** exec_stmt_raise(PLpgSQL_execstate *estat
*** 3142,3148 ****
condname = NULL;
}
else
! err_message = pstrdup(unpack_sql_state(err_code));
}
/*
--- 3323,3330 ----
condname = NULL;
}
else
! err_message = MemoryContextStrdup(stmt_mcontext,
! unpack_sql_state(err_code));
}
/*
*************** exec_stmt_raise(PLpgSQL_execstate *estat
*** 3164,3187 ****
(err_schema != NULL) ?
err_generic_string(PG_DIAG_SCHEMA_NAME, err_schema) : 0));
! if (condname != NULL)
! pfree(condname);
! if (err_message != NULL)
! pfree(err_message);
! if (err_detail != NULL)
! pfree(err_detail);
! if (err_hint != NULL)
! pfree(err_hint);
! if (err_column != NULL)
! pfree(err_column);
! if (err_constraint != NULL)
! pfree(err_constraint);
! if (err_datatype != NULL)
! pfree(err_datatype);
! if (err_table != NULL)
! pfree(err_table);
! if (err_schema != NULL)
! pfree(err_schema);
return PLPGSQL_RC_OK;
}
--- 3346,3353 ----
(err_schema != NULL) ?
err_generic_string(PG_DIAG_SCHEMA_NAME, err_schema) : 0));
! /* Clean up transient strings */
! MemoryContextReset(stmt_mcontext);
return PLPGSQL_RC_OK;
}
*************** plpgsql_estate_setup(PLpgSQL_execstate *
*** 3329,3334 ****
--- 3495,3508 ----
estate->cast_hash_context = shared_cast_context;
}
+ /*
+ * We start with no stmt_mcontext; one will be created only if needed.
+ * That context will be a direct child of the function's main execution
+ * context. Additional stmt_mcontexts might be created as children of it.
+ */
+ estate->stmt_mcontext = NULL;
+ estate->stmt_mcontext_parent = CurrentMemoryContext;
+
estate->eval_tuptable = NULL;
estate->eval_processed = 0;
estate->eval_lastoid = InvalidOid;
*************** exec_eval_cleanup(PLpgSQL_execstate *est
*** 3378,3384 ****
SPI_freetuptable(estate->eval_tuptable);
estate->eval_tuptable = NULL;
! /* Clear result of exec_eval_simple_expr (but keep the econtext) */
if (estate->eval_econtext != NULL)
ResetExprContext(estate->eval_econtext);
}
--- 3552,3561 ----
SPI_freetuptable(estate->eval_tuptable);
estate->eval_tuptable = NULL;
! /*
! * Clear result of exec_eval_simple_expr (but keep the econtext). This
! * also clears any short-lived allocations done via get_eval_mcontext.
! */
if (estate->eval_econtext != NULL)
ResetExprContext(estate->eval_econtext);
}
*************** exec_prepare_plan(PLpgSQL_execstate *est
*** 3430,3436 ****
expr->plan = plan;
/* Check to see if it's a simple expression */
! exec_simple_check_plan(expr);
/*
* Mark expression as not using a read-write param. exec_assign_value has
--- 3607,3613 ----
expr->plan = plan;
/* Check to see if it's a simple expression */
! exec_simple_check_plan(estate, expr);
/*
* Mark expression as not using a read-write param. exec_assign_value has
*************** exec_prepare_plan(PLpgSQL_execstate *est
*** 3443,3448 ****
--- 3620,3628 ----
/* ----------
* exec_stmt_execsql Execute an SQL statement (possibly with INTO).
+ *
+ * Note: some callers rely on this not touching stmt_mcontext. If it ever
+ * needs to use that, fix those callers to push/pop stmt_mcontext.
* ----------
*/
static int
*************** exec_stmt_dynexecute(PLpgSQL_execstate *
*** 3675,3680 ****
--- 3855,3861 ----
char *querystr;
int exec_res;
PreparedParamsData *ppd = NULL;
+ MemoryContext stmt_mcontext = get_stmt_mcontext(estate);
/*
* First we evaluate the string expression after the EXECUTE keyword. Its
*************** exec_stmt_dynexecute(PLpgSQL_execstate *
*** 3689,3696 ****
/* Get the C-String representation */
querystr = convert_value_to_string(estate, query, restype);
! /* copy it out of the temporary context before we clean up */
! querystr = pstrdup(querystr);
exec_eval_cleanup(estate);
--- 3870,3877 ----
/* Get the C-String representation */
querystr = convert_value_to_string(estate, query, restype);
! /* copy it into the stmt_mcontext before we clean up */
! querystr = MemoryContextStrdup(stmt_mcontext, querystr);
exec_eval_cleanup(estate);
*************** exec_stmt_dynexecute(PLpgSQL_execstate *
*** 3843,3854 ****
*/
}
! if (ppd)
! free_params_data(ppd);
!
! /* Release any result from SPI_execute, as well as the querystring */
SPI_freetuptable(SPI_tuptable);
! pfree(querystr);
return PLPGSQL_RC_OK;
}
--- 4024,4032 ----
*/
}
! /* Release any result from SPI_execute, as well as transient data */
SPI_freetuptable(SPI_tuptable);
! MemoryContextReset(stmt_mcontext);
return PLPGSQL_RC_OK;
}
*************** static int
*** 3892,3897 ****
--- 4070,4076 ----
exec_stmt_open(PLpgSQL_execstate *estate, PLpgSQL_stmt_open *stmt)
{
PLpgSQL_var *curvar;
+ MemoryContext stmt_mcontext = NULL;
char *curname = NULL;
PLpgSQL_expr *query;
Portal portal;
*************** exec_stmt_open(PLpgSQL_execstate *estate
*** 3905,3911 ****
--- 4084,4097 ----
curvar = (PLpgSQL_var *) (estate->datums[stmt->curvar]);
if (!curvar->isnull)
{
+ MemoryContext oldcontext;
+
+ /* We only need stmt_mcontext to hold the cursor name string */
+ stmt_mcontext = get_stmt_mcontext(estate);
+ oldcontext = MemoryContextSwitchTo(stmt_mcontext);
curname = TextDatumGetCString(curvar->value);
+ MemoryContextSwitchTo(oldcontext);
+
if (SPI_cursor_find(curname) != NULL)
ereport(ERROR,
(errcode(ERRCODE_DUPLICATE_CURSOR),
*************** exec_stmt_open(PLpgSQL_execstate *estate
*** 3942,3948 ****
stmt->cursor_options);
/*
! * If cursor variable was NULL, store the generated portal name in it
*/
if (curname == NULL)
assign_text_var(estate, curvar, portal->name);
--- 4128,4137 ----
stmt->cursor_options);
/*
! * If cursor variable was NULL, store the generated portal name in it.
! * Note: exec_dynquery_with_params already reset the stmt_mcontext, so
! * curname is a dangling pointer here; but testing it for nullness is
! * OK.
*/
if (curname == NULL)
assign_text_var(estate, curvar, portal->name);
*************** exec_stmt_open(PLpgSQL_execstate *estate
*** 4019,4028 ****
if (curname == NULL)
assign_text_var(estate, curvar, portal->name);
! if (curname)
! pfree(curname);
! if (paramLI)
! pfree(paramLI);
return PLPGSQL_RC_OK;
}
--- 4208,4217 ----
if (curname == NULL)
assign_text_var(estate, curvar, portal->name);
! /* If we had any transient data, clean it up */
! exec_eval_cleanup(estate);
! if (stmt_mcontext)
! MemoryContextReset(stmt_mcontext);
return PLPGSQL_RC_OK;
}
*************** exec_stmt_open(PLpgSQL_execstate *estate
*** 4036,4042 ****
static int
exec_stmt_fetch(PLpgSQL_execstate *estate, PLpgSQL_stmt_fetch *stmt)
{
! PLpgSQL_var *curvar = NULL;
PLpgSQL_rec *rec = NULL;
PLpgSQL_row *row = NULL;
long how_many = stmt->how_many;
--- 4225,4231 ----
static int
exec_stmt_fetch(PLpgSQL_execstate *estate, PLpgSQL_stmt_fetch *stmt)
{
! PLpgSQL_var *curvar;
PLpgSQL_rec *rec = NULL;
PLpgSQL_row *row = NULL;
long how_many = stmt->how_many;
*************** exec_stmt_fetch(PLpgSQL_execstate *estat
*** 4044,4049 ****
--- 4233,4239 ----
Portal portal;
char *curname;
uint64 n;
+ MemoryContext oldcontext;
/* ----------
* Get the portal of the cursor by name
*************** exec_stmt_fetch(PLpgSQL_execstate *estat
*** 4054,4067 ****
ereport(ERROR,
(errcode(ERRCODE_NULL_VALUE_NOT_ALLOWED),
errmsg("cursor variable \"%s\" is null", curvar->refname)));
curname = TextDatumGetCString(curvar->value);
portal = SPI_cursor_find(curname);
if (portal == NULL)
ereport(ERROR,
(errcode(ERRCODE_UNDEFINED_CURSOR),
errmsg("cursor \"%s\" does not exist", curname)));
- pfree(curname);
/* Calculate position for FETCH_RELATIVE or FETCH_ABSOLUTE */
if (stmt->expr)
--- 4244,4260 ----
ereport(ERROR,
(errcode(ERRCODE_NULL_VALUE_NOT_ALLOWED),
errmsg("cursor variable \"%s\" is null", curvar->refname)));
+
+ /* Use eval_mcontext for short-lived string */
+ oldcontext = MemoryContextSwitchTo(get_eval_mcontext(estate));
curname = TextDatumGetCString(curvar->value);
+ MemoryContextSwitchTo(oldcontext);
portal = SPI_cursor_find(curname);
if (portal == NULL)
ereport(ERROR,
(errcode(ERRCODE_UNDEFINED_CURSOR),
errmsg("cursor \"%s\" does not exist", curname)));
/* Calculate position for FETCH_RELATIVE or FETCH_ABSOLUTE */
if (stmt->expr)
*************** exec_stmt_fetch(PLpgSQL_execstate *estat
*** 4133,4141 ****
static int
exec_stmt_close(PLpgSQL_execstate *estate, PLpgSQL_stmt_close *stmt)
{
! PLpgSQL_var *curvar = NULL;
Portal portal;
char *curname;
/* ----------
* Get the portal of the cursor by name
--- 4326,4335 ----
static int
exec_stmt_close(PLpgSQL_execstate *estate, PLpgSQL_stmt_close *stmt)
{
! PLpgSQL_var *curvar;
Portal portal;
char *curname;
+ MemoryContext oldcontext;
/* ----------
* Get the portal of the cursor by name
*************** exec_stmt_close(PLpgSQL_execstate *estat
*** 4146,4159 ****
ereport(ERROR,
(errcode(ERRCODE_NULL_VALUE_NOT_ALLOWED),
errmsg("cursor variable \"%s\" is null", curvar->refname)));
curname = TextDatumGetCString(curvar->value);
portal = SPI_cursor_find(curname);
if (portal == NULL)
ereport(ERROR,
(errcode(ERRCODE_UNDEFINED_CURSOR),
errmsg("cursor \"%s\" does not exist", curname)));
- pfree(curname);
/* ----------
* And close it.
--- 4340,4356 ----
ereport(ERROR,
(errcode(ERRCODE_NULL_VALUE_NOT_ALLOWED),
errmsg("cursor variable \"%s\" is null", curvar->refname)));
+
+ /* Use eval_mcontext for short-lived string */
+ oldcontext = MemoryContextSwitchTo(get_eval_mcontext(estate));
curname = TextDatumGetCString(curvar->value);
+ MemoryContextSwitchTo(oldcontext);
portal = SPI_cursor_find(curname);
if (portal == NULL)
ereport(ERROR,
(errcode(ERRCODE_UNDEFINED_CURSOR),
errmsg("cursor \"%s\" does not exist", curname)));
/* ----------
* And close it.
*************** exec_assign_expr(PLpgSQL_execstate *esta
*** 4201,4206 ****
--- 4398,4406 ----
* exec_assign_c_string Put a C string into a text variable.
*
* We take a NULL pointer as signifying empty string, not SQL null.
+ *
+ * As with the underlying exec_assign_value, caller is expected to do
+ * exec_eval_cleanup later.
* ----------
*/
static void
*************** exec_assign_c_string(PLpgSQL_execstate *
*** 4208,4228 ****
const char *str)
{
text *value;
if (str != NULL)
value = cstring_to_text(str);
else
value = cstring_to_text("");
exec_assign_value(estate, target, PointerGetDatum(value), false,
TEXTOID, -1);
- pfree(value);
}
/* ----------
* exec_assign_value Put a value into a target datum
*
! * Note: in some code paths, this will leak memory in the eval_econtext;
* we assume that will be cleaned up later by exec_eval_cleanup. We cannot
* call exec_eval_cleanup here for fear of destroying the input Datum value.
* ----------
--- 4408,4432 ----
const char *str)
{
text *value;
+ MemoryContext oldcontext;
+ /* Use eval_mcontext for short-lived text value */
+ oldcontext = MemoryContextSwitchTo(get_eval_mcontext(estate));
if (str != NULL)
value = cstring_to_text(str);
else
value = cstring_to_text("");
+ MemoryContextSwitchTo(oldcontext);
+
exec_assign_value(estate, target, PointerGetDatum(value), false,
TEXTOID, -1);
}
/* ----------
* exec_assign_value Put a value into a target datum
*
! * Note: in some code paths, this will leak memory in the eval_mcontext;
* we assume that will be cleaned up later by exec_eval_cleanup. We cannot
* call exec_eval_cleanup here for fear of destroying the input Datum value.
* ----------
*************** exec_assign_value(PLpgSQL_execstate *est
*** 4259,4268 ****
/*
* If type is by-reference, copy the new value (which is
! * probably in the eval_econtext) into the procedure's memory
! * context. But if it's a read/write reference to an expanded
! * object, no physical copy needs to happen; at most we need
! * to reparent the object's memory context.
*
* If it's an array, we force the value to be stored in R/W
* expanded form. This wins if the function later does, say,
--- 4463,4472 ----
/*
* If type is by-reference, copy the new value (which is
! * probably in the eval_mcontext) into the procedure's main
! * memory context. But if it's a read/write reference to an
! * expanded object, no physical copy needs to happen; at most
! * we need to reparent the object's memory context.
*
* If it's an array, we force the value to be stored in R/W
* expanded form. This wins if the function later does, say,
*************** exec_assign_value(PLpgSQL_execstate *est
*** 4402,4410 ****
* the attributes except the one we want to replace, use the
* value that's in the old tuple.
*/
! values = palloc(sizeof(Datum) * natts);
! nulls = palloc(sizeof(bool) * natts);
! replaces = palloc(sizeof(bool) * natts);
memset(replaces, false, sizeof(bool) * natts);
replaces[fno] = true;
--- 4606,4614 ----
* the attributes except the one we want to replace, use the
* value that's in the old tuple.
*/
! values = eval_mcontext_alloc(estate, sizeof(Datum) * natts);
! nulls = eval_mcontext_alloc(estate, sizeof(bool) * natts);
! replaces = eval_mcontext_alloc(estate, sizeof(bool) * natts);
memset(replaces, false, sizeof(bool) * natts);
replaces[fno] = true;
*************** exec_assign_value(PLpgSQL_execstate *est
*** 4437,4446 ****
rec->tup = newtup;
rec->freetup = true;
- pfree(values);
- pfree(nulls);
- pfree(replaces);
-
break;
}
--- 4641,4646 ----
*************** exec_assign_value(PLpgSQL_execstate *est
*** 4601,4607 ****
return;
/* empty array, if any, and newarraydatum are short-lived */
! oldcontext = MemoryContextSwitchTo(estate->eval_econtext->ecxt_per_tuple_memory);
if (oldarrayisnull)
oldarraydatum = PointerGetDatum(construct_empty_array(arrayelem->elemtypoid));
--- 4801,4807 ----
return;
/* empty array, if any, and newarraydatum are short-lived */
! oldcontext = MemoryContextSwitchTo(get_eval_mcontext(estate));
if (oldarrayisnull)
oldarraydatum = PointerGetDatum(construct_empty_array(arrayelem->elemtypoid));
*************** exec_assign_value(PLpgSQL_execstate *est
*** 4655,4661 ****
* responsibility that the results are semantically OK.
*
* In some cases we have to palloc a return value, and in such cases we put
! * it into the estate's short-term memory context.
*/
static void
exec_eval_datum(PLpgSQL_execstate *estate,
--- 4855,4861 ----
* responsibility that the results are semantically OK.
*
* In some cases we have to palloc a return value, and in such cases we put
! * it into the estate's eval_mcontext.
*/
static void
exec_eval_datum(PLpgSQL_execstate *estate,
*************** exec_eval_datum(PLpgSQL_execstate *estat
*** 4689,4695 ****
elog(ERROR, "row variable has no tupdesc");
/* Make sure we have a valid type/typmod setting */
BlessTupleDesc(row->rowtupdesc);
! oldcontext = MemoryContextSwitchTo(estate->eval_econtext->ecxt_per_tuple_memory);
tup = make_tuple_from_row(estate, row, row->rowtupdesc);
if (tup == NULL) /* should not happen */
elog(ERROR, "row not compatible with its own tupdesc");
--- 4889,4895 ----
elog(ERROR, "row variable has no tupdesc");
/* Make sure we have a valid type/typmod setting */
BlessTupleDesc(row->rowtupdesc);
! oldcontext = MemoryContextSwitchTo(get_eval_mcontext(estate));
tup = make_tuple_from_row(estate, row, row->rowtupdesc);
if (tup == NULL) /* should not happen */
elog(ERROR, "row not compatible with its own tupdesc");
*************** exec_eval_datum(PLpgSQL_execstate *estat
*** 4715,4721 ****
/* Make sure we have a valid type/typmod setting */
BlessTupleDesc(rec->tupdesc);
! oldcontext = MemoryContextSwitchTo(estate->eval_econtext->ecxt_per_tuple_memory);
*typeid = rec->tupdesc->tdtypeid;
*typetypmod = rec->tupdesc->tdtypmod;
*value = heap_copy_tuple_as_datum(rec->tup, rec->tupdesc);
--- 4915,4921 ----
/* Make sure we have a valid type/typmod setting */
BlessTupleDesc(rec->tupdesc);
! oldcontext = MemoryContextSwitchTo(get_eval_mcontext(estate));
*typeid = rec->tupdesc->tdtypeid;
*typetypmod = rec->tupdesc->tdtypmod;
*value = heap_copy_tuple_as_datum(rec->tup, rec->tupdesc);
*************** exec_run_select(PLpgSQL_execstate *estat
*** 5107,5114 ****
if (*portalP == NULL)
elog(ERROR, "could not open implicit cursor for query \"%s\": %s",
expr->query, SPI_result_code_string(SPI_result));
! if (paramLI)
! pfree(paramLI);
return SPI_OK_CURSOR;
}
--- 5307,5313 ----
if (*portalP == NULL)
elog(ERROR, "could not open implicit cursor for query \"%s\": %s",
expr->query, SPI_result_code_string(SPI_result));
! exec_eval_cleanup(estate);
return SPI_OK_CURSOR;
}
*************** loop_exit:
*** 5323,5331 ****
* give correct results if that happens, and it's unlikely to be worth the
* cycles to check.
*
! * Note: if pass-by-reference, the result is in the eval_econtext's
! * temporary memory context. It will be freed when exec_eval_cleanup
! * is done.
* ----------
*/
static bool
--- 5522,5529 ----
* give correct results if that happens, and it's unlikely to be worth the
* cycles to check.
*
! * Note: if pass-by-reference, the result is in the eval_mcontext.
! * It will be freed when exec_eval_cleanup is done.
* ----------
*/
static bool
*************** exec_eval_simple_expr(PLpgSQL_execstate
*** 5357,5365 ****
/*
* Revalidate cached plan, so that we will notice if it became stale. (We
! * need to hold a refcount while using the plan, anyway.)
*/
cplan = SPI_plan_get_cached_plan(expr->plan);
/*
* We can't get a failure here, because the number of CachedPlanSources in
--- 5555,5566 ----
/*
* Revalidate cached plan, so that we will notice if it became stale. (We
! * need to hold a refcount while using the plan, anyway.) If replanning
! * is needed, do that work in the eval_mcontext.
*/
+ oldcontext = MemoryContextSwitchTo(get_eval_mcontext(estate));
cplan = SPI_plan_get_cached_plan(expr->plan);
+ MemoryContextSwitchTo(oldcontext);
/*
* We can't get a failure here, because the number of CachedPlanSources in
*************** exec_eval_simple_expr(PLpgSQL_execstate
*** 5411,5417 ****
*/
SPI_push();
! oldcontext = MemoryContextSwitchTo(econtext->ecxt_per_tuple_memory);
if (!estate->readonly_func)
{
CommandCounterIncrement();
--- 5612,5618 ----
*/
SPI_push();
! oldcontext = MemoryContextSwitchTo(get_eval_mcontext(estate));
if (!estate->readonly_func)
{
CommandCounterIncrement();
*************** exec_eval_simple_expr(PLpgSQL_execstate
*** 5449,5456 ****
/* Assorted cleanup */
expr->expr_simple_in_use = false;
! if (paramLI && paramLI != estate->paramLI)
! pfree(paramLI);
estate->paramLI->parserSetupArg = save_setup_arg;
--- 5650,5656 ----
/* Assorted cleanup */
expr->expr_simple_in_use = false;
! econtext->ecxt_param_list_info = NULL;
estate->paramLI->parserSetupArg = save_setup_arg;
*************** exec_eval_simple_expr(PLpgSQL_execstate
*** 5498,5505 ****
* throw errors (for example "no such record field") and we do not want that
* to happen in a part of the expression that might never be evaluated at
* runtime. For another thing, exec_eval_datum() may return short-lived
! * values stored in the estate's short-term memory context, which will not
! * necessarily survive to the next SPI operation. And for a third thing, ROW
* and RECFIELD datums' values depend on other datums, and we don't have a
* cheap way to track that. Therefore, param slots for non-VAR datum types
* are always reset here and then filled on-demand by plpgsql_param_fetch().
--- 5698,5705 ----
* throw errors (for example "no such record field") and we do not want that
* to happen in a part of the expression that might never be evaluated at
* runtime. For another thing, exec_eval_datum() may return short-lived
! * values stored in the estate's eval_mcontext, which will not necessarily
! * survive to the next SPI operation. And for a third thing, ROW
* and RECFIELD datums' values depend on other datums, and we don't have a
* cheap way to track that. Therefore, param slots for non-VAR datum types
* are always reset here and then filled on-demand by plpgsql_param_fetch().
*************** setup_param_list(PLpgSQL_execstate *esta
*** 5598,5604 ****
* to some trusted function. We don't want the R/W pointer to get into the
* shared param list, where it could get passed to some less-trusted function.
*
! * Caller should pfree the result after use, if it's not NULL.
*
* XXX. Could we use ParamListInfo's new paramMask to avoid creating unshared
* parameter lists?
--- 5798,5804 ----
* to some trusted function. We don't want the R/W pointer to get into the
* shared param list, where it could get passed to some less-trusted function.
*
! * The result, if not NULL, is in the estate's eval_mcontext.
*
* XXX. Could we use ParamListInfo's new paramMask to avoid creating unshared
* parameter lists?
*************** setup_unshared_param_list(PLpgSQL_execst
*** 5626,5633 ****
/* initialize ParamListInfo with one entry per datum, all invalid */
paramLI = (ParamListInfo)
! palloc0(offsetof(ParamListInfoData, params) +
! estate->ndatums * sizeof(ParamExternData));
paramLI->paramFetch = plpgsql_param_fetch;
paramLI->paramFetchArg = (void *) estate;
paramLI->parserSetup = (ParserSetupHook) plpgsql_parser_setup;
--- 5826,5834 ----
/* initialize ParamListInfo with one entry per datum, all invalid */
paramLI = (ParamListInfo)
! eval_mcontext_alloc0(estate,
! offsetof(ParamListInfoData, params) +
! estate->ndatums * sizeof(ParamExternData));
paramLI->paramFetch = plpgsql_param_fetch;
paramLI->paramFetchArg = (void *) estate;
paramLI->parserSetup = (ParserSetupHook) plpgsql_parser_setup;
*************** exec_move_row(PLpgSQL_execstate *estate,
*** 5784,5795 ****
/* If we have a tupdesc but no data, form an all-nulls tuple */
bool *nulls;
! nulls = (bool *) palloc(tupdesc->natts * sizeof(bool));
memset(nulls, true, tupdesc->natts * sizeof(bool));
tup = heap_form_tuple(tupdesc, NULL, nulls);
-
- pfree(nulls);
}
if (tupdesc)
--- 5985,5995 ----
/* If we have a tupdesc but no data, form an all-nulls tuple */
bool *nulls;
! nulls = (bool *)
! eval_mcontext_alloc(estate, tupdesc->natts * sizeof(bool));
memset(nulls, true, tupdesc->natts * sizeof(bool));
tup = heap_form_tuple(tupdesc, NULL, nulls);
}
if (tupdesc)
*************** exec_move_row(PLpgSQL_execstate *estate,
*** 5907,5912 ****
--- 6107,6115 ----
* make_tuple_from_row Make a tuple from the values of a row object
*
* A NULL return indicates rowtype mismatch; caller must raise suitable error
+ *
+ * The result tuple is freshly palloc'd in caller's context. Some junk
+ * may be left behind in eval_mcontext, too.
* ----------
*/
static HeapTuple
*************** make_tuple_from_row(PLpgSQL_execstate *e
*** 5923,5930 ****
if (natts != row->nfields)
return NULL;
! dvalues = (Datum *) palloc0(natts * sizeof(Datum));
! nulls = (bool *) palloc(natts * sizeof(bool));
for (i = 0; i < natts; i++)
{
--- 6126,6133 ----
if (natts != row->nfields)
return NULL;
! dvalues = (Datum *) eval_mcontext_alloc0(estate, natts * sizeof(Datum));
! nulls = (bool *) eval_mcontext_alloc(estate, natts * sizeof(bool));
for (i = 0; i < natts; i++)
{
*************** make_tuple_from_row(PLpgSQL_execstate *e
*** 5949,5964 ****
tuple = heap_form_tuple(tupdesc, dvalues, nulls);
- pfree(dvalues);
- pfree(nulls);
-
return tuple;
}
/* ----------
* get_tuple_from_datum extract a tuple from a composite Datum
*
! * Returns a freshly palloc'd HeapTuple.
*
* Note: it's caller's responsibility to be sure value is of composite type.
* ----------
--- 6152,6164 ----
tuple = heap_form_tuple(tupdesc, dvalues, nulls);
return tuple;
}
/* ----------
* get_tuple_from_datum extract a tuple from a composite Datum
*
! * Returns a HeapTuple, freshly palloc'd in caller's context.
*
* Note: it's caller's responsibility to be sure value is of composite type.
* ----------
*************** exec_move_row_from_datum(PLpgSQL_execsta
*** 6041,6047 ****
/* ----------
* convert_value_to_string Convert a non-null Datum to C string
*
! * Note: the result is in the estate's eval_econtext, and will be cleared
* by the next exec_eval_cleanup() call. The invoked output function might
* leave additional cruft there as well, so just pfree'ing the result string
* would not be enough to avoid memory leaks if we did not do it like this.
--- 6241,6247 ----
/* ----------
* convert_value_to_string Convert a non-null Datum to C string
*
! * Note: the result is in the estate's eval_mcontext, and will be cleared
* by the next exec_eval_cleanup() call. The invoked output function might
* leave additional cruft there as well, so just pfree'ing the result string
* would not be enough to avoid memory leaks if we did not do it like this.
*************** convert_value_to_string(PLpgSQL_execstat
*** 6061,6067 ****
Oid typoutput;
bool typIsVarlena;
! oldcontext = MemoryContextSwitchTo(estate->eval_econtext->ecxt_per_tuple_memory);
getTypeOutputInfo(valtype, &typoutput, &typIsVarlena);
result = OidOutputFunctionCall(typoutput, value);
MemoryContextSwitchTo(oldcontext);
--- 6261,6267 ----
Oid typoutput;
bool typIsVarlena;
! oldcontext = MemoryContextSwitchTo(get_eval_mcontext(estate));
getTypeOutputInfo(valtype, &typoutput, &typIsVarlena);
result = OidOutputFunctionCall(typoutput, value);
MemoryContextSwitchTo(oldcontext);
*************** convert_value_to_string(PLpgSQL_execstat
*** 6076,6082 ****
* unlikely that a cast operation would produce null from non-null or vice
* versa, that could happen in principle.
*
! * Note: the estate's eval_econtext is used for temporary storage, and may
* also contain the result Datum if we have to do a conversion to a pass-
* by-reference data type. Be sure to do an exec_eval_cleanup() call when
* done with the result.
--- 6276,6282 ----
* unlikely that a cast operation would produce null from non-null or vice
* versa, that could happen in principle.
*
! * Note: the estate's eval_mcontext is used for temporary storage, and may
* also contain the result Datum if we have to do a conversion to a pass-
* by-reference data type. Be sure to do an exec_eval_cleanup() call when
* done with the result.
*************** exec_cast_value(PLpgSQL_execstate *estat
*** 6104,6110 ****
ExprContext *econtext = estate->eval_econtext;
MemoryContext oldcontext;
! oldcontext = MemoryContextSwitchTo(econtext->ecxt_per_tuple_memory);
econtext->caseValue_datum = value;
econtext->caseValue_isNull = *isnull;
--- 6304,6310 ----
ExprContext *econtext = estate->eval_econtext;
MemoryContext oldcontext;
! oldcontext = MemoryContextSwitchTo(get_eval_mcontext(estate));
econtext->caseValue_datum = value;
econtext->caseValue_isNull = *isnull;
*************** get_cast_hashentry(PLpgSQL_execstate *es
*** 6161,6170 ****
/*
* Since we could easily fail (no such coercion), construct a
! * temporary coercion expression tree in a short-lived context, then
! * if successful copy it to cast_hash_context.
*/
! oldcontext = MemoryContextSwitchTo(estate->eval_econtext->ecxt_per_tuple_memory);
/*
* We use a CaseTestExpr as the base of the coercion tree, since it's
--- 6361,6370 ----
/*
* Since we could easily fail (no such coercion), construct a
! * temporary coercion expression tree in the short-lived
! * eval_mcontext, then if successful copy it to cast_hash_context.
*/
! oldcontext = MemoryContextSwitchTo(get_eval_mcontext(estate));
/*
* We use a CaseTestExpr as the base of the coercion tree, since it's
*************** exec_simple_check_node(Node *node)
*** 6545,6556 ****
* ----------
*/
static void
! exec_simple_check_plan(PLpgSQL_expr *expr)
{
List *plansources;
CachedPlanSource *plansource;
Query *query;
CachedPlan *cplan;
/*
* Initialize to "not simple", and remember the plan generation number we
--- 6745,6757 ----
* ----------
*/
static void
! exec_simple_check_plan(PLpgSQL_execstate *estate, PLpgSQL_expr *expr)
{
List *plansources;
CachedPlanSource *plansource;
Query *query;
CachedPlan *cplan;
+ MemoryContext oldcontext;
/*
* Initialize to "not simple", and remember the plan generation number we
*************** exec_simple_check_plan(PLpgSQL_expr *exp
*** 6621,6630 ****
/*
* OK, it seems worth constructing a plan for more careful checking.
*/
!
! /* Get the generic plan for the query */
cplan = SPI_plan_get_cached_plan(expr->plan);
/* Can't fail, because we checked for a single CachedPlanSource above */
Assert(cplan != NULL);
--- 6822,6834 ----
/*
* OK, it seems worth constructing a plan for more careful checking.
+ *
+ * Get the generic plan for the query. If replanning is needed, do that
+ * work in the eval_mcontext.
*/
! oldcontext = MemoryContextSwitchTo(get_eval_mcontext(estate));
cplan = SPI_plan_get_cached_plan(expr->plan);
+ MemoryContextSwitchTo(oldcontext);
/* Can't fail, because we checked for a single CachedPlanSource above */
Assert(cplan != NULL);
*************** assign_text_var(PLpgSQL_execstate *estat
*** 7008,7030 ****
/*
* exec_eval_using_params --- evaluate params of USING clause
*/
static PreparedParamsData *
exec_eval_using_params(PLpgSQL_execstate *estate, List *params)
{
PreparedParamsData *ppd;
int nargs;
int i;
ListCell *lc;
! ppd = (PreparedParamsData *) palloc(sizeof(PreparedParamsData));
nargs = list_length(params);
ppd->nargs = nargs;
! ppd->types = (Oid *) palloc(nargs * sizeof(Oid));
! ppd->values = (Datum *) palloc(nargs * sizeof(Datum));
! ppd->nulls = (char *) palloc(nargs * sizeof(char));
! ppd->freevals = (bool *) palloc(nargs * sizeof(bool));
i = 0;
foreach(lc, params)
--- 7212,7241 ----
/*
* exec_eval_using_params --- evaluate params of USING clause
+ *
+ * The result data structure is created in the stmt_mcontext, and should
+ * be freed by resetting that context.
*/
static PreparedParamsData *
exec_eval_using_params(PLpgSQL_execstate *estate, List *params)
{
PreparedParamsData *ppd;
+ MemoryContext stmt_mcontext = get_stmt_mcontext(estate);
int nargs;
int i;
ListCell *lc;
! ppd = (PreparedParamsData *)
! MemoryContextAlloc(stmt_mcontext, sizeof(PreparedParamsData));
nargs = list_length(params);
ppd->nargs = nargs;
! ppd->types = (Oid *)
! MemoryContextAlloc(stmt_mcontext, nargs * sizeof(Oid));
! ppd->values = (Datum *)
! MemoryContextAlloc(stmt_mcontext, nargs * sizeof(Datum));
! ppd->nulls = (char *)
! MemoryContextAlloc(stmt_mcontext, nargs * sizeof(char));
i = 0;
foreach(lc, params)
*************** exec_eval_using_params(PLpgSQL_execstate
*** 7032,7044 ****
PLpgSQL_expr *param = (PLpgSQL_expr *) lfirst(lc);
bool isnull;
int32 ppdtypmod;
ppd->values[i] = exec_eval_expr(estate, param,
&isnull,
&ppd->types[i],
&ppdtypmod);
ppd->nulls[i] = isnull ? 'n' : ' ';
! ppd->freevals[i] = false;
if (ppd->types[i] == UNKNOWNOID)
{
--- 7243,7257 ----
PLpgSQL_expr *param = (PLpgSQL_expr *) lfirst(lc);
bool isnull;
int32 ppdtypmod;
+ MemoryContext oldcontext;
ppd->values[i] = exec_eval_expr(estate, param,
&isnull,
&ppd->types[i],
&ppdtypmod);
ppd->nulls[i] = isnull ? 'n' : ' ';
!
! oldcontext = MemoryContextSwitchTo(stmt_mcontext);
if (ppd->types[i] == UNKNOWNOID)
{
*************** exec_eval_using_params(PLpgSQL_execstate
*** 7051,7062 ****
*/
ppd->types[i] = TEXTOID;
if (!isnull)
- {
ppd->values[i] = CStringGetTextDatum(DatumGetCString(ppd->values[i]));
- ppd->freevals[i] = true;
- }
}
! /* pass-by-ref non null values must be copied into plpgsql context */
else if (!isnull)
{
int16 typLen;
--- 7264,7272 ----
*/
ppd->types[i] = TEXTOID;
if (!isnull)
ppd->values[i] = CStringGetTextDatum(DatumGetCString(ppd->values[i]));
}
! /* pass-by-ref non null values must be copied into stmt_mcontext */
else if (!isnull)
{
int16 typLen;
*************** exec_eval_using_params(PLpgSQL_execstate
*** 7064,7075 ****
get_typlenbyval(ppd->types[i], &typLen, &typByVal);
if (!typByVal)
- {
ppd->values[i] = datumCopy(ppd->values[i], typByVal, typLen);
- ppd->freevals[i] = true;
- }
}
exec_eval_cleanup(estate);
i++;
--- 7274,7284 ----
get_typlenbyval(ppd->types[i], &typLen, &typByVal);
if (!typByVal)
ppd->values[i] = datumCopy(ppd->values[i], typByVal, typLen);
}
+ MemoryContextSwitchTo(oldcontext);
+
exec_eval_cleanup(estate);
i++;
*************** exec_eval_using_params(PLpgSQL_execstate
*** 7079,7107 ****
}
/*
- * free_params_data --- pfree all pass-by-reference values used in USING clause
- */
- static void
- free_params_data(PreparedParamsData *ppd)
- {
- int i;
-
- for (i = 0; i < ppd->nargs; i++)
- {
- if (ppd->freevals[i])
- pfree(DatumGetPointer(ppd->values[i]));
- }
-
- pfree(ppd->types);
- pfree(ppd->values);
- pfree(ppd->nulls);
- pfree(ppd->freevals);
-
- pfree(ppd);
- }
-
- /*
* Open portal for dynamic query
*/
static Portal
exec_dynquery_with_params(PLpgSQL_execstate *estate,
--- 7288,7299 ----
}
/*
* Open portal for dynamic query
+ *
+ * Caution: this resets the stmt_mcontext at exit. We might eventually need
+ * to move that responsibility to the callers, but currently no caller needs
+ * to have statement-lifetime temp data that survives past this, so it's
+ * simpler to do it here.
*/
static Portal
exec_dynquery_with_params(PLpgSQL_execstate *estate,
*************** exec_dynquery_with_params(PLpgSQL_execst
*** 7116,7121 ****
--- 7308,7314 ----
Oid restype;
int32 restypmod;
char *querystr;
+ MemoryContext stmt_mcontext = get_stmt_mcontext(estate);
/*
* Evaluate the string expression after the EXECUTE keyword. Its result is
*************** exec_dynquery_with_params(PLpgSQL_execst
*** 7130,7137 ****
/* Get the C-String representation */
querystr = convert_value_to_string(estate, query, restype);
! /* copy it out of the temporary context before we clean up */
! querystr = pstrdup(querystr);
exec_eval_cleanup(estate);
--- 7323,7330 ----
/* Get the C-String representation */
querystr = convert_value_to_string(estate, query, restype);
! /* copy it into the stmt_mcontext before we clean up */
! querystr = MemoryContextStrdup(stmt_mcontext, querystr);
exec_eval_cleanup(estate);
*************** exec_dynquery_with_params(PLpgSQL_execst
*** 7151,7157 ****
ppd->values, ppd->nulls,
estate->readonly_func,
cursorOptions);
- free_params_data(ppd);
}
else
{
--- 7344,7349 ----
*************** exec_dynquery_with_params(PLpgSQL_execst
*** 7166,7172 ****
if (portal == NULL)
elog(ERROR, "could not open implicit cursor for query \"%s\": %s",
querystr, SPI_result_code_string(SPI_result));
! pfree(querystr);
return portal;
}
--- 7358,7366 ----
if (portal == NULL)
elog(ERROR, "could not open implicit cursor for query \"%s\": %s",
querystr, SPI_result_code_string(SPI_result));
!
! /* Release transient data */
! MemoryContextReset(stmt_mcontext);
return portal;
}
*************** exec_dynquery_with_params(PLpgSQL_execst
*** 7174,7179 ****
--- 7368,7374 ----
/*
* Return a formatted string with information about an expression's parameters,
* or NULL if the expression does not take any parameters.
+ * The result is in the eval_mcontext.
*/
static char *
format_expr_params(PLpgSQL_execstate *estate,
*************** format_expr_params(PLpgSQL_execstate *es
*** 7182,7191 ****
--- 7377,7389 ----
int paramno;
int dno;
StringInfoData paramstr;
+ MemoryContext oldcontext;
if (!expr->paramnos)
return NULL;
+ oldcontext = MemoryContextSwitchTo(get_eval_mcontext(estate));
+
initStringInfo(¶mstr);
paramno = 0;
dno = -1;
*************** format_expr_params(PLpgSQL_execstate *es
*** 7227,7238 ****
--- 7425,7439 ----
paramno++;
}
+ MemoryContextSwitchTo(oldcontext);
+
return paramstr.data;
}
/*
* Return a formatted string with information about PreparedParamsData, or NULL
* if there are no parameters.
+ * The result is in the eval_mcontext.
*/
static char *
format_preparedparamsdata(PLpgSQL_execstate *estate,
*************** format_preparedparamsdata(PLpgSQL_execst
*** 7240,7249 ****
--- 7441,7453 ----
{
int paramno;
StringInfoData paramstr;
+ MemoryContext oldcontext;
if (!ppd)
return NULL;
+ oldcontext = MemoryContextSwitchTo(get_eval_mcontext(estate));
+
initStringInfo(¶mstr);
for (paramno = 0; paramno < ppd->nargs; paramno++)
{
*************** format_preparedparamsdata(PLpgSQL_execst
*** 7269,7273 ****
--- 7473,7479 ----
}
}
+ MemoryContextSwitchTo(oldcontext);
+
return paramstr.data;
}
diff --git a/src/pl/plpgsql/src/plpgsql.h b/src/pl/plpgsql/src/plpgsql.h
index 140bf4b..e729d3e 100644
*** a/src/pl/plpgsql/src/plpgsql.h
--- b/src/pl/plpgsql/src/plpgsql.h
*************** typedef struct PLpgSQL_execstate
*** 818,823 ****
--- 818,827 ----
HTAB *cast_hash;
MemoryContext cast_hash_context;
+ /* Memory context for statement-lifespan temporary values */
+ MemoryContext stmt_mcontext; /* current stmt context, or NULL if none */
+ MemoryContext stmt_mcontext_parent; /* parent of current context */
+
/* temporary state for results from evaluation of query or expr */
SPITupleTable *eval_tuptable;
uint64 eval_processed;
diff --git a/src/backend/utils/mmgr/aset.c b/src/backend/utils/mmgr/aset.c
index d26991e..65a60b2 100644
*** a/src/backend/utils/mmgr/aset.c
--- b/src/backend/utils/mmgr/aset.c
***************
*** 86,91 ****
--- 86,93 ----
#include "postgres.h"
+ #include <execinfo.h>
+
#include "utils/memdebug.h"
#include "utils/memutils.h"
*************** AllocSetAlloc(MemoryContext context, Siz
*** 667,672 ****
--- 669,703 ----
AssertArg(AllocSetIsValid(set));
+ if (strcmp(context->name, "SPI Proc") == 0)
+ {
+ void *bt[10];
+ int n,
+ j;
+ char **strings;
+
+ n = backtrace(bt, lengthof(bt));
+ if (n > 0)
+ {
+ strings = backtrace_symbols(bt, n);
+ if (strings == NULL)
+ {
+ perror("backtrace_symbols");
+ exit(EXIT_FAILURE);
+ }
+ for (j = 0; j < n; j++)
+ {
+ if (strstr(strings[j], "plpgsql.so"))
+ {
+ fflush(NULL);
+ fprintf(stderr, "alloc from: %s\n", strings[j]);
+ break;
+ }
+ }
+ free(strings);
+ }
+ }
+
/*
* If requested size exceeds maximum for chunks, allocate an entire block
* for this request.
*************** AllocSetFree(MemoryContext context, void
*** 943,948 ****
--- 974,1008 ----
set->header.name, chunk);
#endif
+ if (strcmp(context->name, "SPI Proc") == 0)
+ {
+ void *bt[10];
+ int n,
+ j;
+ char **strings;
+
+ n = backtrace(bt, lengthof(bt));
+ if (n > 0)
+ {
+ strings = backtrace_symbols(bt, n);
+ if (strings == NULL)
+ {
+ perror("backtrace_symbols");
+ exit(EXIT_FAILURE);
+ }
+ for (j = 0; j < n; j++)
+ {
+ if (strstr(strings[j], "plpgsql.so"))
+ {
+ fflush(NULL);
+ fprintf(stderr, "free from: %s\n", strings[j]);
+ break;
+ }
+ }
+ free(strings);
+ }
+ }
+
if (chunk->size > set->allocChunkLimit)
{
/*
diff --git a/src/pl/plpgsql/src/pl_comp.c b/src/pl/plpgsql/src/pl_comp.c
index b628c28..1cdb82a 100644
*** a/src/pl/plpgsql/src/pl_comp.c
--- b/src/pl/plpgsql/src/pl_comp.c
*************** do_compile(FunctionCallInfo fcinfo,
*** 288,293 ****
--- 288,294 ----
int *in_arg_varnos = NULL;
PLpgSQL_variable **out_arg_variables;
MemoryContext func_cxt;
+ MemoryContext oldcontext = CurrentMemoryContext;
/*
* Setup the scanner input and error info. We assume that this function
*************** do_compile(FunctionCallInfo fcinfo,
*** 334,339 ****
--- 335,347 ----
}
plpgsql_curr_compile = function;
+ plpgsql_compile_tmp_cxt =
+ AllocSetContextCreate(CurrentMemoryContext,
+ "PL/pgSQL compile tmp context",
+ ALLOCSET_DEFAULT_MINSIZE,
+ ALLOCSET_DEFAULT_INITSIZE,
+ ALLOCSET_DEFAULT_MAXSIZE);
+
/*
* All the permanent output of compilation (e.g. parse tree) is kept in a
* per-function memory context, so it can be reclaimed easily.
*************** do_compile(FunctionCallInfo fcinfo,
*** 343,349 ****
ALLOCSET_DEFAULT_MINSIZE,
ALLOCSET_DEFAULT_INITSIZE,
ALLOCSET_DEFAULT_MAXSIZE);
! plpgsql_compile_tmp_cxt = MemoryContextSwitchTo(func_cxt);
function->fn_signature = format_procedure(fcinfo->flinfo->fn_oid);
function->fn_oid = fcinfo->flinfo->fn_oid;
--- 351,357 ----
ALLOCSET_DEFAULT_MINSIZE,
ALLOCSET_DEFAULT_INITSIZE,
ALLOCSET_DEFAULT_MAXSIZE);
! MemoryContextSwitchTo(func_cxt);
function->fn_signature = format_procedure(fcinfo->flinfo->fn_oid);
function->fn_oid = fcinfo->flinfo->fn_oid;
*************** do_compile(FunctionCallInfo fcinfo,
*** 774,781 ****
plpgsql_check_syntax = false;
- MemoryContextSwitchTo(plpgsql_compile_tmp_cxt);
plpgsql_compile_tmp_cxt = NULL;
return function;
}
--- 782,790 ----
plpgsql_check_syntax = false;
plpgsql_compile_tmp_cxt = NULL;
+
+ MemoryContextSwitchTo(oldcontext);
return function;
}
*************** plpgsql_compile_inline(char *proc_source
*** 798,803 ****
--- 807,813 ----
PLpgSQL_variable *var;
int parse_rc;
MemoryContext func_cxt;
+ MemoryContext oldcontext = CurrentMemoryContext;
/*
* Setup the scanner input and error info. We assume that this function
*************** plpgsql_compile_inline(char *proc_source
*** 824,829 ****
--- 834,846 ----
plpgsql_curr_compile = function;
+ plpgsql_compile_tmp_cxt =
+ AllocSetContextCreate(CurrentMemoryContext,
+ "PL/pgSQL compile tmp context",
+ ALLOCSET_DEFAULT_MINSIZE,
+ ALLOCSET_DEFAULT_INITSIZE,
+ ALLOCSET_DEFAULT_MAXSIZE);
+
/*
* All the rest of the compile-time storage (e.g. parse tree) is kept in
* its own memory context, so it can be reclaimed easily.
*************** plpgsql_compile_inline(char *proc_source
*** 833,839 ****
ALLOCSET_DEFAULT_MINSIZE,
ALLOCSET_DEFAULT_INITSIZE,
ALLOCSET_DEFAULT_MAXSIZE);
! plpgsql_compile_tmp_cxt = MemoryContextSwitchTo(func_cxt);
function->fn_signature = pstrdup(func_name);
function->fn_is_trigger = PLPGSQL_NOT_TRIGGER;
--- 850,856 ----
ALLOCSET_DEFAULT_MINSIZE,
ALLOCSET_DEFAULT_INITSIZE,
ALLOCSET_DEFAULT_MAXSIZE);
! MemoryContextSwitchTo(func_cxt);
function->fn_signature = pstrdup(func_name);
function->fn_is_trigger = PLPGSQL_NOT_TRIGGER;
*************** plpgsql_compile_inline(char *proc_source
*** 911,918 ****
plpgsql_check_syntax = false;
- MemoryContextSwitchTo(plpgsql_compile_tmp_cxt);
plpgsql_compile_tmp_cxt = NULL;
return function;
}
--- 928,936 ----
plpgsql_check_syntax = false;
plpgsql_compile_tmp_cxt = NULL;
+
+ MemoryContextSwitchTo(oldcontext);
return function;
}
Tom Lane wrote: > Although this is in principle a bug fix, it's invasive enough that I'd > be pretty hesitant to back-patch it, or even to stick it into HEAD > post-beta. I'm inclined to sign it up for the next commitfest instead. Do we have a backpatchable fix for the reported problem? If so, then it seems a good tradeoff to install this more invasive fix in HEAD only and apply the simpler fix to back branches. Otherwise, is the proposal to leave the bug unfixed in back branches? -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Alvaro Herrera wrote: > Tom Lane wrote: > > > Although this is in principle a bug fix, it's invasive enough that I'd > > be pretty hesitant to back-patch it, or even to stick it into HEAD > > post-beta. I'm inclined to sign it up for the next commitfest instead. > > Do we have a backpatchable fix for the reported problem? If so, then it > seems a good tradeoff to install this more invasive fix in HEAD only and > apply the simpler fix to back branches. Otherwise, is the proposal to > leave the bug unfixed in back branches? I meant "install in HEAD only, after the 10.0 branch", which is what you proposed. -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> Tom Lane wrote:
>> Although this is in principle a bug fix, it's invasive enough that I'd
>> be pretty hesitant to back-patch it, or even to stick it into HEAD
>> post-beta. I'm inclined to sign it up for the next commitfest instead.
> Do we have a backpatchable fix for the reported problem? If so, then it
> seems a good tradeoff to install this more invasive fix in HEAD only and
> apply the simpler fix to back branches. Otherwise, is the proposal to
> leave the bug unfixed in back branches?
The latter is what I was thinking. Given that issues like this have been
there, and gone unreported, since we invented subtransactions, I do not
feel too awful about not fixing it in existing branches. It's possible
that we could fix just the one issue originally complained of with a
smaller patch, but I don't find that approach attractive, because it was
also a small leak. The case that seemed nastiest to me is that a FOREACH
... IN ARRAY loop will leak a copy of the entire array if an error causes
control to be thrown out of exec_stmt_foreach_a. That could be a lot more
leaked data than the querystring of an EXECUTE.
I suppose that a fix based on putting PG_TRY blocks into all the affected
functions might be simple enough that we'd risk back-patching it, but
I don't really want to go that way.
regards, tom lane
On Mon, Jul 25, 2016 at 6:04 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > I suppose that a fix based on putting PG_TRY blocks into all the affected > functions might be simple enough that we'd risk back-patching it, but > I don't really want to go that way. try/catch blocks aren't completely free, either, and PL/pgsql is not suffering from a deplorable excess of execution speed. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes:
> On Mon, Jul 25, 2016 at 6:04 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I suppose that a fix based on putting PG_TRY blocks into all the affected
>> functions might be simple enough that we'd risk back-patching it, but
>> I don't really want to go that way.
> try/catch blocks aren't completely free, either, and PL/pgsql is not
> suffering from a deplorable excess of execution speed.
BTW, just to annotate that a bit: I did some measurements and found out
that on my Linux box, creating/deleting a memory context
(AllocSetContextCreate + MemoryContextDelete) is somewhere around 10x
more expensive than a PG_TRY block. This means that the PG_TRY approach
would actually be faster for cases involving only a small number of
statements-needing-local-storage within a single plpgsql function
execution. However, the memory context creation cost is amortized across
repeated executions of a statement, whereas of course PG_TRY won't be.
We can roughly estimate that PG_TRY would lose any time we loop through
the statement in question more than circa ten times. So I believe the
way I want to do it will win speed-wise in cases where it matters, but
it's not entirely an open-and-shut conclusion.
Anyway, there are enough other reasons not to go the PG_TRY route.
regards, tom lane
Hi
2016-07-27 16:49 GMT+02:00 Tom Lane <tgl@sss.pgh.pa.us>:
Robert Haas <robertmhaas@gmail.com> writes:
> On Mon, Jul 25, 2016 at 6:04 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I suppose that a fix based on putting PG_TRY blocks into all the affected
>> functions might be simple enough that we'd risk back-patching it, but
>> I don't really want to go that way.
> try/catch blocks aren't completely free, either, and PL/pgsql is not
> suffering from a deplorable excess of execution speed.
BTW, just to annotate that a bit: I did some measurements and found out
that on my Linux box, creating/deleting a memory context
(AllocSetContextCreate + MemoryContextDelete) is somewhere around 10x
more expensive than a PG_TRY block. This means that the PG_TRY approach
would actually be faster for cases involving only a small number of
statements-needing-local-storage within a single plpgsql function
execution. However, the memory context creation cost is amortized across
repeated executions of a statement, whereas of course PG_TRY won't be.
We can roughly estimate that PG_TRY would lose any time we loop through
the statement in question more than circa ten times. So I believe the
way I want to do it will win speed-wise in cases where it matters, but
it's not entirely an open-and-shut conclusion.
Anyway, there are enough other reasons not to go the PG_TRY route.
I did some synthetic benchmarks related to plpgsql speed - bubble sort and loop over handling errors and I don't see any slowdown
handling exceptions is little bit faster with your patch
CREATE OR REPLACE FUNCTION public.loop_test(a integer)
RETURNS void
LANGUAGE plpgsql
AS $function$
declare x int;
begin
for i in 1..a
loop
declare s text;
begin
s := 'AHOJ';
x := (random()*1000)::int;
raise exception '*****';
exception when others then
x := 0; --raise notice 'handled';
end;
end loop;
end;
$function$
CREATE OR REPLACE FUNCTION public.loop_test(a integer)
RETURNS void
LANGUAGE plpgsql
AS $function$
declare x int;
begin
for i in 1..a
loop
declare s text;
begin
s := 'AHOJ';
x := (random()*1000)::int;
raise exception '*****';
exception when others then
x := 0; --raise notice 'handled';
end;
end loop;
end;
$function$
head - 100K loops 640ms, patched 610ms
Regards
Pavel
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
2016-08-10 11:25 GMT+02:00 Pavel Stehule <pavel.stehule@gmail.com>:
Hi2016-07-27 16:49 GMT+02:00 Tom Lane <tgl@sss.pgh.pa.us>:Robert Haas <robertmhaas@gmail.com> writes:
> On Mon, Jul 25, 2016 at 6:04 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I suppose that a fix based on putting PG_TRY blocks into all the affected
>> functions might be simple enough that we'd risk back-patching it, but
>> I don't really want to go that way.
> try/catch blocks aren't completely free, either, and PL/pgsql is not
> suffering from a deplorable excess of execution speed.
BTW, just to annotate that a bit: I did some measurements and found out
that on my Linux box, creating/deleting a memory context
(AllocSetContextCreate + MemoryContextDelete) is somewhere around 10x
more expensive than a PG_TRY block. This means that the PG_TRY approach
would actually be faster for cases involving only a small number of
statements-needing-local-storage within a single plpgsql function
execution. However, the memory context creation cost is amortized across
repeated executions of a statement, whereas of course PG_TRY won't be.
We can roughly estimate that PG_TRY would lose any time we loop through
the statement in question more than circa ten times. So I believe the
way I want to do it will win speed-wise in cases where it matters, but
it's not entirely an open-and-shut conclusion.
Anyway, there are enough other reasons not to go the PG_TRY route.I did some synthetic benchmarks related to plpgsql speed - bubble sort and loop over handling errors and I don't see any slowdownhandling exceptions is little bit faster with your patch
CREATE OR REPLACE FUNCTION public.loop_test(a integer)
RETURNS void
LANGUAGE plpgsql
AS $function$
declare x int;
begin
for i in 1..a
loop
declare s text;
begin
s := 'AHOJ';
x := (random()*1000)::int;
raise exception '*****';
exception when others then
x := 0; --raise notice 'handled';
end;
end loop;
end;
$function$head - 100K loops 640ms, patched 610msRegards
Hi
I am sending a review of this patch:
1. There was not any problem with patching and compilation
2. All tests passed without any problem
3. The advantages and disadvantages was discussed detailed in this thread - selected way is good
+ the code is little bit reduced and cleaned
+ special context can be used in future
4. For this patch new regress tests or documentation is not necessary
5. I didn't find performance slowdown in special tests - the impact on performance in real code should be insignificant
I'll mark this patch as ready for commiter
Regards
Pavel
Pavel
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Pavel Stehule <pavel.stehule@gmail.com> writes:
> I am sending a review of this patch:
> ...
> I'll mark this patch as ready for commiter
Pushed, thanks for the review.
regards, tom lane
On 7/25/16 1:50 PM, Tom Lane wrote: > There's a glibc-dependent hack in aset.c that reports any > plpgsql-driven palloc or pfree against a context named "SPI Proc", as > well as changes in pl_comp.c so that transient junk created during initial > parsing of a plpgsql function body doesn't end up in the SPI Proc context. > (I did that just to cut the amount of noise I had to chase down. I suppose > in large functions it might be worth adopting such a change so that that > junk can be released immediately after parsing; but I suspect for most > functions it'd just be an extra context without much gain.) Some folks do create very large plpgsql functions, so if there's a handy way to estimate the size of the function (pg_proc.prosrc's varlena size perhaps) then it might be worth doing that for quite large functions. -- Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX Experts in Analytics, Data Architecture and PostgreSQL Data in Trouble? Get it in Treble! http://BlueTreble.com 855-TREBLE2 (855-873-2532) mobile: 512-569-9461
Jim Nasby <Jim.Nasby@bluetreble.com> writes:
> On 7/25/16 1:50 PM, Tom Lane wrote:
>> There's a glibc-dependent hack in aset.c that reports any
>> plpgsql-driven palloc or pfree against a context named "SPI Proc", as
>> well as changes in pl_comp.c so that transient junk created during initial
>> parsing of a plpgsql function body doesn't end up in the SPI Proc context.
>> (I did that just to cut the amount of noise I had to chase down. I suppose
>> in large functions it might be worth adopting such a change so that that
>> junk can be released immediately after parsing; but I suspect for most
>> functions it'd just be an extra context without much gain.)
> Some folks do create very large plpgsql functions, so if there's a handy
> way to estimate the size of the function (pg_proc.prosrc's varlena size
> perhaps) then it might be worth doing that for quite large functions.
I poked at that a bit and realized that during a normal plpgsql function
call, there isn't anything in the SPI Proc context when do_compile is
entered, which means that we could flush all the transient cruft by just
resetting the context afterwards. The sanest place to put that seems to
be in plpgsql_call_handler, as per attached. We could put it in
plpgsql_compile so it's not hit in the main line code path, but that would
require plpgsql_compile to know that it's called in an empty context,
which seems a bit fragile (and indeed probably false in the forValidator
case).
There isn't any equivalently easy way to clean up in the case of a DO
block, but I'm not sure that we care as much in that case.
I'm not sure that this is really a net win. MemoryContextReset is pretty
cheap (at least in non-cassert builds) but it's not zero cost, and in
most scenarios we're not going to care that much about the extra space.
It appeared to me after collecting some stats about the functions present
in the regression tests that for larger functions, the extra space eaten
is just some small multiple (like 2x-3x) of the function body string
length. So it's not *that* much data, even for big functions, and it's
only going to be eaten on the first usage of a function within a session.
regards, tom lane
diff --git a/src/pl/plpgsql/src/pl_handler.c b/src/pl/plpgsql/src/pl_handler.c
index 36868fb..2a36913 100644
*** a/src/pl/plpgsql/src/pl_handler.c
--- b/src/pl/plpgsql/src/pl_handler.c
***************
*** 23,28 ****
--- 23,29 ----
#include "utils/builtins.h"
#include "utils/guc.h"
#include "utils/lsyscache.h"
+ #include "utils/memutils.h"
#include "utils/syscache.h"
*************** plpgsql_call_handler(PG_FUNCTION_ARGS)
*** 230,235 ****
--- 231,239 ----
/* Find or compile the function */
func = plpgsql_compile(fcinfo, false);
+ /* Flush any transient junk allocated during compile */
+ MemoryContextReset(CurrentMemoryContext);
+
/* Must save and restore prior value of cur_estate */
save_cur_estate = func->cur_estate;
On 8/19/16 10:42 AM, Tom Lane wrote: > It appeared to me after collecting some stats about the functions present > in the regression tests that for larger functions, the extra space eaten > is just some small multiple (like 2x-3x) of the function body string > length. So it's not *that* much data, even for big functions, and it's > only going to be eaten on the first usage of a function within a session. I wonder if freeing that memory would help speed up further allocations, and how much of the expense of the final free for the context gets reduced by an early reset. Probably not enough to negate the cost of resetting on every call though... -- Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX Experts in Analytics, Data Architecture and PostgreSQL Data in Trouble? Get it in Treble! http://BlueTreble.com 855-TREBLE2 (855-873-2532) mobile: 512-569-9461