Re: [HACKERS] pltcl.so patch
On 25 Sep 2002, Neil Conway wrote: Nigel J. Andrews [EMAIL PROTECTED] 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 6: Have you searched our list archives? http://archives.postgresql.org
Re: [HACKERS] pltcl.so patch
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 [EMAIL PROTECTED] 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 [EMAIL PROTECTED])
Re: [HACKERS] pltcl.so patch
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 [EMAIL PROTECTED] 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 [EMAIL PROTECTED]) -- Bruce Momjian| http://candle.pha.pa.us [EMAIL PROTECTED] | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup.| Newtown Square, Pennsylvania 19073 ---(end of broadcast)--- TIP 6: Have you searched our list archives? http://archives.postgresql.org
Re: [HACKERS] 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 leak is 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, psql starts 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?
---(end of broadcast)---
TIP 2: you can get off all lists at once with the unregister command
(send unregister YourEmailAddressHere to [EMAIL PROTECTED])
Re: [HACKERS] pltcl.so patch
Nigel J. Andrews [EMAIL PROTECTED] 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 [EMAIL PROTECTED] || PGP Key ID: DB3C29FC
Index: src/pl/tcl/pltcl.c
===
RCS file: /var/lib/cvs/pgsql-server/src/pl/tcl/pltcl.c,v
retrieving revision 1.62
diff -c -r1.62 pltcl.c
*** src/pl/tcl/pltcl.c 21 Sep 2002 18:39:26 - 1.62
--- src/pl/tcl/pltcl.c 25 Sep 2002 04:54:57 -
***
*** 39,50
#include tcl.h
- #include stdio.h
- #include stdlib.h
- #include stdarg.h
#include unistd.h
#include fcntl.h
- #include string.h
#include setjmp.h
#include access/heapam.h
--- 39,46
***
*** 308,313
--- 304,310
/
spi_rc = SPI_exec(select 1 from pg_class
where relname = 'pltcl_modules', 1);
+ SPI_freetuptable(SPI_tuptable);
if (spi_rc != SPI_OK_SELECT)
elog(ERROR, pltcl_init_load_unknown(): select from pg_class failed);
if (SPI_processed == 0)
***
*** 334,339
--- 331,337
if (SPI_processed == 0)
{
Tcl_DStringFree(unknown_src);
+ SPI_freetuptable(SPI_tuptable);
elog(WARNING, pltcl: Module unknown not found in pltcl_modules);
return;
}
***
*** 359,364
--- 357,363
}
tcl_rc = Tcl_GlobalEval(interp, Tcl_DStringValue(unknown_src));
Tcl_DStringFree(unknown_src);
+ SPI_freetuptable(SPI_tuptable);
}
***
*** 955,963
* Build our internal proc name from the functions Oid
/
if (!is_trigger)
! sprintf(internal_proname, __PLTcl_proc_%u, fn_oid);
else
! sprintf(internal_proname, __PLTcl_proc_%u_trigger, fn_oid);
/
* Lookup the internal proc name in the hashtable
--- 954,964
* Build our internal proc name from the functions Oid
/
if (!is_trigger)
! snprintf(internal_proname, sizeof(internal_proname),
! __PLTcl_proc_%u, fn_oid);
else
! snprintf(internal_proname, sizeof(internal_proname),
! __PLTcl_proc_%u_trigger, fn_oid);
/
* Lookup the internal proc name in the hashtable
***
*** 1127,1133
prodesc-arg_is_rel[i] = 1;
if (i 0)
strcat(proc_internal_args, );
! sprintf(buf, __PLTcl_Tup_%d, i + 1);
strcat(proc_internal_args, buf);
ReleaseSysCache(typeTup);
continue;
--- 1128,1134
prodesc-arg_is_rel[i] = 1;
if (i 0)
strcat(proc_internal_args, );
! snprintf(buf, sizeof(buf), __PLTcl_Tup_%d, i + 1);
strcat(proc_internal_args, buf);
ReleaseSysCache(typeTup);
continue;
***
*** 1140,1146
if (i 0)
strcat(proc_internal_args, );
! sprintf(buf, %d, i + 1);
strcat(proc_internal_args, buf);
ReleaseSysCache(typeTup);
--- 1141,1147
if (i 0)
strcat(proc_internal_args, );
! snprintf(buf, sizeof(buf), %d, i + 1);
strcat(proc_internal_args, buf);
ReleaseSysCache(typeTup);
***
*** 1177,1183
{
if (!prodesc-arg_is_rel[i])
continue;
! sprintf(buf, array set %d $__PLTcl_Tup_%d\n, i + 1, i + 1);
