memory leak in libxml2 - fix

On Fri, Nov 26, 2010 at 17:59, Pavel Stehule <pavel.stehule@gmail.com> wrote:

our customer showed a very significant memory leak in xml2.
It never release a memory allocated for context and doctree.

Why did you change doctree and ctxt to global variables?
I'm not sure why /* xmlFreeDoc(doctree); */ is commented out
at the end of pgxml_xpath(), but is it enough to enable the code?

--
Itagaki Takahiro

2010/11/26 Itagaki Takahiro <itagaki.takahiro@gmail.com>:

On Fri, Nov 26, 2010 at 17:59, Pavel Stehule <pavel.stehule@gmail.com> wrote:

our customer showed a very significant memory leak in xml2.
It never release a memory allocated for context and doctree.

Why did you change doctree and ctxt to global variables?
I'm not sure why /* xmlFreeDoc(doctree); */ is commented out
at the end of pgxml_xpath(), but is it enough to enable the code?

I am thinking, so you must not to call xmlFreeDoc(doctree) early.
Probably xmlXPathCastToXXX reading a doctree.

xmlXPathFreeContext(ctxt); xmlFreeDoc(doctree); are called now only
when function doesn't return a value.

Regards

Pavel Stehule

Pavel Stehule <pavel.stehule@gmail.com> writes:

2010/11/26 Itagaki Takahiro <itagaki.takahiro@gmail.com>:

Why did you change doctree and ctxt to global variables?
I'm not sure why /* xmlFreeDoc(doctree); */ is commented out
at the end of pgxml_xpath(), but is it enough to enable the code?

I am thinking, so you must not to call xmlFreeDoc(doctree) early.
Probably xmlXPathCastToXXX reading a doctree.

Those static variables are really ugly, and what's more this patch only
stops some of the leakage. Per experimentation, the result object from
pgxml_xpath has to be freed too, once it's been safely converted to
whatever the end result type is. You can see this by watching

select sum(xpath_number('<data>' || generate_series || '</data>','/data')) from
generate_series(1,500000);

which still shows leakage with the submitted patch. I cleaned it up
as per attached, which doesn't show any leakage.

regards, tom lane

Excerpts from Tom Lane's message of vie nov 26 16:14:08 -0300 2010:

Those static variables are really ugly, and what's more this patch only
stops some of the leakage. Per experimentation, the result object from
pgxml_xpath has to be freed too, once it's been safely converted to
whatever the end result type is. You can see this by watching

select sum(xpath_number('<data>' || generate_series || '</data>','/data')) from
generate_series(1,500000);

which still shows leakage with the submitted patch. I cleaned it up
as per attached, which doesn't show any leakage.

This looks great. As this fixes a problem that was reported to us two
days ago, I'm interested in backpatching it. Are you going to do it?
Otherwise I can work on that.

--
Álvaro Herrera <alvherre@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Alvaro Herrera <alvherre@commandprompt.com> writes:

This looks great. As this fixes a problem that was reported to us two
days ago, I'm interested in backpatching it. Are you going to do it?

Yeah, I'm on it. It's a bit tedious because the back branches are
different ...

regards, tom lane

2010/11/26 Tom Lane <tgl@sss.pgh.pa.us>:

Pavel Stehule <pavel.stehule@gmail.com> writes:

2010/11/26 Itagaki Takahiro <itagaki.takahiro@gmail.com>:

Why did you change doctree and ctxt to global variables?
I'm not sure why /* xmlFreeDoc(doctree); */ is commented out
at the end of pgxml_xpath(), but is it enough to enable the code?

I am thinking, so you must not to call xmlFreeDoc(doctree) early.
Probably xmlXPathCastToXXX reading a doctree.

Those static variables are really ugly, and what's more this patch only
stops some of the leakage.  Per experimentation, the result object from
pgxml_xpath has to be freed too, once it's been safely converted to
whatever the end result type is.  You can see this by watching

select sum(xpath_number('<data>' || generate_series || '</data>','/data')) from
generate_series(1,500000);

which still shows leakage with the submitted patch.  I cleaned it up
as per attached, which doesn't show any leakage.

great

thank you very much

regards

Pavel Stehule