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
diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c
index bf952b6..a691905 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_raise(PLpgSQL_execstate *estat
*** 2724,2734 ****
¶mtypeid);
if (paramisnull)
! extval = "<NULL>";
else
! extval = convert_value_to_string(paramvalue, paramtypeid);
appendStringInfoString(&ds, extval);
current_param = lnext(current_param);
exec_eval_cleanup(estate);
}
else
--- 2729,2741 ----
¶mtypeid);
if (paramisnull)
! extval = pstrdup("<NULL>");
else
! extval = convert_value_to_string(estate,
! paramvalue, paramtypeid);
appendStringInfoString(&ds, extval);
current_param = lnext(current_param);
+ pfree(extval);
exec_eval_cleanup(estate);
}
else
*************** 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)
{
--- 2771,2777 ----
(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);
--- 3235,3241 ----
errmsg("query string argument of EXECUTE is null")));
/* Get the C-String representation */
! querystr = convert_value_to_string(estate, query, restype);
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,
--- 3760,3766 ----
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,
--- 3953,3960 ----
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,
--- 4126,4133 ----
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);
--- 4523,4529 ----
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);
--- 4545,4551 ----
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 ****
--- 5286,5293 ----
value, valtype, &isnull);
}
+ exec_eval_cleanup(estate);
+
return;
}
*************** make_tuple_from_row(PLpgSQL_execstate *e
*** 5339,5357 ****
* 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);
}
/* ----------
--- 5350,5375 ----
* 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.
*
* Note: not caching the conversion function lookup is bad for performance.
* ----------
*/
static char *
! convert_value_to_string(PLpgSQL_execstate *estate, Datum value, Oid valtype)
{
! Oid typoutput;
! bool typIsVarlena;
! MemoryContext oldcontext;
! char *ret;
getTypeOutputInfo(valtype, &typoutput, &typIsVarlena);
! oldcontext = MemoryContextSwitchTo(
! estate->eval_econtext->ecxt_per_tuple_memory);
! ret = OidOutputFunctionCall(typoutput, value);
! MemoryContextSwitchTo(oldcontext);
!
! return pstrdup(ret);
}
/* ----------
*************** convert_value_to_string(Datum value, Oid
*** 5359,5365 ****
* ----------
*/
static Datum
! exec_cast_value(Datum value, Oid valtype,
Oid reqtype,
FmgrInfo *reqinput,
Oid reqtypioparam,
--- 5377,5384 ----
* ----------
*/
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,5382 ****
{
char *extval;
! extval = convert_value_to_string(value, valtype);
value = InputFunctionCall(reqinput, extval,
reqtypioparam, reqtypmod);
pfree(extval);
--- 5395,5401 ----
{
char *extval;
! extval = convert_value_to_string(estate, value, valtype);
value = InputFunctionCall(reqinput, extval,
reqtypioparam, reqtypmod);
pfree(extval);
*************** 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)
{
--- 5419,5425 ----
* ----------
*/
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,
--- 5433,5440 ----
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);
--- 6157,6163 ----
errmsg("query string argument of EXECUTE is null")));
/* Get the C-String representation */
! querystr = convert_value_to_string(estate, query, restype);
exec_eval_cleanup(estate);
plpgsql-leaks.sql
Description: application/sql
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
