Thread: plpgsql leaking memory when stringifying datums
While chasing a PL/Python memory leak, I did a few tests with PL/pgSQL and I think there are places where memory is not freed sufficiently early. Attached are two functions that when run will make the backend's memory consumption increase until they finish. With both, the cause is convert_value_to_string that calls a datum's output function, which for toasted data results in an allocation. The proposed patch changes convert_value_to_string to call the output function in the per-tuple memory context and then copy the result string back to the original context. The comment in that function says that callers generally pfree its result, but that wasn't the case with exec_stmt_raise, so I added a pfree() there as well. With that I was still left with a leak in the typecast() test function and it turns out that sticking a exec_eval_cleanup into exec_move_row fixed it. The regression tests pass, but I'm not 100% sure if it's actually safe. Since convert_value_to_string needed to access the PL/pgSQL's execution state to get its hands on the per-tuple context, I needed to pass it to every caller that didn't have it already, which means exec_cast_value and exec_simple_cast_value. Anyone has a better idea? The initial diagnosis and proposed solution are by Andres Freund - thanks! Cheers, Jan
Attachment
=?UTF-8?B?SmFuIFVyYmHFhHNraQ==?= <wulczer@wulczer.org> writes:
> While chasing a PL/Python memory leak, I did a few tests with PL/pgSQL
> and I think there are places where memory is not freed sufficiently early.
I think the basic issue here is that the type output function might
generate (and not bother to free) additional cruft besides its output
string, so that pfree'ing the output alone is not sufficient to avoid
a memory leak if the call occurs in a long-lived context.
However, I don't much care for the details of the proposed patch: if
we're going to fix this by running the output function in the per-tuple
memory context, and expecting the caller to do exec_eval_cleanup later,
why should we add extra pstrdup/pfree overhead? We can just leave the
result in the temp context in most cases, and thus get a net savings
rather than a net cost from fixing this. The attached modified patch
does it like that.
BTW, it occurs to me to wonder whether we need to worry about such
subsidiary leaks in type input functions as well. I see at least one
place where pl_exec.c is tediously freeing the result of
exec_simple_cast_value, but if there are secondary leaks that's not
going to be good enough. Maybe we should switch over to a similar
definition where the cast result is in the per-tuple context, and you've
got to copy it if you want it to be long-lived.
regards, tom lane
diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c
index bf952b62478792b5564fe2af744a318322ea197c..e93b7c63be5d8a385b420dc0d9afa04cb90174a7 100644
*** a/src/pl/plpgsql/src/pl_exec.c
--- b/src/pl/plpgsql/src/pl_exec.c
*************** static void exec_move_row(PLpgSQL_execst
*** 188,201 ****
static HeapTuple make_tuple_from_row(PLpgSQL_execstate *estate,
PLpgSQL_row *row,
TupleDesc tupdesc);
! static char *convert_value_to_string(Datum value, Oid valtype);
! static Datum exec_cast_value(Datum value, Oid valtype,
Oid reqtype,
FmgrInfo *reqinput,
Oid reqtypioparam,
int32 reqtypmod,
bool isnull);
! static Datum exec_simple_cast_value(Datum value, Oid valtype,
Oid reqtype, int32 reqtypmod,
bool isnull);
static void exec_init_tuple_store(PLpgSQL_execstate *estate);
--- 188,204 ----
static HeapTuple make_tuple_from_row(PLpgSQL_execstate *estate,
PLpgSQL_row *row,
TupleDesc tupdesc);
! static char *convert_value_to_string(PLpgSQL_execstate *estate,
! Datum value, Oid valtype);
! static Datum exec_cast_value(PLpgSQL_execstate *estate,
! Datum value, Oid valtype,
Oid reqtype,
FmgrInfo *reqinput,
Oid reqtypioparam,
int32 reqtypmod,
bool isnull);
! static Datum exec_simple_cast_value(PLpgSQL_execstate *estate,
! Datum value, Oid valtype,
Oid reqtype, int32 reqtypmod,
bool isnull);
static void exec_init_tuple_store(PLpgSQL_execstate *estate);
*************** plpgsql_exec_function(PLpgSQL_function *
*** 430,436 ****
else
{
/* Cast value to proper type */
! estate.retval = exec_cast_value(estate.retval, estate.rettype,
func->fn_rettype,
&(func->fn_retinput),
func->fn_rettypioparam,
--- 433,440 ----
else
{
/* Cast value to proper type */
! estate.retval = exec_cast_value(&estate, estate.retval,
! estate.rettype,
func->fn_rettype,
&(func->fn_retinput),
func->fn_rettypioparam,
*************** exec_stmt_fori(PLpgSQL_execstate *estate
*** 1757,1763 ****
* Get the value of the lower bound
*/
value = exec_eval_expr(estate, stmt->lower, &isnull, &valtype);
! value = exec_cast_value(value, valtype, var->datatype->typoid,
&(var->datatype->typinput),
var->datatype->typioparam,
var->datatype->atttypmod, isnull);
--- 1761,1767 ----
* Get the value of the lower bound
*/
value = exec_eval_expr(estate, stmt->lower, &isnull, &valtype);
! value = exec_cast_value(estate, value, valtype, var->datatype->typoid,
&(var->datatype->typinput),
var->datatype->typioparam,
var->datatype->atttypmod, isnull);
*************** exec_stmt_fori(PLpgSQL_execstate *estate
*** 1772,1778 ****
* Get the value of the upper bound
*/
value = exec_eval_expr(estate, stmt->upper, &isnull, &valtype);
! value = exec_cast_value(value, valtype, var->datatype->typoid,
&(var->datatype->typinput),
var->datatype->typioparam,
var->datatype->atttypmod, isnull);
--- 1776,1782 ----
* Get the value of the upper bound
*/
value = exec_eval_expr(estate, stmt->upper, &isnull, &valtype);
! value = exec_cast_value(estate, value, valtype, var->datatype->typoid,
&(var->datatype->typinput),
var->datatype->typioparam,
var->datatype->atttypmod, isnull);
*************** exec_stmt_fori(PLpgSQL_execstate *estate
*** 1789,1795 ****
if (stmt->step)
{
value = exec_eval_expr(estate, stmt->step, &isnull, &valtype);
! value = exec_cast_value(value, valtype, var->datatype->typoid,
&(var->datatype->typinput),
var->datatype->typioparam,
var->datatype->atttypmod, isnull);
--- 1793,1799 ----
if (stmt->step)
{
value = exec_eval_expr(estate, stmt->step, &isnull, &valtype);
! value = exec_cast_value(estate, value, valtype, var->datatype->typoid,
&(var->datatype->typinput),
var->datatype->typioparam,
var->datatype->atttypmod, isnull);
*************** exec_stmt_return_next(PLpgSQL_execstate
*** 2440,2446 ****
errmsg("wrong result type supplied in RETURN NEXT")));
/* coerce type if needed */
! retval = exec_simple_cast_value(retval,
var->datatype->typoid,
tupdesc->attrs[0]->atttypid,
tupdesc->attrs[0]->atttypmod,
--- 2444,2450 ----
errmsg("wrong result type supplied in RETURN NEXT")));
/* coerce type if needed */
! retval = exec_simple_cast_value(estate, retval,
var->datatype->typoid,
tupdesc->attrs[0]->atttypid,
tupdesc->attrs[0]->atttypmod,
*************** exec_stmt_return_next(PLpgSQL_execstate
*** 2511,2517 ****
&rettype);
/* coerce type if needed */
! retval = exec_simple_cast_value(retval,
rettype,
tupdesc->attrs[0]->atttypid,
tupdesc->attrs[0]->atttypmod,
--- 2515,2522 ----
&rettype);
/* coerce type if needed */
! retval = exec_simple_cast_value(estate,
! retval,
rettype,
tupdesc->attrs[0]->atttypid,
tupdesc->attrs[0]->atttypmod,
*************** exec_stmt_return_next(PLpgSQL_execstate
*** 2519,2526 ****
tuplestore_putvalues(estate->tuple_store, tupdesc,
&retval, &isNull);
-
- exec_eval_cleanup(estate);
}
else
{
--- 2524,2529 ----
*************** exec_stmt_return_next(PLpgSQL_execstate
*** 2537,2542 ****
--- 2540,2547 ----
heap_freetuple(tuple);
}
+ exec_eval_cleanup(estate);
+
return PLPGSQL_RC_OK;
}
*************** exec_stmt_raise(PLpgSQL_execstate *estat
*** 2726,2732 ****
if (paramisnull)
extval = "<NULL>";
else
! extval = convert_value_to_string(paramvalue, paramtypeid);
appendStringInfoString(&ds, extval);
current_param = lnext(current_param);
exec_eval_cleanup(estate);
--- 2731,2738 ----
if (paramisnull)
extval = "<NULL>";
else
! extval = convert_value_to_string(estate,
! paramvalue, paramtypeid);
appendStringInfoString(&ds, extval);
current_param = lnext(current_param);
exec_eval_cleanup(estate);
*************** exec_stmt_raise(PLpgSQL_execstate *estat
*** 2764,2770 ****
(errcode(ERRCODE_NULL_VALUE_NOT_ALLOWED),
errmsg("RAISE statement option cannot be null")));
! extval = convert_value_to_string(optionvalue, optiontypeid);
switch (opt->opt_type)
{
--- 2770,2776 ----
(errcode(ERRCODE_NULL_VALUE_NOT_ALLOWED),
errmsg("RAISE statement option cannot be null")));
! extval = convert_value_to_string(estate, optionvalue, optiontypeid);
switch (opt->opt_type)
{
*************** exec_stmt_dynexecute(PLpgSQL_execstate *
*** 3228,3234 ****
errmsg("query string argument of EXECUTE is null")));
/* Get the C-String representation */
! querystr = convert_value_to_string(query, restype);
exec_eval_cleanup(estate);
--- 3234,3243 ----
errmsg("query string argument of EXECUTE is null")));
/* 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);
*************** exec_assign_value(PLpgSQL_execstate *est
*** 3753,3759 ****
PLpgSQL_var *var = (PLpgSQL_var *) target;
Datum newvalue;
! newvalue = exec_cast_value(value, valtype, var->datatype->typoid,
&(var->datatype->typinput),
var->datatype->typioparam,
var->datatype->atttypmod,
--- 3762,3769 ----
PLpgSQL_var *var = (PLpgSQL_var *) target;
Datum newvalue;
! newvalue = exec_cast_value(estate, value, valtype,
! var->datatype->typoid,
&(var->datatype->typinput),
var->datatype->typioparam,
var->datatype->atttypmod,
*************** exec_assign_value(PLpgSQL_execstate *est
*** 3946,3952 ****
atttype = SPI_gettypeid(rec->tupdesc, fno + 1);
atttypmod = rec->tupdesc->attrs[fno]->atttypmod;
attisnull = *isNull;
! values[fno] = exec_simple_cast_value(value,
valtype,
atttype,
atttypmod,
--- 3956,3963 ----
atttype = SPI_gettypeid(rec->tupdesc, fno + 1);
atttypmod = rec->tupdesc->attrs[fno]->atttypmod;
attisnull = *isNull;
! values[fno] = exec_simple_cast_value(estate,
! value,
valtype,
atttype,
atttypmod,
*************** exec_assign_value(PLpgSQL_execstate *est
*** 4118,4124 ****
estate->eval_tuptable = save_eval_tuptable;
/* Coerce source value to match array element type. */
! coerced_value = exec_simple_cast_value(value,
valtype,
arrayelem->elemtypoid,
arrayelem->arraytypmod,
--- 4129,4136 ----
estate->eval_tuptable = save_eval_tuptable;
/* Coerce source value to match array element type. */
! coerced_value = exec_simple_cast_value(estate,
! value,
valtype,
arrayelem->elemtypoid,
arrayelem->arraytypmod,
*************** exec_eval_integer(PLpgSQL_execstate *est
*** 4514,4520 ****
Oid exprtypeid;
exprdatum = exec_eval_expr(estate, expr, isNull, &exprtypeid);
! exprdatum = exec_simple_cast_value(exprdatum, exprtypeid,
INT4OID, -1,
*isNull);
return DatumGetInt32(exprdatum);
--- 4526,4532 ----
Oid exprtypeid;
exprdatum = exec_eval_expr(estate, expr, isNull, &exprtypeid);
! exprdatum = exec_simple_cast_value(estate, exprdatum, exprtypeid,
INT4OID, -1,
*isNull);
return DatumGetInt32(exprdatum);
*************** exec_eval_boolean(PLpgSQL_execstate *est
*** 4536,4542 ****
Oid exprtypeid;
exprdatum = exec_eval_expr(estate, expr, isNull, &exprtypeid);
! exprdatum = exec_simple_cast_value(exprdatum, exprtypeid,
BOOLOID, -1,
*isNull);
return DatumGetBool(exprdatum);
--- 4548,4554 ----
Oid exprtypeid;
exprdatum = exec_eval_expr(estate, expr, isNull, &exprtypeid);
! exprdatum = exec_simple_cast_value(estate, exprdatum, exprtypeid,
BOOLOID, -1,
*isNull);
return DatumGetBool(exprdatum);
*************** exec_move_row(PLpgSQL_execstate *estate,
*** 5277,5282 ****
--- 5289,5296 ----
value, valtype, &isnull);
}
+ exec_eval_cleanup(estate);
+
return;
}
*************** make_tuple_from_row(PLpgSQL_execstate *e
*** 5338,5365 ****
/* ----------
* convert_value_to_string Convert a non-null Datum to C string
*
! * Note: callers generally assume that the result is a palloc'd string and
! * should be pfree'd. This is not all that safe an assumption ...
*
* Note: not caching the conversion function lookup is bad for performance.
* ----------
*/
static char *
! convert_value_to_string(Datum value, Oid valtype)
{
Oid typoutput;
bool typIsVarlena;
getTypeOutputInfo(valtype, &typoutput, &typIsVarlena);
! return OidOutputFunctionCall(typoutput, value);
}
/* ----------
* exec_cast_value Cast a value if required
* ----------
*/
static Datum
! exec_cast_value(Datum value, Oid valtype,
Oid reqtype,
FmgrInfo *reqinput,
Oid reqtypioparam,
--- 5352,5394 ----
/* ----------
* 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.
! * In most usages the Datum being passed in is also in that context (if
! * pass-by-reference) and so an exec_eval_cleanup() call is needed anyway.
*
* Note: not caching the conversion function lookup is bad for performance.
* ----------
*/
static char *
! convert_value_to_string(PLpgSQL_execstate *estate, Datum value, Oid valtype)
{
+ char *result;
+ MemoryContext oldcontext;
Oid typoutput;
bool typIsVarlena;
+ oldcontext = MemoryContextSwitchTo(estate->eval_econtext->ecxt_per_tuple_memory);
getTypeOutputInfo(valtype, &typoutput, &typIsVarlena);
! result = OidOutputFunctionCall(typoutput, value);
! MemoryContextSwitchTo(oldcontext);
!
! return result;
}
/* ----------
* exec_cast_value Cast a value if required
+ *
+ * Note: the estate's eval_econtext is used for temporary storage.
+ * If you use this in a loop, be sure there is an exec_eval_cleanup() call.
+ * The result Datum is in the caller's memory context, however.
* ----------
*/
static Datum
! exec_cast_value(PLpgSQL_execstate *estate,
! Datum value, Oid valtype,
Oid reqtype,
FmgrInfo *reqinput,
Oid reqtypioparam,
*************** exec_cast_value(Datum value, Oid valtype
*** 5376,5385 ****
{
char *extval;
! extval = convert_value_to_string(value, valtype);
value = InputFunctionCall(reqinput, extval,
reqtypioparam, reqtypmod);
- pfree(extval);
}
else
{
--- 5405,5413 ----
{
char *extval;
! extval = convert_value_to_string(estate, value, valtype);
value = InputFunctionCall(reqinput, extval,
reqtypioparam, reqtypmod);
}
else
{
*************** exec_cast_value(Datum value, Oid valtype
*** 5400,5406 ****
* ----------
*/
static Datum
! exec_simple_cast_value(Datum value, Oid valtype,
Oid reqtype, int32 reqtypmod,
bool isnull)
{
--- 5428,5434 ----
* ----------
*/
static Datum
! exec_simple_cast_value(PLpgSQL_execstate *estate, Datum value, Oid valtype,
Oid reqtype, int32 reqtypmod,
bool isnull)
{
*************** exec_simple_cast_value(Datum value, Oid
*** 5414,5420 ****
fmgr_info(typinput, &finfo_input);
! value = exec_cast_value(value,
valtype,
reqtype,
&finfo_input,
--- 5442,5449 ----
fmgr_info(typinput, &finfo_input);
! value = exec_cast_value(estate,
! value,
valtype,
reqtype,
&finfo_input,
*************** exec_dynquery_with_params(PLpgSQL_execst
*** 6137,6143 ****
errmsg("query string argument of EXECUTE is null")));
/* Get the C-String representation */
! querystr = convert_value_to_string(query, restype);
exec_eval_cleanup(estate);
--- 6166,6175 ----
errmsg("query string argument of EXECUTE is null")));
/* 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);
I wrote:
> BTW, it occurs to me to wonder whether we need to worry about such
> subsidiary leaks in type input functions as well.
Sure enough, once you find an input function that leaks memory, there's
trouble:
create type myrow as (f1 text, f2 text, f3 text);
create or replace function leak_assign() returns void as $$
declare
t myrow[];
i int;
begin
for i in 1..10000000 loop
t := '{"(abcd,efg' || ',hij)", "(a,b,c)"}';
end loop;
end;
$$ language plpgsql;
So the attached third try also moves the input function calls in
exec_cast_value into the short-lived context, and rejiggers callers as
necessary to deal with that. This actually ends up simpler and probably
faster than the original coding, because we are able to get rid of some
ad-hoc data copying and pfree'ing, and most of the performance-critical
code paths already had exec_eval_cleanup calls anyway.
Also, you wrote:
>> With that I was still left with a leak in the typecast() test function
>> and it turns out that sticking a exec_eval_cleanup into exec_move_row
>> fixed it. The regression tests pass, but I'm not 100% sure if it's
>> actually safe.
After some study I felt pretty nervous about that too. It's safe enough
with the statement-level callers of exec_move_row, but there are several
calls from exec_assign_value, whose API contract says specifically that
it *won't* call exec_eval_cleanup. Even if it works today, that's a bug
waiting to happen. So I took the exec_eval_cleanup back out of
exec_move_row, and instead made all the statement-level callers do it.
I think this version is ready to go, so barring objections I'll set to
work on back-patching it.
regards, tom lane
diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c
index bf952b62478792b5564fe2af744a318322ea197c..0b126a3f4ac0e16e4ae19b8507c5c1352842a7e2 100644
*** a/src/pl/plpgsql/src/pl_exec.c
--- b/src/pl/plpgsql/src/pl_exec.c
*************** static void exec_move_row(PLpgSQL_execst
*** 188,201 ****
static HeapTuple make_tuple_from_row(PLpgSQL_execstate *estate,
PLpgSQL_row *row,
TupleDesc tupdesc);
! static char *convert_value_to_string(Datum value, Oid valtype);
! static Datum exec_cast_value(Datum value, Oid valtype,
Oid reqtype,
FmgrInfo *reqinput,
Oid reqtypioparam,
int32 reqtypmod,
bool isnull);
! static Datum exec_simple_cast_value(Datum value, Oid valtype,
Oid reqtype, int32 reqtypmod,
bool isnull);
static void exec_init_tuple_store(PLpgSQL_execstate *estate);
--- 188,204 ----
static HeapTuple make_tuple_from_row(PLpgSQL_execstate *estate,
PLpgSQL_row *row,
TupleDesc tupdesc);
! static char *convert_value_to_string(PLpgSQL_execstate *estate,
! Datum value, Oid valtype);
! static Datum exec_cast_value(PLpgSQL_execstate *estate,
! Datum value, Oid valtype,
Oid reqtype,
FmgrInfo *reqinput,
Oid reqtypioparam,
int32 reqtypmod,
bool isnull);
! static Datum exec_simple_cast_value(PLpgSQL_execstate *estate,
! Datum value, Oid valtype,
Oid reqtype, int32 reqtypmod,
bool isnull);
static void exec_init_tuple_store(PLpgSQL_execstate *estate);
*************** plpgsql_exec_function(PLpgSQL_function *
*** 295,300 ****
--- 298,305 ----
/* If arg is null, treat it as an empty row */
exec_move_row(&estate, NULL, row, NULL, NULL);
}
+ /* clean up after exec_move_row() */
+ exec_eval_cleanup(&estate);
}
break;
*************** plpgsql_exec_function(PLpgSQL_function *
*** 430,436 ****
else
{
/* Cast value to proper type */
! estate.retval = exec_cast_value(estate.retval, estate.rettype,
func->fn_rettype,
&(func->fn_retinput),
func->fn_rettypioparam,
--- 435,443 ----
else
{
/* Cast value to proper type */
! estate.retval = exec_cast_value(&estate,
! estate.retval,
! estate.rettype,
func->fn_rettype,
&(func->fn_retinput),
func->fn_rettypioparam,
*************** exec_stmt_fori(PLpgSQL_execstate *estate
*** 1757,1763 ****
* Get the value of the lower bound
*/
value = exec_eval_expr(estate, stmt->lower, &isnull, &valtype);
! value = exec_cast_value(value, valtype, var->datatype->typoid,
&(var->datatype->typinput),
var->datatype->typioparam,
var->datatype->atttypmod, isnull);
--- 1764,1770 ----
* Get the value of the lower bound
*/
value = exec_eval_expr(estate, stmt->lower, &isnull, &valtype);
! value = exec_cast_value(estate, value, valtype, var->datatype->typoid,
&(var->datatype->typinput),
var->datatype->typioparam,
var->datatype->atttypmod, isnull);
*************** exec_stmt_fori(PLpgSQL_execstate *estate
*** 1772,1778 ****
* Get the value of the upper bound
*/
value = exec_eval_expr(estate, stmt->upper, &isnull, &valtype);
! value = exec_cast_value(value, valtype, var->datatype->typoid,
&(var->datatype->typinput),
var->datatype->typioparam,
var->datatype->atttypmod, isnull);
--- 1779,1785 ----
* Get the value of the upper bound
*/
value = exec_eval_expr(estate, stmt->upper, &isnull, &valtype);
! value = exec_cast_value(estate, value, valtype, var->datatype->typoid,
&(var->datatype->typinput),
var->datatype->typioparam,
var->datatype->atttypmod, isnull);
*************** exec_stmt_fori(PLpgSQL_execstate *estate
*** 1789,1795 ****
if (stmt->step)
{
value = exec_eval_expr(estate, stmt->step, &isnull, &valtype);
! value = exec_cast_value(value, valtype, var->datatype->typoid,
&(var->datatype->typinput),
var->datatype->typioparam,
var->datatype->atttypmod, isnull);
--- 1796,1802 ----
if (stmt->step)
{
value = exec_eval_expr(estate, stmt->step, &isnull, &valtype);
! value = exec_cast_value(estate, value, valtype, var->datatype->typoid,
&(var->datatype->typinput),
var->datatype->typioparam,
var->datatype->atttypmod, isnull);
*************** exec_stmt_return_next(PLpgSQL_execstate
*** 2440,2446 ****
errmsg("wrong result type supplied in RETURN NEXT")));
/* coerce type if needed */
! retval = exec_simple_cast_value(retval,
var->datatype->typoid,
tupdesc->attrs[0]->atttypid,
tupdesc->attrs[0]->atttypmod,
--- 2447,2454 ----
errmsg("wrong result type supplied in RETURN NEXT")));
/* coerce type if needed */
! retval = exec_simple_cast_value(estate,
! retval,
var->datatype->typoid,
tupdesc->attrs[0]->atttypid,
tupdesc->attrs[0]->atttypmod,
*************** exec_stmt_return_next(PLpgSQL_execstate
*** 2511,2517 ****
&rettype);
/* coerce type if needed */
! retval = exec_simple_cast_value(retval,
rettype,
tupdesc->attrs[0]->atttypid,
tupdesc->attrs[0]->atttypmod,
--- 2519,2526 ----
&rettype);
/* coerce type if needed */
! retval = exec_simple_cast_value(estate,
! retval,
rettype,
tupdesc->attrs[0]->atttypid,
tupdesc->attrs[0]->atttypmod,
*************** exec_stmt_return_next(PLpgSQL_execstate
*** 2519,2526 ****
tuplestore_putvalues(estate->tuple_store, tupdesc,
&retval, &isNull);
-
- exec_eval_cleanup(estate);
}
else
{
--- 2528,2533 ----
*************** exec_stmt_return_next(PLpgSQL_execstate
*** 2537,2542 ****
--- 2544,2551 ----
heap_freetuple(tuple);
}
+ exec_eval_cleanup(estate);
+
return PLPGSQL_RC_OK;
}
*************** exec_stmt_raise(PLpgSQL_execstate *estat
*** 2726,2732 ****
if (paramisnull)
extval = "<NULL>";
else
! extval = convert_value_to_string(paramvalue, paramtypeid);
appendStringInfoString(&ds, extval);
current_param = lnext(current_param);
exec_eval_cleanup(estate);
--- 2735,2743 ----
if (paramisnull)
extval = "<NULL>";
else
! extval = convert_value_to_string(estate,
! paramvalue,
! paramtypeid);
appendStringInfoString(&ds, extval);
current_param = lnext(current_param);
exec_eval_cleanup(estate);
*************** exec_stmt_raise(PLpgSQL_execstate *estat
*** 2764,2770 ****
(errcode(ERRCODE_NULL_VALUE_NOT_ALLOWED),
errmsg("RAISE statement option cannot be null")));
! extval = convert_value_to_string(optionvalue, optiontypeid);
switch (opt->opt_type)
{
--- 2775,2781 ----
(errcode(ERRCODE_NULL_VALUE_NOT_ALLOWED),
errmsg("RAISE statement option cannot be null")));
! extval = convert_value_to_string(estate, optionvalue, optiontypeid);
switch (opt->opt_type)
{
*************** exec_stmt_execsql(PLpgSQL_execstate *est
*** 3183,3188 ****
--- 3194,3200 ----
}
/* Clean up */
+ exec_eval_cleanup(estate);
SPI_freetuptable(SPI_tuptable);
}
else
*************** exec_stmt_dynexecute(PLpgSQL_execstate *
*** 3228,3234 ****
errmsg("query string argument of EXECUTE is null")));
/* Get the C-String representation */
! querystr = convert_value_to_string(query, restype);
exec_eval_cleanup(estate);
--- 3240,3249 ----
errmsg("query string argument of EXECUTE is null")));
/* 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);
*************** exec_stmt_dynexecute(PLpgSQL_execstate *
*** 3361,3366 ****
--- 3376,3383 ----
/* Put the first result row into the target */
exec_move_row(estate, rec, row, tuptab->vals[0], tuptab->tupdesc);
}
+ /* clean up after exec_move_row() */
+ exec_eval_cleanup(estate);
}
else
{
*************** exec_stmt_fetch(PLpgSQL_execstate *estat
*** 3634,3639 ****
--- 3651,3657 ----
else
exec_move_row(estate, rec, row, tuptab->vals[0], tuptab->tupdesc);
+ exec_eval_cleanup(estate);
SPI_freetuptable(tuptab);
}
else
*************** exec_assign_c_string(PLpgSQL_execstate *
*** 3733,3739 ****
/* ----------
* exec_assign_value Put a value into a target field
*
! * Note: in some code paths, this may 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.
* ----------
--- 3751,3757 ----
/* ----------
* exec_assign_value Put a value into a target field
*
! * 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.
* ----------
*************** exec_assign_value(PLpgSQL_execstate *est
*** 3753,3759 ****
PLpgSQL_var *var = (PLpgSQL_var *) target;
Datum newvalue;
! newvalue = exec_cast_value(value, valtype, var->datatype->typoid,
&(var->datatype->typinput),
var->datatype->typioparam,
var->datatype->atttypmod,
--- 3771,3780 ----
PLpgSQL_var *var = (PLpgSQL_var *) target;
Datum newvalue;
! newvalue = exec_cast_value(estate,
! value,
! valtype,
! var->datatype->typoid,
&(var->datatype->typinput),
var->datatype->typioparam,
var->datatype->atttypmod,
*************** exec_assign_value(PLpgSQL_execstate *est
*** 3766,3784 ****
var->refname)));
/*
! * If type is by-reference, make sure we have a freshly
! * palloc'd copy; the originally passed value may not live as
! * long as the variable! But we don't need to re-copy if
! * exec_cast_value performed a conversion; its output must
! * already be palloc'd.
*/
if (!var->datatype->typbyval && !*isNull)
! {
! if (newvalue == value)
! newvalue = datumCopy(newvalue,
! false,
! var->datatype->typlen);
! }
/*
* Now free the old value. (We can't do this any earlier
--- 3787,3800 ----
var->refname)));
/*
! * If type is by-reference, copy the new value (which is
! * probably in the eval_econtext) into the procedure's
! * memory context.
*/
if (!var->datatype->typbyval && !*isNull)
! newvalue = datumCopy(newvalue,
! false,
! var->datatype->typlen);
/*
* Now free the old value. (We can't do this any earlier
*************** exec_assign_value(PLpgSQL_execstate *est
*** 3894,3900 ****
Datum *values;
bool *nulls;
bool *replaces;
- void *mustfree;
bool attisnull;
Oid atttype;
int32 atttypmod;
--- 3910,3915 ----
*************** exec_assign_value(PLpgSQL_execstate *est
*** 3946,3952 ****
atttype = SPI_gettypeid(rec->tupdesc, fno + 1);
atttypmod = rec->tupdesc->attrs[fno]->atttypmod;
attisnull = *isNull;
! values[fno] = exec_simple_cast_value(value,
valtype,
atttype,
atttypmod,
--- 3961,3968 ----
atttype = SPI_gettypeid(rec->tupdesc, fno + 1);
atttypmod = rec->tupdesc->attrs[fno]->atttypmod;
attisnull = *isNull;
! values[fno] = exec_simple_cast_value(estate,
! value,
valtype,
atttype,
atttypmod,
*************** exec_assign_value(PLpgSQL_execstate *est
*** 3954,3968 ****
nulls[fno] = attisnull;
/*
- * Avoid leaking the result of exec_simple_cast_value, if it
- * performed a conversion to a pass-by-ref type.
- */
- if (!attisnull && values[fno] != value && !get_typbyval(atttype))
- mustfree = DatumGetPointer(values[fno]);
- else
- mustfree = NULL;
-
- /*
* Now call heap_modify_tuple() to create a new tuple that
* replaces the old one in the record.
*/
--- 3970,3975 ----
*************** exec_assign_value(PLpgSQL_execstate *est
*** 3978,3985 ****
pfree(values);
pfree(nulls);
pfree(replaces);
- if (mustfree)
- pfree(mustfree);
break;
}
--- 3985,3990 ----
*************** exec_assign_value(PLpgSQL_execstate *est
*** 4002,4007 ****
--- 4007,4013 ----
ArrayType *oldarrayval;
ArrayType *newarrayval;
SPITupleTable *save_eval_tuptable;
+ MemoryContext oldcontext;
/*
* We need to do subscript evaluation, which might require
*************** exec_assign_value(PLpgSQL_execstate *est
*** 4118,4124 ****
estate->eval_tuptable = save_eval_tuptable;
/* Coerce source value to match array element type. */
! coerced_value = exec_simple_cast_value(value,
valtype,
arrayelem->elemtypoid,
arrayelem->arraytypmod,
--- 4124,4131 ----
estate->eval_tuptable = save_eval_tuptable;
/* Coerce source value to match array element type. */
! coerced_value = exec_simple_cast_value(estate,
! value,
valtype,
arrayelem->elemtypoid,
arrayelem->arraytypmod,
*************** exec_assign_value(PLpgSQL_execstate *est
*** 4138,4143 ****
--- 4145,4153 ----
(oldarrayisnull || *isNull))
return;
+ /* oldarrayval and newarrayval should be short-lived */
+ oldcontext = MemoryContextSwitchTo(estate->eval_econtext->ecxt_per_tuple_memory);
+
if (oldarrayisnull)
oldarrayval = construct_empty_array(arrayelem->elemtypoid);
else
*************** exec_assign_value(PLpgSQL_execstate *est
*** 4156,4168 ****
arrayelem->elemtypbyval,
arrayelem->elemtypalign);
! /*
! * Avoid leaking the result of exec_simple_cast_value, if it
! * performed a conversion to a pass-by-ref type.
! */
! if (!*isNull && coerced_value != value &&
! !arrayelem->elemtypbyval)
! pfree(DatumGetPointer(coerced_value));
/*
* Assign the new array to the base variable. It's never NULL
--- 4166,4172 ----
arrayelem->elemtypbyval,
arrayelem->elemtypalign);
! MemoryContextSwitchTo(oldcontext);
/*
* Assign the new array to the base variable. It's never NULL
*************** exec_assign_value(PLpgSQL_execstate *est
*** 4174,4184 ****
exec_assign_value(estate, target,
PointerGetDatum(newarrayval),
arrayelem->arraytypoid, isNull);
-
- /*
- * Avoid leaking the modified array value, too.
- */
- pfree(newarrayval);
break;
}
--- 4178,4183 ----
*************** exec_eval_integer(PLpgSQL_execstate *est
*** 4514,4520 ****
Oid exprtypeid;
exprdatum = exec_eval_expr(estate, expr, isNull, &exprtypeid);
! exprdatum = exec_simple_cast_value(exprdatum, exprtypeid,
INT4OID, -1,
*isNull);
return DatumGetInt32(exprdatum);
--- 4513,4519 ----
Oid exprtypeid;
exprdatum = exec_eval_expr(estate, expr, isNull, &exprtypeid);
! exprdatum = exec_simple_cast_value(estate, exprdatum, exprtypeid,
INT4OID, -1,
*isNull);
return DatumGetInt32(exprdatum);
*************** exec_eval_boolean(PLpgSQL_execstate *est
*** 4536,4542 ****
Oid exprtypeid;
exprdatum = exec_eval_expr(estate, expr, isNull, &exprtypeid);
! exprdatum = exec_simple_cast_value(exprdatum, exprtypeid,
BOOLOID, -1,
*isNull);
return DatumGetBool(exprdatum);
--- 4535,4541 ----
Oid exprtypeid;
exprdatum = exec_eval_expr(estate, expr, isNull, &exprtypeid);
! exprdatum = exec_simple_cast_value(estate, exprdatum, exprtypeid,
BOOLOID, -1,
*isNull);
return DatumGetBool(exprdatum);
*************** exec_for_query(PLpgSQL_execstate *estate
*** 4730,4736 ****
--- 4729,4738 ----
* through with found = false.
*/
if (n <= 0)
+ {
exec_move_row(estate, rec, row, NULL, tuptab->tupdesc);
+ exec_eval_cleanup(estate);
+ }
else
found = true; /* processed at least one tuple */
*************** exec_for_query(PLpgSQL_execstate *estate
*** 4747,4752 ****
--- 4749,4755 ----
* Assign the tuple to the target
*/
exec_move_row(estate, rec, row, tuptab->vals[i], tuptab->tupdesc);
+ exec_eval_cleanup(estate);
/*
* Execute the statements
*************** plpgsql_param_fetch(ParamListInfo params
*** 5138,5143 ****
--- 5141,5149 ----
/* ----------
* exec_move_row Move one tuple's values into a record or row
+ *
+ * Since this uses exec_assign_value, caller should eventually call
+ * exec_eval_cleanup to prevent long-term memory leaks.
* ----------
*/
static void
*************** make_tuple_from_row(PLpgSQL_execstate *e
*** 5338,5365 ****
/* ----------
* convert_value_to_string Convert a non-null Datum to C string
*
! * Note: callers generally assume that the result is a palloc'd string and
! * should be pfree'd. This is not all that safe an assumption ...
*
* Note: not caching the conversion function lookup is bad for performance.
* ----------
*/
static char *
! convert_value_to_string(Datum value, Oid valtype)
{
Oid typoutput;
bool typIsVarlena;
getTypeOutputInfo(valtype, &typoutput, &typIsVarlena);
! return OidOutputFunctionCall(typoutput, value);
}
/* ----------
* exec_cast_value Cast a value if required
* ----------
*/
static Datum
! exec_cast_value(Datum value, Oid valtype,
Oid reqtype,
FmgrInfo *reqinput,
Oid reqtypioparam,
--- 5344,5387 ----
/* ----------
* 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.
! * In most usages the Datum being passed in is also in that context (if
! * pass-by-reference) and so an exec_eval_cleanup() call is needed anyway.
*
* Note: not caching the conversion function lookup is bad for performance.
* ----------
*/
static char *
! convert_value_to_string(PLpgSQL_execstate *estate, Datum value, Oid valtype)
{
+ char *result;
+ MemoryContext oldcontext;
Oid typoutput;
bool typIsVarlena;
+ oldcontext = MemoryContextSwitchTo(estate->eval_econtext->ecxt_per_tuple_memory);
getTypeOutputInfo(valtype, &typoutput, &typIsVarlena);
! result = OidOutputFunctionCall(typoutput, value);
! MemoryContextSwitchTo(oldcontext);
!
! return result;
}
/* ----------
* exec_cast_value Cast a value if required
+ *
+ * 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.
* ----------
*/
static Datum
! exec_cast_value(PLpgSQL_execstate *estate,
! Datum value, Oid valtype,
Oid reqtype,
FmgrInfo *reqinput,
Oid reqtypioparam,
*************** exec_cast_value(Datum value, Oid valtype
*** 5367,5391 ****
bool isnull)
{
/*
! * If the type of the queries return value isn't that of the variable,
! * convert it.
*/
if (valtype != reqtype || reqtypmod != -1)
{
if (!isnull)
{
char *extval;
! extval = convert_value_to_string(value, valtype);
value = InputFunctionCall(reqinput, extval,
reqtypioparam, reqtypmod);
- pfree(extval);
}
else
{
value = InputFunctionCall(reqinput, NULL,
reqtypioparam, reqtypmod);
}
}
return value;
--- 5389,5415 ----
bool isnull)
{
/*
! * If the type of the given value isn't what's requested, convert it.
*/
if (valtype != reqtype || reqtypmod != -1)
{
+ MemoryContext oldcontext;
+
+ oldcontext = MemoryContextSwitchTo(estate->eval_econtext->ecxt_per_tuple_memory);
if (!isnull)
{
char *extval;
! extval = convert_value_to_string(estate, value, valtype);
value = InputFunctionCall(reqinput, extval,
reqtypioparam, reqtypmod);
}
else
{
value = InputFunctionCall(reqinput, NULL,
reqtypioparam, reqtypmod);
}
+ MemoryContextSwitchTo(oldcontext);
}
return value;
*************** exec_cast_value(Datum value, Oid valtype
*** 5400,5406 ****
* ----------
*/
static Datum
! exec_simple_cast_value(Datum value, Oid valtype,
Oid reqtype, int32 reqtypmod,
bool isnull)
{
--- 5424,5431 ----
* ----------
*/
static Datum
! exec_simple_cast_value(PLpgSQL_execstate *estate,
! Datum value, Oid valtype,
Oid reqtype, int32 reqtypmod,
bool isnull)
{
*************** exec_simple_cast_value(Datum value, Oid
*** 5414,5420 ****
fmgr_info(typinput, &finfo_input);
! value = exec_cast_value(value,
valtype,
reqtype,
&finfo_input,
--- 5439,5446 ----
fmgr_info(typinput, &finfo_input);
! value = exec_cast_value(estate,
! value,
valtype,
reqtype,
&finfo_input,
*************** exec_dynquery_with_params(PLpgSQL_execst
*** 6137,6143 ****
errmsg("query string argument of EXECUTE is null")));
/* Get the C-String representation */
! querystr = convert_value_to_string(query, restype);
exec_eval_cleanup(estate);
--- 6163,6172 ----
errmsg("query string argument of EXECUTE is null")));
/* 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);