Thread: Re: [COMMITTERS] pgsql: Fix plpgsql to release SPI plans when a function or DO block is

On Sun, Mar 27, 2011 at 04:51:13PM +0000, Tom Lane wrote:
> Fix plpgsql to release SPI plans when a function or DO block is freed.

Do the other PLs we ship need similar fixes?

Cheers,
David.
-- 
David Fetter <david@fetter.org> http://fetter.org/
Phone: +1 415 235 3778  AIM: dfetter666  Yahoo!: dfetter
Skype: davidfetter      XMPP: david.fetter@gmail.com
iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

David Fetter <david@fetter.org> writes:
> On Sun, Mar 27, 2011 at 04:51:13PM +0000, Tom Lane wrote:
>> Fix plpgsql to release SPI plans when a function or DO block is freed.

> Do the other PLs we ship need similar fixes?

Offhand I think the other PLs leave management of prepared plans to the
user.  If there are any places where they cache plans behind the scenes,
maybe a similar fix would be appropriate.
        regards, tom lane

On 28/03/11 04:31, Tom Lane wrote:
> David Fetter <david@fetter.org> writes:
>> On Sun, Mar 27, 2011 at 04:51:13PM +0000, Tom Lane wrote:
>>> Fix plpgsql to release SPI plans when a function or DO block is freed.
> 
>> Do the other PLs we ship need similar fixes?
> 
> Offhand I think the other PLs leave management of prepared plans to the
> user.  If there are any places where they cache plans behind the scenes,
> maybe a similar fix would be appropriate.

FWIW I executed

do $$ plpy.execute("select 1 from pg_class") $$ language plpythonu;

10k times in a session and the backend grew a lot in memory and never
released it. I can't offhand see where the memory went, I'll try to
investigate in the evening.

Cheers,
Jan

* Jan Urbański (wulczer@wulczer.org) wrote:
> On 28/03/11 04:31, Tom Lane wrote:
> >> Do the other PLs we ship need similar fixes?
> >
> > Offhand I think the other PLs leave management of prepared plans to the
> > user.  If there are any places where they cache plans behind the scenes,
> > maybe a similar fix would be appropriate.
>
> FWIW I executed
>
> do $$ plpy.execute("select 1 from pg_class") $$ language plpythonu;
>
> 10k times in a session and the backend grew a lot in memory and never
> released it. I can't offhand see where the memory went, I'll try to
> investigate in the evening.

I'm about 90% sure that they all have this problem..  I havn't had a
chance to look at how Tom fixed pl/pgsql (I didn't think it'd be easy to
do w/o coming up with a way to explicitly tell the PL to release
something) so perhaps I'm mistaken, but they all share very similar
code..
Thanks,
    Stephen

On 28/03/11 17:25, Stephen Frost wrote:
> * Jan Urbański (wulczer@wulczer.org) wrote:
>> On 28/03/11 04:31, Tom Lane wrote:
>>>> Do the other PLs we ship need similar fixes?
>>>
>>> Offhand I think the other PLs leave management of prepared plans to the
>>> user.  If there are any places where they cache plans behind the scenes,
>>> maybe a similar fix would be appropriate.
>>
>> FWIW I executed
>>
>> do $$ plpy.execute("select 1 from pg_class") $$ language plpythonu;
>>
>> 10k times in a session and the backend grew a lot in memory and never
>> released it. I can't offhand see where the memory went, I'll try to
>> investigate in the evening.
>
> I'm about 90% sure that they all have this problem..  I havn't had a
> chance to look at how Tom fixed pl/pgsql (I didn't think it'd be easy to
> do w/o coming up with a way to explicitly tell the PL to release
> something) so perhaps I'm mistaken, but they all share very similar
> code..

Valgrind showed me the way. PFA a trivial patch to avoid leaking a
PLyProcedure struct in inline blocks.

Prepared plans get cleaned up currectly, the SPI plan is deallocated
when the Python plan object is garbage collected.

Cheers,
Jan

On 30.03.2011 21:21, Jan Urbański wrote:
> Valgrind showed me the way. PFA a trivial patch to avoid leaking a
> PLyProcedure struct in inline blocks.

Hmm, any reason the PLyProcedure struct needs to be allocated in
TopMemoryContext in the first place? Could you palloc0 it in a
shorter-lived context, or even better, just allocate it in stack?

PS. I don't think the volatile qualifier in 'proc' is in necessary. The
variable is not changed in PG_TRY-block.

--
   Heikki Linnakangas
   EnterpriseDB   http://www.enterprisedb.com

On 31/03/11 07:35, Heikki Linnakangas wrote:
> On 30.03.2011 21:21, Jan Urbański wrote:
>> Valgrind showed me the way. PFA a trivial patch to avoid leaking a
>> PLyProcedure struct in inline blocks.
> 
> Hmm, any reason the PLyProcedure struct needs to be allocated in
> TopMemoryContext in the first place? Could you palloc0 it in a
> shorter-lived context, or even better, just allocate it in stack?

Yeah, you're right, you can keep it on the stack.

> PS. I don't think the volatile qualifier in 'proc' is in necessary. The
> variable is not changed in PG_TRY-block.

That always confuses me, but I guess you're right, the variable does not
change, only the memory it points to.

Cheers,
Jan

On 31.03.2011 12:30, Jan Urbański wrote:
> On 31/03/11 07:35, Heikki Linnakangas wrote:
>> On 30.03.2011 21:21, Jan Urbański wrote:
>>> Valgrind showed me the way. PFA a trivial patch to avoid leaking a
>>> PLyProcedure struct in inline blocks.
>>
>> Hmm, any reason the PLyProcedure struct needs to be allocated in
>> TopMemoryContext in the first place? Could you palloc0 it in a
>> shorter-lived context, or even better, just allocate it in stack?
>
> Yeah, you're right, you can keep it on the stack.
>
>> PS. I don't think the volatile qualifier in 'proc' is in necessary. The
>> variable is not changed in PG_TRY-block.
>
> That always confuses me, but I guess you're right, the variable does not
> change, only the memory it points to.

Ok then, committed.

--   Heikki Linnakangas  EnterpriseDB   http://www.enterprisedb.com