Re: plpgsql leaking memory when stringifying datums - Mailing list pgsql-hackers
| From | Tom Lane |
|---|---|
| Subject | Re: plpgsql leaking memory when stringifying datums |
| Date | |
| Msg-id | 29668.1328925904@sss.pgh.pa.us Whole thread Raw |
| In response to | plpgsql leaking memory when stringifying datums (Jan Urbański <wulczer@wulczer.org>) |
| Responses |
Re: plpgsql leaking memory when stringifying datums
|
| List | pgsql-hackers |
=?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);
pgsql-hackers by date: