Re: Memory Errors...

"Ian Harding" <ianh(at)tpchd(dot)org> writes:
> It is pltcl [not plpgsql]

Ah. I don't think we've done much of any work on plugging leaks in
pltcl :-(.

> It hurts when I do this:

> drop function memleak();
> create function memleak() returns int as '
> for {set counter 1} {$counter < 100000} {incr counter} {
> set sql "select ''foo''"
> spi_exec "$sql"
> }
> ' language 'pltcl';
> select memleak();

Yeah, I see very quick memory exhaustion also :-(. Looks like the
spi_exec call is the culprit, but I'm not sure exactly why ...
anyone have time to look at this?

regards, tom lane

I said:
> Yeah, I see very quick memory exhaustion also :-(. Looks like the
> spi_exec call is the culprit, but I'm not sure exactly why ...
> anyone have time to look at this?

On looking a little more closely, it's clear that pltcl_SPI_exec()
should be, and is not, calling SPI_freetuptable() once it's done with
the tuple table returned by SPI_exec(). This needs to be done in all
the non-elog code paths after SPI_exec has returned SPI_OK_SELECT.
pltcl_SPI_execp() has a similar problem, and there may be comparable
bugs in other pltcl routines (not to mention other sources of memory
leaks, but I think this is the problem for your example).

I have no time to work on this right now; any volunteers out there?

regards, tom lane

Tom Lane wrote:
> I said:
>
>>Yeah, I see very quick memory exhaustion also :-(. Looks like the
>>spi_exec call is the culprit, but I'm not sure exactly why ...
>>anyone have time to look at this?
>
>
> On looking a little more closely, it's clear that pltcl_SPI_exec()
> should be, and is not, calling SPI_freetuptable() once it's done with
> the tuple table returned by SPI_exec(). This needs to be done in all
> the non-elog code paths after SPI_exec has returned SPI_OK_SELECT.
> pltcl_SPI_execp() has a similar problem, and there may be comparable
> bugs in other pltcl routines (not to mention other sources of memory
> leaks, but I think this is the problem for your example).
>
> I have no time to work on this right now; any volunteers out there?
>

I can give it a shot, but probably not until the weekend.

I haven't really followed this thread closely, and don't know tcl very well,
so it would help if someone can send me a minimal tcl function which triggers
the problem.

Thanks,

Joe

> "Ian Harding" <ianh(at)tpchd(dot)org> writes:
> > It is pltcl [not plpgsql]

Quick, minor point, in the manner of a question:

Why is the pltcl directory called tcl where all the other pls are pl<language>?

That's in src/pl of course. Also in my anoncvs fetch which is a few weeks old
now being from the day before beta freeze.

--
Nigel J. Andrews
Director

---
Logictree Systems Limited
Computer Consultants

Nigel J. Andrews wrote:
>
> > "Ian Harding" <ianh(at)tpchd(dot)org> writes:
> > > It is pltcl [not plpgsql]
>
> Quick, minor point, in the manner of a question:
>
> Why is the pltcl directory called tcl where all the other pls are pl<language>?

I asked the same question a while ago. I asked about changing it but
others didn't want the change. It is hard to rename stuff in CVS and
keep proper history.

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

"Nigel J. Andrews" <nandrews(at)investsystems(dot)co(dot)uk> writes:
> Why is the pltcl directory called tcl where all the other pls are pl<language>?

Consistency? We don't need no steenking consistency!

Personally I'd prefer to remove the pl prefix from the other
subdirectories of src/pl/ ... it seems redundantly wasted excessive
typing ;-) And I'd have preferred to flatten out the src/ subdirectory
of src/pl/[pl]pgsql, which is likewise redundant and inconsistent with
the other PLs.

However, it's fairly painful to make any such change without losing
the CVS version history for the moved files, which is Not a Good Thing.
Or breaking our ability to reconstitute old releases from the CVS tree,
which is Much Worse. So I'm afraid we're stuck with this historical
mischance.

regards, tom lane

On Thu, 19 Sep 2002, Joe Conway wrote:

> Tom Lane wrote:
> > I said:
> >
> >>Yeah, I see very quick memory exhaustion also :-(. Looks like the
> >>spi_exec call is the culprit, but I'm not sure exactly why ...
> >>anyone have time to look at this?
> >
> >
> > On looking a little more closely, it's clear that pltcl_SPI_exec()
> > should be, and is not, calling SPI_freetuptable() once it's done with
> > the tuple table returned by SPI_exec(). This needs to be done in all
> > the non-elog code paths after SPI_exec has returned SPI_OK_SELECT.
> > pltcl_SPI_execp() has a similar problem, and there may be comparable
> > bugs in other pltcl routines (not to mention other sources of memory
> > leaks, but I think this is the problem for your example).
> >
> > I have no time to work on this right now; any volunteers out there?
> >
>
> I can give it a shot, but probably not until the weekend.
>
> I haven't really followed this thread closely, and don't know tcl very well,
> so it would help if someone can send me a minimal tcl function which triggers
> the problem.

I can probably take a look at this tomorrow, already started by looking at the
pltcl_SPI_exec routine. I think a quick glance at ...init_unknown() also shows
a lack of tuptable freeing.

--
Nigel J. Andrews

Nigel J. Andrews wrote:
> On Thu, 19 Sep 2002, Joe Conway wrote:
>>I can give it a shot, but probably not until the weekend.
>>
>>I haven't really followed this thread closely, and don't know tcl very well,
>>so it would help if someone can send me a minimal tcl function which triggers
>>the problem.
>
> I can probably take a look at this tomorrow, already started by looking at the
> pltcl_SPI_exec routine. I think a quick glance at ...init_unknown() also shows
> a lack of tuptable freeing.
>

OK -- let me know if you can't find the time and I'll jump back in to it.

Joe

On Thu, 19 Sep 2002, Tom Lane wrote:

> "Ian Harding" <ianh(at)tpchd(dot)org> writes:
> > It is pltcl [not plpgsql]
>
> Ah. I don't think we've done much of any work on plugging leaks in
> pltcl :-(.
>
> > It hurts when I do this:
>
> > drop function memleak();
> > create function memleak() returns int as '
> > for {set counter 1} {$counter < 100000} {incr counter} {
> > set sql "select ''foo''"
> > spi_exec "$sql"
> > }
> > ' language 'pltcl';
> > select memleak();
>
> Yeah, I see very quick memory exhaustion also :-(. Looks like the
> spi_exec call is the culprit, but I'm not sure exactly why ...
> anyone have time to look at this?

Attached is a patch that frees the SPI_tuptable in all post SPI_exec
non-elog paths in both pltcl_SPI_exec() and pltcl_SPI_execp().

The fault as triggered by the above code has been fixed by this patch but
please read my assumptions below to ensure they are correct.

I have assumed that Tom's comment about this only being required in non-elog
paths is correct, which seems a reasonable assumption to me.

I have also assumed, rather than verified, that freeing the tuptable does
indeed free the tuples as well. Tests with the above function show that the
process does not increase it's memory footprint during it's operation, although
if my assumption here is wrong this could be a feature of selecting
insignificantly sized tuples.

I have not worried about other uses of SPI_exec for selects in pltcl.c on the
basis that those are not under the control of the function writer and the
normal function management will release the storage.

--
Nigel J. Andrews

Tom Lane writes:

> On looking a little more closely, it's clear that pltcl_SPI_exec()
> should be, and is not, calling SPI_freetuptable() once it's done with
> the tuple table returned by SPI_exec(). This needs to be done in all
> the non-elog code paths after SPI_exec has returned SPI_OK_SELECT.

There's a note in the PL/Python documentation that it's leaking memory if
SPI plans are used. Maybe that's related and someone could take a look at
it.

--
Peter Eisentraut peter_e(at)gmx(dot)net

I'll try to have a look-see by the end of the weekend. Any code that
can reproduce it or is it ANY code that uses SPI?

Greg

On Fri, 2002-09-20 at 11:39, Peter Eisentraut wrote:
> Tom Lane writes:
>
> > On looking a little more closely, it's clear that pltcl_SPI_exec()
> > should be, and is not, calling SPI_freetuptable() once it's done with
> > the tuple table returned by SPI_exec(). This needs to be done in all
> > the non-elog code paths after SPI_exec has returned SPI_OK_SELECT.
>
> There's a note in the PL/Python documentation that it's leaking memory if
> SPI plans are used. Maybe that's related and someone could take a look at
> it.
>
> --
> Peter Eisentraut peter_e(at)gmx(dot)net
>
>
> ---------------------------(end of broadcast)---------------------------
> TIP 4: Don't 'kill -9' the postmaster

On 20 Sep 2002, Greg Copeland wrote:

> I'll try to have a look-see by the end of the weekend. Any code that
> can reproduce it or is it ANY code that uses SPI?
>
> Greg
>
>
> On Fri, 2002-09-20 at 11:39, Peter Eisentraut wrote:
> > Tom Lane writes:
> >
> > > On looking a little more closely, it's clear that pltcl_SPI_exec()
> > > should be, and is not, calling SPI_freetuptable() once it's done with
> > > the tuple table returned by SPI_exec(). This needs to be done in all
> > > the non-elog code paths after SPI_exec has returned SPI_OK_SELECT.
> >
> > There's a note in the PL/Python documentation that it's leaking memory if
> > SPI plans are used. Maybe that's related and someone could take a look at
> > it.

I've added the call to free the tuptable just as in the pltcl patch I submited
earlier (which I can't remember if I've seen in the list so I may well resend).

However, the comments in the code imply there might be another leak with
prepared plans. I'm looking into that so I won't be sending this patch just
yet.

--
Nigel J. Andrews

Ok, below is the original email I sent, which I can not remember seeing come
across the patches list. Please do read the assumptions since they might throw
up problems with what I have done.

I have attached the pltcl patch again, just in case. For the sake of clarity
let's say this patch superscedes the previous one.

I have also attached a patch addressing the similar memory leak problem in
plpython. This includes a slight adjustment of the tests in the source
directory. The patch also includes a cosmetic change to remove a compiler
warning although I think the change makes the code look worse though.

Once again, please read my text below and also take a quick look at the comment
I've added in the plpython patch since it may well show that that
particular change is complete rubbish.

BTW, by my reckoning the memory leak would occur with prepared plans and
without. If that is not the case then I've been barking up the wrong tree.

Of further note, I have not tested for the memory leak in plpython but the
build passes the normal and big checks. However, I have tried testing using the
test.sh script in src/pl/plpython. This seems to be generating errors where
before there were warnings. Can anyone comment on the correctness of this?
Reversing my changes doesn't really help matters so I presume it is something
else that is causing the different behaviour.

--
Nigel J. Andrews

On Fri, 20 Sep 2002, Nigel J. Andrews wrote:

> On Thu, 19 Sep 2002, Tom Lane wrote:
>
> > "Ian Harding" <ianh(at)tpchd(dot)org> writes:
> > > It is pltcl [not plpgsql]
> >
> > Ah. I don't think we've done much of any work on plugging leaks in
> > pltcl :-(.
> >
> > > It hurts when I do this:
> >
> > > drop function memleak();
> > > create function memleak() returns int as '
> > > for {set counter 1} {$counter < 100000} {incr counter} {
> > > set sql "select ''foo''"
> > > spi_exec "$sql"
> > > }
> > > ' language 'pltcl';
> > > select memleak();
> >
> > Yeah, I see very quick memory exhaustion also :-(. Looks like the
> > spi_exec call is the culprit, but I'm not sure exactly why ...
> > anyone have time to look at this?
>
> Attached is a patch that frees the SPI_tuptable in all post SPI_exec
> non-elog paths in both pltcl_SPI_exec() and pltcl_SPI_execp().
>
> The fault as triggered by the above code has been fixed by this patch but
> please read my assumptions below to ensure they are correct.
>
> I have assumed that Tom's comment about this only being required in non-elog
> paths is correct, which seems a reasonable assumption to me.
>
> I have also assumed, rather than verified, that freeing the tuptable does
> indeed free the tuples as well. Tests with the above function show that the
> process does not increase it's memory footprint during it's operation, although
> if my assumption here is wrong this could be a feature of selecting
> insignificantly sized tuples.
>
> I have not worried about other uses of SPI_exec for selects in pltcl.c on the
> basis that those are not under the control of the function writer and the
> normal function management will release the storage.

Your patch has been added to the PostgreSQL unapplied patches list at:

http://candle.pha.pa.us/cgi-bin/pgpatches

I will try to apply it within the next 48 hours.

---------------------------------------------------------------------------

Nigel J. Andrews wrote:
> On Thu, 19 Sep 2002, Tom Lane wrote:
>
> > "Ian Harding" <ianh(at)tpchd(dot)org> writes:
> > > It is pltcl [not plpgsql]
> >
> > Ah. I don't think we've done much of any work on plugging leaks in
> > pltcl :-(.
> >
> > > It hurts when I do this:
> >
> > > drop function memleak();
> > > create function memleak() returns int as '
> > > for {set counter 1} {$counter < 100000} {incr counter} {
> > > set sql "select ''foo''"
> > > spi_exec "$sql"
> > > }
> > > ' language 'pltcl';
> > > select memleak();
> >
> > Yeah, I see very quick memory exhaustion also :-(. Looks like the
> > spi_exec call is the culprit, but I'm not sure exactly why ...
> > anyone have time to look at this?
>
> Attached is a patch that frees the SPI_tuptable in all post SPI_exec
> non-elog paths in both pltcl_SPI_exec() and pltcl_SPI_execp().
>
> The fault as triggered by the above code has been fixed by this patch but
> please read my assumptions below to ensure they are correct.
>
> I have assumed that Tom's comment about this only being required in non-elog
> paths is correct, which seems a reasonable assumption to me.
>
> I have also assumed, rather than verified, that freeing the tuptable does
> indeed free the tuples as well. Tests with the above function show that the
> process does not increase it's memory footprint during it's operation, although
> if my assumption here is wrong this could be a feature of selecting
> insignificantly sized tuples.
>
> I have not worried about other uses of SPI_exec for selects in pltcl.c on the
> basis that those are not under the control of the function writer and the
> normal function management will release the storage.
>
>
> --
> Nigel J. Andrews

Content-Description:

[ Attachment, skipping... ]

>
> ---------------------------(end of broadcast)---------------------------
> TIP 5: Have you checked our extensive FAQ?
>
> http://www.postgresql.org/users-lounge/docs/faq.html

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

[ Previous version removed from patches queue..]

Thanks for doing both interfaces.

Your patch has been added to the PostgreSQL unapplied patches list at:

http://candle.pha.pa.us/cgi-bin/pgpatches

I will try to apply it within the next 48 hours.

---------------------------------------------------------------------------

Nigel J. Andrews wrote:
>
> Ok, below is the original email I sent, which I can not remember seeing come
> across the patches list. Please do read the assumptions since they might throw
> up problems with what I have done.
>
> I have attached the pltcl patch again, just in case. For the sake of clarity
> let's say this patch superscedes the previous one.
>
> I have also attached a patch addressing the similar memory leak problem in
> plpython. This includes a slight adjustment of the tests in the source
> directory. The patch also includes a cosmetic change to remove a compiler
> warning although I think the change makes the code look worse though.
>
> Once again, please read my text below and also take a quick look at the comment
> I've added in the plpython patch since it may well show that that
> particular change is complete rubbish.
>
> BTW, by my reckoning the memory leak would occur with prepared plans and
> without. If that is not the case then I've been barking up the wrong tree.
>
> Of further note, I have not tested for the memory leak in plpython but the
> build passes the normal and big checks. However, I have tried testing using the
> test.sh script in src/pl/plpython. This seems to be generating errors where
> before there were warnings. Can anyone comment on the correctness of this?
> Reversing my changes doesn't really help matters so I presume it is something
> else that is causing the different behaviour.
>
>
> --
> Nigel J. Andrews
>
>
> On Fri, 20 Sep 2002, Nigel J. Andrews wrote:
>
> > On Thu, 19 Sep 2002, Tom Lane wrote:
> >
> > > "Ian Harding" <ianh(at)tpchd(dot)org> writes:
> > > > It is pltcl [not plpgsql]
> > >
> > > Ah. I don't think we've done much of any work on plugging leaks in
> > > pltcl :-(.
> > >
> > > > It hurts when I do this:
> > >
> > > > drop function memleak();
> > > > create function memleak() returns int as '
> > > > for {set counter 1} {$counter < 100000} {incr counter} {
> > > > set sql "select ''foo''"
> > > > spi_exec "$sql"
> > > > }
> > > > ' language 'pltcl';
> > > > select memleak();
> > >
> > > Yeah, I see very quick memory exhaustion also :-(. Looks like the
> > > spi_exec call is the culprit, but I'm not sure exactly why ...
> > > anyone have time to look at this?
> >
> > Attached is a patch that frees the SPI_tuptable in all post SPI_exec
> > non-elog paths in both pltcl_SPI_exec() and pltcl_SPI_execp().
> >
> > The fault as triggered by the above code has been fixed by this patch but
> > please read my assumptions below to ensure they are correct.
> >
> > I have assumed that Tom's comment about this only being required in non-elog
> > paths is correct, which seems a reasonable assumption to me.
> >
> > I have also assumed, rather than verified, that freeing the tuptable does
> > indeed free the tuples as well. Tests with the above function show that the
> > process does not increase it's memory footprint during it's operation, although
> > if my assumption here is wrong this could be a feature of selecting
> > insignificantly sized tuples.
> >
> > I have not worried about other uses of SPI_exec for selects in pltcl.c on the
> > basis that those are not under the control of the function writer and the
> > normal function management will release the storage.
>

Content-Description:

[ Attachment, skipping... ]

Content-Description:

[ Attachment, skipping... ]

Content-Description:

[ Attachment, skipping... ]

>
> ---------------------------(end of broadcast)---------------------------
> TIP 6: Have you searched our list archives?
>
> http://archives.postgresql.org

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

Well, it looks like it was already taken to the mat.

;)

Greg

On Thu, 2002-09-19 at 16:58, Joe Conway wrote:
> Nigel J. Andrews wrote:
> > On Thu, 19 Sep 2002, Joe Conway wrote:
> >>I can give it a shot, but probably not until the weekend.
> >>
> >>I haven't really followed this thread closely, and don't know tcl very well,
> >>so it would help if someone can send me a minimal tcl function which triggers
> >>the problem.
> >
> > I can probably take a look at this tomorrow, already started by looking at the
> > pltcl_SPI_exec routine. I think a quick glance at ...init_unknown() also shows
> > a lack of tuptable freeing.
> >
>
> OK -- let me know if you can't find the time and I'll jump back in to it.
>
> Joe
>
>
>
>
> ---------------------------(end of broadcast)---------------------------
> TIP 5: Have you checked our extensive FAQ?
>
> http://www.postgresql.org/users-lounge/docs/faq.html

Patch applied. Thanks.

---------------------------------------------------------------------------

Nigel J. Andrews wrote:
>
> Ok, below is the original email I sent, which I can not remember seeing come
> across the patches list. Please do read the assumptions since they might throw
> up problems with what I have done.
>
> I have attached the pltcl patch again, just in case. For the sake of clarity
> let's say this patch superscedes the previous one.
>
> I have also attached a patch addressing the similar memory leak problem in
> plpython. This includes a slight adjustment of the tests in the source
> directory. The patch also includes a cosmetic change to remove a compiler
> warning although I think the change makes the code look worse though.
>
> Once again, please read my text below and also take a quick look at the comment
> I've added in the plpython patch since it may well show that that
> particular change is complete rubbish.
>
> BTW, by my reckoning the memory leak would occur with prepared plans and
> without. If that is not the case then I've been barking up the wrong tree.
>
> Of further note, I have not tested for the memory leak in plpython but the
> build passes the normal and big checks. However, I have tried testing using the
> test.sh script in src/pl/plpython. This seems to be generating errors where
> before there were warnings. Can anyone comment on the correctness of this?
> Reversing my changes doesn't really help matters so I presume it is something
> else that is causing the different behaviour.
>
>
> --
> Nigel J. Andrews
>
>
> On Fri, 20 Sep 2002, Nigel J. Andrews wrote:
>
> > On Thu, 19 Sep 2002, Tom Lane wrote:
> >
> > > "Ian Harding" <ianh(at)tpchd(dot)org> writes:
> > > > It is pltcl [not plpgsql]
> > >
> > > Ah. I don't think we've done much of any work on plugging leaks in
> > > pltcl :-(.
> > >
> > > > It hurts when I do this:
> > >
> > > > drop function memleak();
> > > > create function memleak() returns int as '
> > > > for {set counter 1} {$counter < 100000} {incr counter} {
> > > > set sql "select ''foo''"
> > > > spi_exec "$sql"
> > > > }
> > > > ' language 'pltcl';
> > > > select memleak();
> > >
> > > Yeah, I see very quick memory exhaustion also :-(. Looks like the
> > > spi_exec call is the culprit, but I'm not sure exactly why ...
> > > anyone have time to look at this?
> >
> > Attached is a patch that frees the SPI_tuptable in all post SPI_exec
> > non-elog paths in both pltcl_SPI_exec() and pltcl_SPI_execp().
> >
> > The fault as triggered by the above code has been fixed by this patch but
> > please read my assumptions below to ensure they are correct.
> >
> > I have assumed that Tom's comment about this only being required in non-elog
> > paths is correct, which seems a reasonable assumption to me.
> >
> > I have also assumed, rather than verified, that freeing the tuptable does
> > indeed free the tuples as well. Tests with the above function show that the
> > process does not increase it's memory footprint during it's operation, although
> > if my assumption here is wrong this could be a feature of selecting
> > insignificantly sized tuples.
> >
> > I have not worried about other uses of SPI_exec for selects in pltcl.c on the
> > basis that those are not under the control of the function writer and the
> > normal function management will release the storage.
>

Content-Description:

[ Attachment, skipping... ]

Content-Description:

[ Attachment, skipping... ]

Content-Description:

[ Attachment, skipping... ]

>
> ---------------------------(end of broadcast)---------------------------
> TIP 6: Have you searched our list archives?
>
> http://archives.postgresql.org

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073