Horton

Thread

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Fix plpgsql to release SPI plans when a function or DO block is freed.

  1. pgsql: Fix plpgsql to release SPI plans when a function or DO block is

    Tom Lane <tgl@sss.pgh.pa.us> — 2011-03-27T16:51:13Z

    Fix plpgsql to release SPI plans when a function or DO block is freed.

This fixes the gripe I made a few months ago about DO blocks getting
slower with repeated use.  At least, it fixes it for the case where
the DO block isn't aborted by an error.  We could try running
plpgsql_free_function_memory() even during error exit, but that seems
a bit scary since it makes a lot of presumptions about the data
structures being in good shape.  It's probably reasonable to assume
that repeated failures of DO blocks isn't a performance-critical case.

Branch
------
master

Details
-------
http://git.postgresql.org/pg/commitdiff/87f2ad1326bff5cd37dde6fbf024137a2243efea

Modified Files
--------------
src/pl/plpgsql/src/pl_comp.c    |    7 +-
src/pl/plpgsql/src/pl_funcs.c   |  398 +++++++++++++++++++++++++++++++++++++++
src/pl/plpgsql/src/pl_handler.c |   10 +
src/pl/plpgsql/src/plpgsql.h    |    1 +
4 files changed, 411 insertions(+), 5 deletions(-)

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

    David Fetter <david@fetter.org> — 2011-03-27T21:07:13Z

    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

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2011-03-28T02:31:41Z

    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

  4. Re: pgsql: Fix plpgsql to release SPI plans when a function or DO block is

    Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> — 2011-03-28T07:25:06Z

    On 27.03.2011 19:51, Tom Lane wrote:
> Fix plpgsql to release SPI plans when a function or DO block is freed.
>
> This fixes the gripe I made a few months ago about DO blocks getting
> slower with repeated use.  At least, it fixes it for the case where
> the DO block isn't aborted by an error.  We could try running
> plpgsql_free_function_memory() even during error exit, but that seems
> a bit scary since it makes a lot of presumptions about the data
> structures being in good shape.  It's probably reasonable to assume
> that repeated failures of DO blocks isn't a performance-critical case.

I was quite surprised by the way you did this. Instead of adding all 
that code to traverse the PLpgSQL_stmt tree (that we'll have to remember 
to keep up-to-date), can't we just have a list of cached plans in 
PLpgSQL_function? As attached.

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

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

    Jan Urbański <wulczer@wulczer.org> — 2011-03-28T10:21:25Z

    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

  6. Re: pgsql: Fix plpgsql to release SPI plans when a function or DO block is

    Tom Lane <tgl@sss.pgh.pa.us> — 2011-03-28T13:21:43Z

    Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> writes:
> On 27.03.2011 19:51, Tom Lane wrote:
>> Fix plpgsql to release SPI plans when a function or DO block is freed.

> I was quite surprised by the way you did this. Instead of adding all 
> that code to traverse the PLpgSQL_stmt tree (that we'll have to remember 
> to keep up-to-date), can't we just have a list of cached plans in 
> PLpgSQL_function? As attached.

The code I added is a direct rip-off of the adjacent function tree
dumping code, which we've had no trouble keeping up-to-date.  (I checked
it as I went through, and it did not appear to be missing any cases.)
I'm unconvinced that adding a separate new mechanism to track the
function's SPI plans would be more easily maintainable or less prone to
oversights.  Also, what if we come up with another resource that has to
be explicitly released here?  The tree walk won't require additional
mechanism for that.

			regards, tom lane

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

    Stephen Frost <sfrost@snowman.net> — 2011-03-28T15:25:39Z

    * 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

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

    Jan Urbański <wulczer@wulczer.org> — 2011-03-30T18:21:05Z

    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

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

    Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> — 2011-03-31T05:35:08Z

    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

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

    Jan Urbański <wulczer@wulczer.org> — 2011-03-31T09:30:29Z

    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

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

    Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> — 2011-03-31T09:43:26Z

    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