Repair memory leaks in plpython.

PLy_spi_execute_plan (PLyPlan.execute) and PLy_cursor_plan
(plpy.cursor) use PLy_output_convert to convert Python values
into Datums that can be passed to the query-to-execute.  But they
failed to pay much attention to its warning that it can leave "cruft
generated along the way" behind.  Repeated use of these methods can
result in a substantial memory leak for the duration of the calling
plpython function.

To fix, make a temporary memory context to invoke PLy_output_convert
in.  This also lets us get rid of the rather fragile code that was
here for retail pfree's of the converted Datums.  Indeed, we don't
need the PLyPlanObject.values field anymore at all, though I left it
in place in the back branches in the name of ABI stability.

Mat Arye and Tom Lane, per report from Mat Arye.  Back-patch to all
supported branches.

Discussion: https://postgr.es/m/CADsUR0DvVgnZYWwnmKRK65MZg7YLUSTDLV61qdnrwtrAJgU6xw@mail.gmail.com

diff --git a/src/pl/plpython/plpy_cursorobject.c b/src/pl/plpython/plpy_cursorobject.c
index 6b6e74334531b07189685482f735c74209246bab..55c83faed02820a52ba96c7f7097072d2b01b5b2 100644 (file)
--- a/src/pl/plpython/plpy_cursorobject.c
+++ b/src/pl/plpython/plpy_cursorobject.c
@@ -142,7 +142,6 @@ PLy_cursor_plan(PyObject *ob, PyObject *args)
 {
        PLyCursorObject *cursor;
        volatile int nargs;
-       int                     i;
        PLyPlanObject *plan;
        PLyExecutionContext *exec_ctx = PLy_current_execution_context();
        volatile MemoryContext oldcontext;
@@ -201,13 +200,30 @@ PLy_cursor_plan(PyObject *ob, PyObject *args)
        PG_TRY();
        {
                Portal          portal;
+               MemoryContext tmpcontext;
+               Datum      *volatile values;
                char       *volatile nulls;
                volatile int j;
 
+               /*
+                * Converted arguments and associated cruft will be in this context,
+                * which is local to our subtransaction.
+                */
+               tmpcontext = AllocSetContextCreate(CurTransactionContext,
+                                                                                  "PL/Python temporary context",
+                                                                                  ALLOCSET_SMALL_SIZES);
+               MemoryContextSwitchTo(tmpcontext);
+
                if (nargs > 0)
-                       nulls = palloc(nargs * sizeof(char));
+               {
+                       values = (Datum *) palloc(nargs * sizeof(Datum));
+                       nulls = (char *) palloc(nargs * sizeof(char));
+               }
                else
+               {
+                       values = NULL;
                        nulls = NULL;
+               }
 
                for (j = 0; j < nargs; j++)
                {
@@ -219,7 +235,7 @@ PLy_cursor_plan(PyObject *ob, PyObject *args)
                        {
                                bool            isnull;
 
-                               plan->values[j] = PLy_output_convert(arg, elem, &isnull);
+                               values[j] = PLy_output_convert(arg, elem, &isnull);
                                nulls[j] = isnull ? 'n' : ' ';
                        }
                        PG_FINALLY();
@@ -229,7 +245,9 @@ PLy_cursor_plan(PyObject *ob, PyObject *args)
                        PG_END_TRY();
                }
 
-               portal = SPI_cursor_open(NULL, plan->plan, plan->values, nulls,
+               MemoryContextSwitchTo(oldcontext);
+
+               portal = SPI_cursor_open(NULL, plan->plan, values, nulls,
                                                                 exec_ctx->curr_proc->fn_readonly);
                if (portal == NULL)
                        elog(ERROR, "SPI_cursor_open() failed: %s",
@@ -239,40 +257,18 @@ PLy_cursor_plan(PyObject *ob, PyObject *args)
 
                PinPortal(portal);
 
+               MemoryContextDelete(tmpcontext);
                PLy_spi_subtransaction_commit(oldcontext, oldowner);
        }
        PG_CATCH();
        {
-               int                     k;
-
-               /* cleanup plan->values array */
-               for (k = 0; k < nargs; k++)
-               {
-                       if (!plan->args[k].typbyval &&
-                               (plan->values[k] != PointerGetDatum(NULL)))
-                       {
-                               pfree(DatumGetPointer(plan->values[k]));
-                               plan->values[k] = PointerGetDatum(NULL);
-                       }
-               }
-
                Py_DECREF(cursor);
-
+               /* Subtransaction abort will remove the tmpcontext */
                PLy_spi_subtransaction_abort(oldcontext, oldowner);
                return NULL;
        }
        PG_END_TRY();
 
-       for (i = 0; i < nargs; i++)
-       {
-               if (!plan->args[i].typbyval &&
-                       (plan->values[i] != PointerGetDatum(NULL)))
-               {
-                       pfree(DatumGetPointer(plan->values[i]));
-                       plan->values[i] = PointerGetDatum(NULL);
-               }
-       }
-
        Assert(cursor->portalname != NULL);
        return (PyObject *) cursor;
 }

diff --git a/src/pl/plpython/plpy_spi.c b/src/pl/plpython/plpy_spi.c
index 9a71a42c15f2944c534cd5017186e826c23c2dab..68810e59c92f750779057573b6eceb4a2a3eb2bc 100644 (file)
--- a/src/pl/plpython/plpy_spi.c
+++ b/src/pl/plpython/plpy_spi.c
@@ -175,8 +175,7 @@ PyObject *
 PLy_spi_execute_plan(PyObject *ob, PyObject *list, long limit)
 {
        volatile int nargs;
-       int                     i,
-                               rv;
+       int                     rv;
        PLyPlanObject *plan;
        volatile MemoryContext oldcontext;
        volatile ResourceOwner oldowner;
@@ -222,13 +221,30 @@ PLy_spi_execute_plan(PyObject *ob, PyObject *list, long limit)
        PG_TRY();
        {
                PLyExecutionContext *exec_ctx = PLy_current_execution_context();
+               MemoryContext tmpcontext;
+               Datum      *volatile values;
                char       *volatile nulls;
                volatile int j;
 
+               /*
+                * Converted arguments and associated cruft will be in this context,
+                * which is local to our subtransaction.
+                */
+               tmpcontext = AllocSetContextCreate(CurTransactionContext,
+                                                                                  "PL/Python temporary context",
+                                                                                  ALLOCSET_SMALL_SIZES);
+               MemoryContextSwitchTo(tmpcontext);
+
                if (nargs > 0)
-                       nulls = palloc(nargs * sizeof(char));
+               {
+                       values = (Datum *) palloc(nargs * sizeof(Datum));
+                       nulls = (char *) palloc(nargs * sizeof(char));
+               }
                else
+               {
+                       values = NULL;
                        nulls = NULL;
+               }
 
                for (j = 0; j < nargs; j++)
                {
@@ -240,7 +256,7 @@ PLy_spi_execute_plan(PyObject *ob, PyObject *list, long limit)
                        {
                                bool            isnull;
 
-                               plan->values[j] = PLy_output_convert(arg, elem, &isnull);
+                               values[j] = PLy_output_convert(arg, elem, &isnull);
                                nulls[j] = isnull ? 'n' : ' ';
                        }
                        PG_FINALLY();
@@ -250,47 +266,23 @@ PLy_spi_execute_plan(PyObject *ob, PyObject *list, long limit)
                        PG_END_TRY();
                }
 
-               rv = SPI_execute_plan(plan->plan, plan->values, nulls,
+               MemoryContextSwitchTo(oldcontext);
+
+               rv = SPI_execute_plan(plan->plan, values, nulls,
                                                          exec_ctx->curr_proc->fn_readonly, limit);
                ret = PLy_spi_execute_fetch_result(SPI_tuptable, SPI_processed, rv);
 
-               if (nargs > 0)
-                       pfree(nulls);
-
+               MemoryContextDelete(tmpcontext);
                PLy_spi_subtransaction_commit(oldcontext, oldowner);
        }
        PG_CATCH();
        {
-               int                     k;
-
-               /*
-                * cleanup plan->values array
-                */
-               for (k = 0; k < nargs; k++)
-               {
-                       if (!plan->args[k].typbyval &&
-                               (plan->values[k] != PointerGetDatum(NULL)))
-                       {
-                               pfree(DatumGetPointer(plan->values[k]));
-                               plan->values[k] = PointerGetDatum(NULL);
-                       }
-               }
-
+               /* Subtransaction abort will remove the tmpcontext */
                PLy_spi_subtransaction_abort(oldcontext, oldowner);
                return NULL;
        }
        PG_END_TRY();
 
-       for (i = 0; i < nargs; i++)
-       {
-               if (!plan->args[i].typbyval &&
-                       (plan->values[i] != PointerGetDatum(NULL)))
-               {
-                       pfree(DatumGetPointer(plan->values[i]));
-                       plan->values[i] = PointerGetDatum(NULL);
-               }
-       }
-
        if (rv < 0)
        {
                PLy_exception_set(PLy_exc_spi_error,