Thread: Re: [BUGS] BUG #9223: plperlu result memory leak

Hello, All!

eshkinkot@gmail.com writes:

> create function perl_test(IN data text, OUT v1 text, OUT v2 integer, OUT v3
> integer, OUT v4 json, OUT v5 json)
>   returns record as
> $BODY$
>
> use strict;
> use warnings;
>
> my $res->{'v1'} = 'content';
>
> return $res;
>
> $BODY$
>   language plperlu volatile strict;

> test case:
> select count(perl_test('')) from generate_series(1, 1000000);

It looks like I found the problem, Perl use reference count and something that
is called "Mortal" for memory management.  As I understand it, mortal is free
after FREETMPS. Plperl call FREETMPS in plperl_call_perl_func() but after it,
plperl ask perl interpreter again for new mortal SV variables, for example, in
hek2cstr from plperl_sv_to_datum, and this new SV is newer freed.

I experiment with this patch, and it fix memory leak in my case, but patch
is incomplete. It does not free on error and fix only plperl_func_handler,
plperl_trigger_handler and may be plperl_inline_handler must be also fixed.

Patch for REL9_2_STABLE.

without patch:
  PID    VSZ   RSS
 2503  74112  7740
 2503 152928 86860
 2503 232208 165836
 2503 310732 244508
 2503 389264 323032

with patch:
  PID    VSZ   RSS
 4322  74112  7740
 4322  74380  8340
 4322  74380  8340
 4322  74380  8340
 4322  74380  8340


diff --git a/src/pl/plperl/plperl.c b/src/pl/plperl/plperl.c
index 49d50c4..9c9874d 100644
--- a/src/pl/plperl/plperl.c
+++ b/src/pl/plperl/plperl.c
@@ -2173,6 +2173,9 @@ plperl_func_handler(PG_FUNCTION_ARGS)
     ReturnSetInfo *rsi;
     ErrorContextCallback pl_error_context;

+    ENTER;
+    SAVETMPS;
+
     if (SPI_connect() != SPI_OK_CONNECT)
         elog(ERROR, "could not connect to SPI manager");

@@ -2271,6 +2274,9 @@ plperl_func_handler(PG_FUNCTION_ARGS)

     SvREFCNT_dec(perlret);

+    FREETMPS;
+    LEAVE;
+
     return retval;
 }

On Tue, Feb 25, 2014 at 6:56 AM, Sergey Burladyan <eshkinkot@gmail.com> wrote:

> It looks like I found the problem, Perl use reference count and something that
> is called "Mortal" for memory management.  As I understand it, mortal is free
> after FREETMPS. Plperl call FREETMPS in plperl_call_perl_func() but after it,
> plperl ask perl interpreter again for new mortal SV variables, for example, in
> hek2cstr from plperl_sv_to_datum, and this new SV is newer freed.

So I think hek2cstr is the only place we leak (its the only place I
can see that allocates a mortal sv without being wrapped in
ENTER/SAVETMPS/FREETMPS/LEAVE).

Does the attached fix it for you?

Alex Hunsaker <badalex@gmail.com> writes:

> On Tue, Feb 25, 2014 at 6:56 AM, Sergey Burladyan <eshkinkot@gmail.com> wrote:
>
> > It looks like I found the problem, Perl use reference count and something that
> > is called "Mortal" for memory management.  As I understand it, mortal is free
> > after FREETMPS. Plperl call FREETMPS in plperl_call_perl_func() but after it,
> > plperl ask perl interpreter again for new mortal SV variables, for example, in
> > hek2cstr from plperl_sv_to_datum, and this new SV is newer freed.
>
> So I think hek2cstr is the only place we leak (its the only place I
> can see that allocates a mortal sv without being wrapped in
> ENTER/SAVETMPS/FREETMPS/LEAVE).

Yeah, I also try to fix only hek2cstr, but failed.

> Does the attached fix it for you?

Yes, your patch is fix it for me, thank you, Alex!

Alex Hunsaker escribió:
> On Tue, Feb 25, 2014 at 6:56 AM, Sergey Burladyan <eshkinkot@gmail.com> wrote:
> 
> > It looks like I found the problem, Perl use reference count and something that
> > is called "Mortal" for memory management.  As I understand it, mortal is free
> > after FREETMPS. Plperl call FREETMPS in plperl_call_perl_func() but after it,
> > plperl ask perl interpreter again for new mortal SV variables, for example, in
> > hek2cstr from plperl_sv_to_datum, and this new SV is newer freed.
> 
> So I think hek2cstr is the only place we leak (its the only place I
> can see that allocates a mortal sv without being wrapped in
> ENTER/SAVETMPS/FREETMPS/LEAVE).
> 
> Does the attached fix it for you?

Can I bug you into verifying what supported releases need this patch,
and to which does it backpatch cleanly?  And if there's any to which it
doesn't, can I further bug you into providing one that does?

Thanks,

-- 
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Wed, Mar 5, 2014 at 12:22 PM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:

> Can I bug you into verifying what supported releases need this patch,
> and to which does it backpatch cleanly?  And if there's any to which it
> doesn't, can I further bug you into providing one that does?

Sure! Not bugging at all. I'll dig into this in a few hours.

On Wed, Mar 5, 2014 at 12:55 PM, Alex Hunsaker <badalex@gmail.com> wrote:
> On Wed, Mar 5, 2014 at 12:22 PM, Alvaro Herrera
> <alvherre@2ndquadrant.com> wrote:
>
>> Can I bug you into verifying what supported releases need this patch,
>> and to which does it backpatch cleanly?  And if there's any to which it
>> doesn't, can I further bug you into providing one that does?
>
> Sure! Not bugging at all. I'll dig into this in a few hours.

This will apply cleanly all the way to REL9_2_STABLE. It applies (with
fuzz, but cleanly to REL9_1). REL9_0 does this completely differently
and so does not have this leak.

Thanks!

Alex Hunsaker escribió:
> On Wed, Mar 5, 2014 at 12:55 PM, Alex Hunsaker <badalex@gmail.com> wrote:
> > On Wed, Mar 5, 2014 at 12:22 PM, Alvaro Herrera
> > <alvherre@2ndquadrant.com> wrote:
> >
> >> Can I bug you into verifying what supported releases need this patch,
> >> and to which does it backpatch cleanly?  And if there's any to which it
> >> doesn't, can I further bug you into providing one that does?
> >
> > Sure! Not bugging at all. I'll dig into this in a few hours.
> 
> This will apply cleanly all the way to REL9_2_STABLE. It applies (with
> fuzz, but cleanly to REL9_1). REL9_0 does this completely differently
> and so does not have this leak.

Excellent, thanks.  I verified all these assertions and then pushed.

-- 
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services