Thread: plpython memory leak uppon empty resultsets in all versions
Hi all,
plpython[u] leaks memory in PLy_spi_execute_fetch_result in the following
snippet:
Py_DECREF(result->nrows);
result->nrows = PyInt_FromLong(rows);
PLy_typeinfo_init(&args);
oldcontext = CurrentMemoryContext;
PG_TRY();
{
if (rows)
{
Py_DECREF(result->rows);
result->rows = PyList_New(rows);
PLy_input_tuple_funcs(&args, tuptable->tupdesc);
for (i = 0; i < rows; i++)
{
PyObject *row = PLyDict_FromTuple(&args, tuptable->vals[i],
tuptable->tupdesc);
PyList_SetItem(result->rows, i, row);
}
PLy_typeinfo_dealloc(&args);
SPI_freetuptable(tuptable);
}
}
PG_CATCH();
{
MemoryContextSwitchTo(oldcontext);
PLy_error_in_progress = CopyErrorData();
FlushErrorState();
if (!PyErr_Occurred())
PyErr_SetString(PLy_exc_error,
"Unknown error in PLy_spi_execute_fetch_result");
Py_DECREF(result);
PLy_typeinfo_dealloc(&args);
return NULL;
}
PG_END_TRY();
if rows is 0 PLy_typeinfo_dealloc and SPI_freetuptable will not be called. An
easy example where this is easily leading to a fast growing memleak (1G in
5s):
DO LANGUAGE 'plpythonu' $$while True: plpy.execute("SELECT 1 WHERE false")$$;
The fix is simple. Just move those two outside the if block. As a slightly
alternative solution one could also remove the "return NULL" and move those
outside the PG_CATCH.
Found when investigating a problem of 'bag' on irc (bcc'ed, he had to go home
before I though of asking him whether its ok to publish his name/mail).
Andres
Andres Freund <andres@anarazel.de> writes:
> plpython[u] leaks memory in PLy_spi_execute_fetch_result in the following
> snippet:
Yeah. There's a leak of the tuptable in the CATCH path, too. Will fix.
regards, tom lane
Hi, On Friday 30 April 2010 19:39:32 Tom Lane wrote: > Andres Freund <andres@anarazel.de> writes: > > plpython[u] leaks memory in PLy_spi_execute_fetch_result in the following > > > snippet: > Yeah. There's a leak of the tuptable in the CATCH path, too. Will fix. Yes, theres even more than that (measured it) in the error case. Will have a look later today. Andres
Andres Freund <andres@anarazel.de> writes:
> On Friday 30 April 2010 19:39:32 Tom Lane wrote:
>> Yeah. There's a leak of the tuptable in the CATCH path, too. Will fix.
> Yes, theres even more than that (measured it) in the error case. Will have a
> look later today.
Here's the patch I'm planning to apply --- working it back into the
back branches now.
regards, tom lane
Index: plpython.c
===================================================================
RCS file: /cvsroot/pgsql/src/pl/plpython/plpython.c,v
retrieving revision 1.141
diff -c -r1.141 plpython.c
*** plpython.c 18 Mar 2010 19:43:03 -0000 1.141
--- plpython.c 30 Apr 2010 17:38:20 -0000
***************
*** 3147,3155 ****
PyList_SetItem(result->rows, i, row);
}
- PLy_typeinfo_dealloc(&args);
-
- SPI_freetuptable(tuptable);
}
}
PG_CATCH();
--- 3147,3152 ----
***************
*** 3160,3170 ****
if (!PyErr_Occurred())
PLy_exception_set(PLy_exc_error,
"unrecognized error in PLy_spi_execute_fetch_result");
- Py_DECREF(result);
PLy_typeinfo_dealloc(&args);
return NULL;
}
PG_END_TRY();
}
return (PyObject *) result;
--- 3157,3171 ----
if (!PyErr_Occurred())
PLy_exception_set(PLy_exc_error,
"unrecognized error in PLy_spi_execute_fetch_result");
PLy_typeinfo_dealloc(&args);
+ SPI_freetuptable(tuptable);
+ Py_DECREF(result);
return NULL;
}
PG_END_TRY();
+
+ PLy_typeinfo_dealloc(&args);
+ SPI_freetuptable(tuptable);
}
return (PyObject *) result;
On Friday 30 April 2010 20:09:48 Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > On Friday 30 April 2010 19:39:32 Tom Lane wrote:
> >> Yeah. There's a leak of the tuptable in the CATCH path, too. Will fix.
> >
> > Yes, theres even more than that (measured it) in the error case. Will
> > have a look later today.
>
> Here's the patch I'm planning to apply --- working it back into the
> back branches now.
The one I measured was 9.0 only:
diff --git a/src/pl/plpython/plpython.c b/src/pl/plpython/plpython.c
index 6063628..a6dd9d0 100644
*** a/src/pl/plpython/plpython.c
--- b/src/pl/plpython/plpython.c
*************** plpython_inline_handler(PG_FUNCTION_ARGS
*** 538,546 ****
--- 538,548 ----
PLy_procedure_compile(proc, codeblock->source_text);
PLy_curr_procedure = proc;
PLy_function_handler(&fake_fcinfo, proc);
+ PLy_free(proc);
}
PG_CATCH();
{
+ PLy_free(proc);
PLy_curr_procedure = save_curr_proc;
PyErr_Clear();
PG_RE_THROW();
Found by running something like:
while true; do echo 'DO LANGUAGE plpythonu $$import
gc;gc.collect();plpy.execute("SELECT unknown"); $$;';done|psql -h /tmp -p 5433
postgres
The gc stuff is just to make real leaks way much easier to spot.
Andres
On Friday 30 April 2010 23:54:16 Andres Freund wrote:
> On Friday 30 April 2010 20:09:48 Tom Lane wrote:
> > Andres Freund <andres@anarazel.de> writes:
> > > On Friday 30 April 2010 19:39:32 Tom Lane wrote:
> > >> Yeah. There's a leak of the tuptable in the CATCH path, too. Will
> > >> fix.
> > >
> > > Yes, theres even more than that (measured it) in the error case. Will
> > > have a look later today.
> >
> > Here's the patch I'm planning to apply --- working it back into the
> > back branches now.
>
> The one I measured was 9.0 only:
> Found by running something like:
>
> while true; do echo 'DO LANGUAGE plpythonu $$import
> gc;gc.collect();plpy.execute("SELECT unknown"); $$;';done|psql -h /tmp -p
> 5433 postgres
Actually this one is also visible by 'SELECT 1' which is kinda obivous with
the above diff I guess - but I wanted to mention it for completeness.
Andres
Andres Freund <andres@anarazel.de> writes:
> The one I measured was 9.0 only:
> diff --git a/src/pl/plpython/plpython.c b/src/pl/plpython/plpython.c
> index 6063628..a6dd9d0 100644
> *** a/src/pl/plpython/plpython.c
> --- b/src/pl/plpython/plpython.c
> *************** plpython_inline_handler(PG_FUNCTION_ARGS
> *** 538,546 ****
> --- 538,548 ----
> PLy_procedure_compile(proc, codeblock->source_text);
> PLy_curr_procedure = proc;
> PLy_function_handler(&fake_fcinfo, proc);
> + PLy_free(proc);
> }
> PG_CATCH();
> {
> + PLy_free(proc);
> PLy_curr_procedure = save_curr_proc;
> PyErr_Clear();
> PG_RE_THROW();
> Found by running something like:
> while true; do echo 'DO LANGUAGE plpythonu $$import
> gc;gc.collect();plpy.execute("SELECT unknown"); $$;';done|psql -h /tmp -p 5433
> postgres
I tried this and found there was still a leak after applying your patch.
What seems like the correct thing is to use PLy_procedure_delete(),
as in the attached applied patch. With this, I see zero leak rate for
either this test or the no-error-thrown variant.
This shows that there is a pre-existing leak in PLy_procedure_delete(),
since it was failing to release the proc block itself. I did not bother
to back-patch that, though, because the one pre-existing call was not in
a place where it'd be likely to get executed over and over.
Thanks for the report!
regards, tom lane
Index: plpython.c
===================================================================
RCS file: /cvsroot/pgsql/src/pl/plpython/plpython.c,v
retrieving revision 1.142
diff -c -r1.142 plpython.c
*** plpython.c 30 Apr 2010 19:15:45 -0000 1.142
--- plpython.c 1 May 2010 17:03:35 -0000
***************
*** 541,552 ****
--- 541,555 ----
}
PG_CATCH();
{
+ PLy_procedure_delete(proc);
PLy_curr_procedure = save_curr_proc;
PyErr_Clear();
PG_RE_THROW();
}
PG_END_TRY();
+ PLy_procedure_delete(proc);
+
/* Pop the error context stack */
error_context_stack = plerrcontext.previous;
***************
*** 1664,1669 ****
--- 1667,1673 ----
}
if (proc->argnames)
PLy_free(proc->argnames);
+ PLy_free(proc);
}
/*
Hi,
On Saturday 01 May 2010 19:08:40 Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > while true; do echo 'DO LANGUAGE plpythonu $$import
> > gc;gc.collect();plpy.execute("SELECT unknown"); $$;';done|psql -h /tmp -p
> > 5433 postgres
>
> I tried this and found there was still a leak after applying your patch.
> What seems like the correct thing is to use PLy_procedure_delete(),
> as in the attached applied patch. With this, I see zero leak rate for
> either this test or the no-error-thrown variant.
Yes, I am not surprised. I havent read enough of the code to really understand
its codepaths. I didnt find it exactly obvious what happens where and when
what happens...
Thanks for really fixing the issue.
At times a sql level function that returns the current heap size would be
rather neat for writing minimal regression tests for such things would be
kinda neat. But likely it would increase the runtime a bit too much and would
be hard to implement cross-platformish...
Andres
Hi!
I have tested the proposed patch and it works nicely. Let me know if
further testing is necessary or if i can help.
Thanks!
Björn
Am Freitag, den 30.04.2010, 01:14 +0200 schrieb Andres Freund:
> Hi all,
>
> plpython[u] leaks memory in PLy_spi_execute_fetch_result in the following
> snippet:
>
> Py_DECREF(result->nrows);
> result->nrows = PyInt_FromLong(rows);
> PLy_typeinfo_init(&args);
>
> oldcontext = CurrentMemoryContext;
> PG_TRY();
> {
> if (rows)
> {
> Py_DECREF(result->rows);
> result->rows = PyList_New(rows);
>
> PLy_input_tuple_funcs(&args, tuptable->tupdesc);
> for (i = 0; i < rows; i++)
> {
> PyObject *row = PLyDict_FromTuple(&args, tuptable->vals[i],
> tuptable->tupdesc);
>
> PyList_SetItem(result->rows, i, row);
> }
> PLy_typeinfo_dealloc(&args);
>
> SPI_freetuptable(tuptable);
> }
> }
> PG_CATCH();
> {
> MemoryContextSwitchTo(oldcontext);
> PLy_error_in_progress = CopyErrorData();
> FlushErrorState();
> if (!PyErr_Occurred())
> PyErr_SetString(PLy_exc_error,
> "iso-8859-1 error in PLy_spi_execute_fetch_result");
> Py_DECREF(result);
> PLy_typeinfo_dealloc(&args);
> return NULL;
> }
> PG_END_TRY();
>
> if rows is 0 PLy_typeinfo_dealloc and SPI_freetuptable will not be called. An
> easy example where this is easily leading to a fast growing memleak (1G in
> 5s):
>
> DO LANGUAGE 'plpythonu' $$while True: plpy.execute("SELECT 1 WHERE false")$$;
>
>
> The fix is simple. Just move those two outside the if block. As a slightly
> alternative solution one could also remove the "return NULL" and move those
> outside the PG_CATCH.
>
> Found when investigating a problem of 'bag' on irc (bcc'ed, he had to go home
> before I though of asking him whether its ok to publish his name/mail).
>
> Andres
--
Björn Grüning
Albert-Ludwigs-Universität Freiburg
Institut für Pharmazeutische Wissenschaften
Pharmazeutische Bioinformatik
Hermann-Herder-Strasse 9
D-79104 Freiburg i. Br.
Tel.: +49-761-203-4872
E-Mail: bjoern.gruening@pharmazie.uni-freiburg.de