Thread: Re: pltcl.so patch
In answer to the question posed at the end of the message below:
Yes, I do get the similar results.
A quick investigation shows that the SPI_freetuptable at the end of
pltcl_SPI_exec is trying to free a tuptable of value 0x82ebe64 (which looks
sensible to me) but which has a memory context of 0x7f7f7f7f (the unallocated
marker).
Briefly following through to check this value shows that as long as I have
CLOBBER_FREED_MEMORY defined, which I presume I do having configured with
--debug, this value is also consistent with the tuptable having been freed
before this faulting invocation.
I haven't looked too closely yet but at a glance I can't see what could be
going wrong with the exception that the tuptable is freed even if zero rows are
returned by SPI_exec. That and I'm not sure what that $T(id) thing is doing in
the SQL submited to pltcl_SPI_exec. Oh 'eck, I've been reading that test
function wrong, it's got a level of nesting.
Unfortunately, I am currently trying to throw together a quick demo of
something at the moment so can't investigate too fully for the next day or so.
If someone wants to pick this up feel free otherwise I'll look into it later.
--
Nigel J. Andrews
On Tue, 24 Sep 2002, Ian Harding wrote to me:
> First, thank you very much for working on this issue. Pltcl is extremely important to me right now, and this memory
leakis cramping my style a bit.
>
> I applied the patch you sent to my pltcl.c (I am at version 7.2.1, but it seems to apply fine...) It builds fine,
psqlstarts fine, but my test function still blows up dramatically.
>
> Here is the script I am using:
>
> drop function memleak();
> create function memleak() returns int as '
>
> for {set i 1} {$i < 100} {incr i} {
> set sql "select ''foo''"
> spi_exec "$sql"
> }
>
>
> ' language 'pltcl';
>
> drop table testable;
> create table testable (
> id int,
> data text);
>
> insert into testable values (1, 'foobar');
> insert into testable values (2, 'foobar');
> insert into testable values (3, 'foobar');
> insert into testable values (4, 'foobar');
> insert into testable values (5, 'foobar');
> insert into testable values (6, 'foobar');
>
> drop function memleak(int);
> create function memleak(int) returns int as '
>
> set sql "select * From testable"
> spi_exec -array T "$sql" {
>
> for {set i 1} {$i < 100} {incr i} {
> set sql "select * from testable where id = $T(id)"
> spi_exec "$sql"
> }
> }
> ' language 'pltcl';
>
> Here is what happens:
>
> bash-2.05# psql -U iharding test < testfunction
> DROP
> CREATE
> ERROR: table "testable" does not exist
> CREATE
> INSERT 118942676 1
> INSERT 118942677 1
> INSERT 118942678 1
> INSERT 118942679 1
> INSERT 118942680 1
> INSERT 118942681 1
> DROP
> CREATE
> bash-2.05# psql -U iharding test
> Welcome to psql, the PostgreSQL interactive terminal.
>
> Type: \copyright for distribution terms
> \h for help with SQL commands
> \? for help on internal slash commands
> \g or terminate with semicolon to execute query
> \q to quit
>
> test=# select memleak();
> memleak
> ---------
> 0
> (1 row)
>
> test=# select memleak(1);
> server closed the connection unexpectedly
> This probably means the server terminated abnormally
> before or while processing the request.
> The connection to the server was lost. Attempting reset: Failed.
> !#
>
>
> Here is the end of the log:
>
> DEBUG: server process (pid 1992) was terminated by signal 11
> DEBUG: terminating any other active server processes
> DEBUG: all server processes terminated; reinitializing shared memory and semaphores
> IpcMemoryCreate: shmget(key=5432001, size=29769728, 03600) failed: Cannot allocate memory
>
> This error usually means that PostgreSQL's request for a shared
> ...
>
>
> Do you have similar results?
"Nigel J. Andrews" <nandrews@investsystems.co.uk> writes:
> Yes, I do get the similar results.
>
> A quick investigation shows that the SPI_freetuptable at the end of
> pltcl_SPI_exec is trying to free a tuptable of value 0x82ebe64
> (which looks sensible to me) but which has a memory context of
> 0x7f7f7f7f (the unallocated marker).
Attached is a patch against CVS HEAD which fixes this, I believe. The
problem appears to be the newly added free of the tuptable at the end
of pltcl_SPI_exec(). I've added a comment to that effect:
/*
* Do *NOT* free the tuptable here. That's because if the loop
* body executed any SQL statements, it will have already free'd
* the tuptable itself, so freeing it twice is not wise. We could
* get around this by making a copy of SPI_tuptable->vals and
* feeding that to pltcl_set_tuple_values above, but that would
* still leak memory (the palloc'ed copy would only be free'd on
* context reset).
*/
At least, I *think* that's the problem -- I've only been looking at
the code for about 20 minutes, so I may be wrong. In any case, this
makes both memleak() and memleak(1) work on my machine. Let me know if
it works for you, and/or if someone knows of a better solution.
I also added some SPI_freetuptable() calls in some places where Nigel
didn't, and added some paranoia when dealing with statically sized
buffers (snprintf() rather than sprintf(), and so on). I also didn't
include Nigel's changes to some apparently unrelated PL/Python stuff
-- this patch includes only the PL/Tcl changes.
Cheers,
Neil
--
Neil Conway <neilc@samurai.com> || PGP Key ID: DB3C29FC
Attachment
On 25 Sep 2002, Neil Conway wrote: > "Nigel J. Andrews" <nandrews@investsystems.co.uk> writes: > > Yes, I do get the similar results. > > > > A quick investigation shows that the SPI_freetuptable at the end of > > pltcl_SPI_exec is trying to free a tuptable of value 0x82ebe64 > > (which looks sensible to me) but which has a memory context of > > 0x7f7f7f7f (the unallocated marker). > > Attached is a patch against CVS HEAD which fixes this, I believe. The > problem appears to be the newly added free of the tuptable at the end > of pltcl_SPI_exec(). I've added a comment to that effect: > > /* > * Do *NOT* free the tuptable here. That's because if the loop > * body executed any SQL statements, it will have already free'd > * the tuptable itself, so freeing it twice is not wise. We could > * get around this by making a copy of SPI_tuptable->vals and > * feeding that to pltcl_set_tuple_values above, but that would > * still leak memory (the palloc'ed copy would only be free'd on > * context reset). > */ That's certainly where the fault was happening. However, that's where the original memory leak problem was coming from (without the SPI_freetuptable call). It could be I got that fix wrong and the extra calls you've added are the right fix for that. I'll take a look to see what I can learn later. > At least, I *think* that's the problem -- I've only been looking at > the code for about 20 minutes, so I may be wrong. In any case, this > makes both memleak() and memleak(1) work on my machine. Let me know if > it works for you, and/or if someone knows of a better solution. I'll have to check later. > > I also added some SPI_freetuptable() calls in some places where Nigel > didn't, and added some paranoia when dealing with statically sized > buffers (snprintf() rather than sprintf(), and so on). I also didn't > include Nigel's changes to some apparently unrelated PL/Python stuff > -- this patch includes only the PL/Tcl changes. I dare say the plpython needs to be checked by someone who knows how to since I can well imagine the same nested call fault will exist there. -- Nigel J. Andrews
Okay, I've looked again at spi_exec and I believe I can fix the bug I introduced and the memory leak. However, I have only looked quickly and not made these most recent changes to the execp version nor to the plpython code. Therefore I am not attaching a patch at the moment, just mentioning that I've straightened this out in my brain a bit more. On Wed, 25 Sep 2002, Nigel J. Andrews wrote: > On 25 Sep 2002, Neil Conway wrote: > > > "Nigel J. Andrews" <nandrews@investsystems.co.uk> writes: > > > Yes, I do get the similar results. > > > > > > A quick investigation shows that the SPI_freetuptable at the end of > > > pltcl_SPI_exec is trying to free a tuptable of value 0x82ebe64 > > > (which looks sensible to me) but which has a memory context of > > > 0x7f7f7f7f (the unallocated marker). > > > > Attached is a patch against CVS HEAD which fixes this, I believe. The > > problem appears to be the newly added free of the tuptable at the end > > of pltcl_SPI_exec(). I've added a comment to that effect: > > > > /* > > * Do *NOT* free the tuptable here. That's because if the loop > > * body executed any SQL statements, it will have already free'd > > * the tuptable itself, so freeing it twice is not wise. We could > > * get around this by making a copy of SPI_tuptable->vals and > > * feeding that to pltcl_set_tuple_values above, but that would > > * still leak memory (the palloc'ed copy would only be free'd on > > * context reset). > > */ > > That's certainly where the fault was happening. However, that's where the > original memory leak problem was coming from (without the SPI_freetuptable > call). It could be I got that fix wrong and the extra calls you've added are > the right fix for that. I'll take a look to see what I can learn later. > > > At least, I *think* that's the problem -- I've only been looking at > > the code for about 20 minutes, so I may be wrong. In any case, this > > makes both memleak() and memleak(1) work on my machine. Let me know if > > it works for you, and/or if someone knows of a better solution. > > I'll have to check later. > > > > > I also added some SPI_freetuptable() calls in some places where Nigel > > didn't, and added some paranoia when dealing with statically sized > > buffers (snprintf() rather than sprintf(), and so on). I also didn't > > include Nigel's changes to some apparently unrelated PL/Python stuff > > -- this patch includes only the PL/Tcl changes. > > I dare say the plpython needs to be checked by someone who knows how to since I > can well imagine the same nested call fault will exist there. > > > -- Nigel J. Andrews
Oh, so this is the later version. Fine. Let me know when it is ready. --------------------------------------------------------------------------- Nigel J. Andrews wrote: > > > Okay, I've looked again at spi_exec and I believe I can fix the bug I > introduced and the memory leak. However, I have only looked quickly and not > made these most recent changes to the execp version nor to the plpython > code. Therefore I am not attaching a patch at the moment, just mentioning that > I've straightened this out in my brain a bit more. > > > On Wed, 25 Sep 2002, Nigel J. Andrews wrote: > > > On 25 Sep 2002, Neil Conway wrote: > > > > > "Nigel J. Andrews" <nandrews@investsystems.co.uk> writes: > > > > Yes, I do get the similar results. > > > > > > > > A quick investigation shows that the SPI_freetuptable at the end of > > > > pltcl_SPI_exec is trying to free a tuptable of value 0x82ebe64 > > > > (which looks sensible to me) but which has a memory context of > > > > 0x7f7f7f7f (the unallocated marker). > > > > > > Attached is a patch against CVS HEAD which fixes this, I believe. The > > > problem appears to be the newly added free of the tuptable at the end > > > of pltcl_SPI_exec(). I've added a comment to that effect: > > > > > > /* > > > * Do *NOT* free the tuptable here. That's because if the loop > > > * body executed any SQL statements, it will have already free'd > > > * the tuptable itself, so freeing it twice is not wise. We could > > > * get around this by making a copy of SPI_tuptable->vals and > > > * feeding that to pltcl_set_tuple_values above, but that would > > > * still leak memory (the palloc'ed copy would only be free'd on > > > * context reset). > > > */ > > > > That's certainly where the fault was happening. However, that's where the > > original memory leak problem was coming from (without the SPI_freetuptable > > call). It could be I got that fix wrong and the extra calls you've added are > > the right fix for that. I'll take a look to see what I can learn later. > > > > > At least, I *think* that's the problem -- I've only been looking at > > > the code for about 20 minutes, so I may be wrong. In any case, this > > > makes both memleak() and memleak(1) work on my machine. Let me know if > > > it works for you, and/or if someone knows of a better solution. > > > > I'll have to check later. > > > > > > > > I also added some SPI_freetuptable() calls in some places where Nigel > > > didn't, and added some paranoia when dealing with statically sized > > > buffers (snprintf() rather than sprintf(), and so on). I also didn't > > > include Nigel's changes to some apparently unrelated PL/Python stuff > > > -- this patch includes only the PL/Tcl changes. > > > > I dare say the plpython needs to be checked by someone who knows how to since I > > can well imagine the same nested call fault will exist there. > > > > > > > > -- > Nigel J. Andrews > > > > ---------------------------(end of broadcast)--------------------------- > TIP 2: you can get off all lists at once with the unregister command > (send "unregister YourEmailAddressHere" to majordomo@postgresql.org) > -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001+ If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square, Pennsylvania19073