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 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

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

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

* 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