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 6108384c9a50c1b584020a19a55b2943e3323b37..bb3fa8a390990b002ffe256b97b90c8b8d72e2fe 100644 (file)
--- a/src/pl/plpython/plpy_cursorobject.c
+++ b/src/pl/plpython/plpy_cursorobject.c
@@ -140,7 +140,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;
@@ -199,13 +198,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++)
        {
@@ -217,7 +233,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(2);
@@ -227,7 +243,9 @@ PLy_cursor_plan(PyObject *ob, PyObject *args)
            PG_END_TRY(2);
        }
 
-       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",
@@ -237,40 +255,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_planobject.c b/src/pl/plpython/plpy_planobject.c
index bbef889329e8854750040548294bc6f18003d7ab..9427674d2f4a6506ad7f6fb6f80d046e18eac903 100644 (file)
--- a/src/pl/plpython/plpy_planobject.c
+++ b/src/pl/plpython/plpy_planobject.c
@@ -54,7 +54,6 @@ PLy_plan_new(void)
    ob->plan = NULL;
    ob->nargs = 0;
    ob->types = NULL;
-   ob->values = NULL;
    ob->args = NULL;
    ob->mcxt = NULL;
 

diff --git a/src/pl/plpython/plpy_planobject.h b/src/pl/plpython/plpy_planobject.h
index 729effb1631daafd2d9cb9caf738f64054ae0c41..a6b34fae199b150b6fbc0588553511ebb1afef70 100644 (file)
--- a/src/pl/plpython/plpy_planobject.h
+++ b/src/pl/plpython/plpy_planobject.h
@@ -15,7 +15,6 @@ typedef struct PLyPlanObject
    SPIPlanPtr  plan;
    int         nargs;
    Oid        *types;
-   Datum      *values;
    PLyObToDatum *args;
    MemoryContext mcxt;
 } PLyPlanObject;

diff --git a/src/pl/plpython/plpy_spi.c b/src/pl/plpython/plpy_spi.c
index bcbd07b70ae98d844efe928a7d1cbe0f6b128ac2..77fbfd6c86819852e1cbbae33e4a9abc996eca9b 100644 (file)
--- a/src/pl/plpython/plpy_spi.c
+++ b/src/pl/plpython/plpy_spi.c
@@ -66,7 +66,6 @@ PLy_spi_prepare(PyObject *self, PyObject *args)
 
    plan->nargs = nargs;
    plan->types = nargs ? palloc0(sizeof(Oid) * nargs) : NULL;
-   plan->values = nargs ? palloc0(sizeof(Datum) * nargs) : NULL;
    plan->args = nargs ? palloc0(sizeof(PLyObToDatum) * nargs) : NULL;
 
    MemoryContextSwitchTo(oldcontext);
@@ -172,8 +171,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;
@@ -219,13 +217,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++)
        {
@@ -237,7 +252,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(2);
@@ -247,47 +262,23 @@ PLy_spi_execute_plan(PyObject *ob, PyObject *list, long limit)
            PG_END_TRY(2);
        }
 
-       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,