Horton

Thread

Commits

  1. Repair memory leaks in plpython.

  1. Memory leak in plpython3u (with testcase and patch)

    Mat Arye <mat@timescale.com> — 2025-01-10T18:43:00Z

    Hi all,

I found a memory leak in plpython3u. It happens when converting the
argument for an spi plan execution (via plpy.execute() or similar). I've
attached a sql file to create two plpython3u functions to reproduce the
issue (memory_leak_test.sql).

When running
```
set client_min_messages = 'debug';
select test_mem(1000);
```
You'll see that the memory goes to 720MB.

In contrast the test_mem_query function you won't see the leak (it uses
~70MB):
```
set client_min_messages = 'debug';
select test_mem_query(1000);
```
This is because test_mem_query executes the query using a sql string
instead of a spi plan with parameters.

I believe the issue is in the call to `PLy_output_convert` inside
`PLy_spi_execute_plan`. I've attached a patch that reduces the memory usage
to ~70MB from ~720 MB when using test_mem. I based what I did on what
`PLy_input_convert` does. But I am not an expert in this part of the code
so I am not sure the patch is correct. In particular, I don't fully grasp
the comment in `PLy_input_convert` about why the scratch is reset before
not after the conversion cycle and what that has to do with python
refcounts. A review by an expert would be appreciated.

Thanks,
Mat

-- 
Mat Arye, Technical lead of all thing AI @ Timescale

  2. Re: Memory leak in plpython3u (with testcase and patch)

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-01-10T21:37:56Z

    Mat Arye <mat@timescale.com> writes:
> I found a memory leak in plpython3u. It happens when converting the
> argument for an spi plan execution (via plpy.execute() or similar). I've
> attached a sql file to create two plpython3u functions to reproduce the
> issue (memory_leak_test.sql).

I see the leak all right, and your patch does seem to fix it, so
I believe your diagnosis that PLy_output_convert() is what's leaking.

> I believe the issue is in the call to `PLy_output_convert` inside
> `PLy_spi_execute_plan`. I've attached a patch that reduces the memory usage
> to ~70MB from ~720 MB when using test_mem. I based what I did on what
> `PLy_input_convert` does. But I am not an expert in this part of the code
> so I am not sure the patch is correct. In particular, I don't fully grasp
> the comment in `PLy_input_convert` about why the scratch is reset before
> not after the conversion cycle and what that has to do with python
> refcounts. A review by an expert would be appreciated.

The corresponding concern here would be that some pass-by-reference
conversion result Datum could come back verbatim as an output of
SPI_execute_plan, and we mustn't reset the scratch context again
before we're done with using the Datum.  Sadly, I don't think it
will work if that happens, because what we do as soon as
SPI_execute_plan finishes is to call PLy_spi_execute_fetch_result,
and that calls PLy_input_from_tuple, which is one of the things
that will zap the scratch context.  So this doesn't feel very safe
... and indeed it falls over in plpython's existing regression tests.

However, I don't really see why we need to use that scratch context.
PLy_spi_execute_plan runs a subtransaction, so we could perfectly well
use the subtransaction's CurTransactionContext.  (IOW, this wouldn't
even be an issue if PLy_spi_subtransaction_begin didn't forcibly
switch back to oldcontext.)

A couple of other points:

* PLy_spi_execute_plan already has an outer-level variable named
oldcontext, so this coding will draw complaints about a shadowed
local variable in compilers that are picky about that.  But
I don't think you need the inner oldcontext variable anyway, since you
know perfectly well that the previous context is the outer oldcontext.
Just switch back to that after using CurTransactionContext.

* Looking around at other callers of PLy_output_convert, it seems
likely that PLy_cursor_plan() has this same problem.  (I think we're
okay though with the calls from PLy_exec_function and
PLy_modify_tuple, since those could only happen right before return
from the plpython function; there's no path to iterate them enough
times to create a meaningful leak.)

			regards, tom lane

  3. Re: Memory leak in plpython3u (with testcase and patch)

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-01-10T22:45:22Z

    I wrote:
> However, I don't really see why we need to use that scratch context.
> PLy_spi_execute_plan runs a subtransaction, so we could perfectly well
> use the subtransaction's CurTransactionContext.

Nah, I'm wrong about that: I forgot CurTransactionContext goes away on
subtransaction abort, but not subtransaction commit.  So we really
need to make our own context with suitable lifespan.

I also noticed that the code is moving heaven and earth to store
the argument Datums ... but for some reason not the associated
isnull flags ... in the PLyPlanObject.  This is just nuts, because
the values don't need to survive past the functions that are computing
them.  In the cursor case, the values will be copied into the cursor
portal, so they still don't need to survive.  We can drop all of
that.

So I arrive at the attached.  (This is just for HEAD.  In the back
branches, we'd better leave the PLyPlanObject.values field in place
for ABI safety, but we don't have to use it.)

			regards, tom lane