Обсуждение: possible memory leak in Server Status window
Hello, all! Last Friday I've installed pgAdmin 1.12.2 (build Dec 13, 2010) into one of our clients servers (Windows server 2003 SE SP2). Postgresql 8.3.3 runs on it. As we were looking how to improve performance we were using Server Status tool (great tool, especially with recent changes - thank you for it). I have noticed that pgAdmin consumes virtual memory when the tool is running. Today I have opened the Server Status tool, and during an hour or so the VM size used by pgAdmin increased from 13Mb to 28Mb. I tried to reproduce the behavior on other systems but with no success yet. Could you give me a hint what I should try? Also I noticed, that Lock window is updated not according to interval chosen in combo box. When I select 10 minutes interval lock list is updated every minute (or so). Is this behavior expected? -- Julius Tuskenis Programavimo skyriaus vadovas UAB nSoft mob. +37068233050
Le 07/02/2011 15:30, Julius Tuskenis a écrit : > Hello, all! > > Last Friday I've installed pgAdmin 1.12.2 (build Dec 13, 2010) into one > of our clients servers (Windows server 2003 SE SP2). Postgresql 8.3.3 > runs on it. As we were looking how to improve performance we were using > Server Status tool (great tool, especially with recent changes - thank > you for it). I have noticed that pgAdmin consumes virtual memory when > the tool is running. > Today I have opened the Server Status tool, and during an hour or so the > VM size used by pgAdmin increased from 13Mb to 28Mb. > I tried to reproduce the behavior on other systems but with no success > yet. Could you give me a hint what I should try? > > Also I noticed, that Lock window is updated not according to interval > chosen in combo box. When I select 10 minutes interval lock list is > updated every minute (or so). Is this behavior expected? > Nope, I'll check this. Make sure you specifically set the interval for this report. -- Guillaumehttp://www.postgresql.frhttp://dalibo.com
2011.02.07 17:05, Guillaume Lelarge rašė: > Nope, I'll check this. Make sure you specifically set the interval for > this report. Thank you, Guillaume, for the hint. I did't realize there are different intervals for every report in server status window. Now I was able to isolate the window responsible for the memory leak - it's the logfile window. Also now I was able to reproduce it on my own computer - I set "don't refresh" on all the other reports and "1 second" on the log file. In Task manager I see VM constantly increasing. I hope that info will be useful for you. Thank you for the patience. -- Julius Tuskenis Programavimo skyriaus vadovas UAB nSoft mob. +37068233050
Le 08/02/2011 08:54, Julius Tuskenis a écrit : > 2011.02.07 17:05, Guillaume Lelarge rašė: >> Nope, I'll check this. Make sure you specifically set the interval for >> this report. > Thank you, Guillaume, for the hint. I did't realize there are different > intervals for every report in server status window. Now I was able to > isolate the window responsible for the memory leak - it's the logfile > window. Also now I was able to reproduce it on my own computer - I set > "don't refresh" on all the other reports and "1 second" on the log file. > In Task manager I see VM constantly increasing. I hope that info will be > useful for you. > > Thank you for the patience. > I looked a bit in the source code, but can't find any memory leak in it. The code is pretty much the same for each report. The only specific thing for the lock report is that it can use a specific connection. But this doesn't trigger anything useful to fix this. I'm a bit puzzled. -- Guillaumehttp://www.postgresql.frhttp://dalibo.com
2011.02.10 23:55, Guillaume Lelarge rašė: > I looked a bit in the source code, but can't find any memory leak in it. > The code is pretty much the same for each report. The only specific > thing for the lock report is that it can use a specific connection. But > this doesn't trigger anything useful to fix this. > > I'm a bit puzzled. Were you able to reproduce it? Do you see VM increasing? -- Julius Tuskenis Programavimo skyriaus vadovas UAB nSoft mob. +37068233050
Hello, Guillaume
Please look at frmStatus::fillLogfileCombo(). Please note, that the set
is defined and assigned, but then as it has not enough rows to be
"interesting" the function returns 0 and exits. So "delete set;" is
never called. Can this be the issue?
int frmStatus::fillLogfileCombo()
{ int count = cbLogfiles->GetCount(); if (!count) cbLogfiles->Append(_("Current log")); else
count--;
pgSet *set = connection->ExecuteSet( wxT("SELECT filename, filetime\n")
wxT(" FROM pg_logdir_ls() AS A(filetime
timestamp, filename text)\n") wxT(" ORDER BY filetime DESC")); if (set) { if
(set->NumRows()<= count) return 0;
--
Julius Tuskenis
Programavimo skyriaus vadovas
UAB nSoft
mob. +37068233050
On 11 February 2011 10:42, Julius Tuskenis <julius@nsoft.lt> wrote: > Hello, Guillaume > > Please look at frmStatus::fillLogfileCombo(). Please note, that the set is > defined and assigned, but then as it has not enough rows to be "interesting" > the function returns 0 and exits. So "delete set;" is never called. Can this > be the issue? Yep, that's a memory leak. I really think that we should be wrapping pgSet results in a smart pointer. wxWidgets 2.9 has a templated smart pointer class. It might make sense to look at std::unique_ptr instead, but that might be a problem on some more exotic platforms, and we'd have to make autotools detect if it was available and possibly error if it wasn't. Alternatively we could abandon pointer semantics and write our own RAII wrapper class. This would be a large patch. -- Regards, Peter Geoghegan
On Fri, Feb 11, 2011 at 2:45 PM, Peter Geoghegan <peter.geoghegan86@gmail.com> wrote: > On 11 February 2011 10:42, Julius Tuskenis <julius@nsoft.lt> wrote: >> Hello, Guillaume >> >> Please look at frmStatus::fillLogfileCombo(). Please note, that the set is >> defined and assigned, but then as it has not enough rows to be "interesting" >> the function returns 0 and exits. So "delete set;" is never called. Can this >> be the issue? > > Yep, that's a memory leak. > > I really think that we should be wrapping pgSet results in a smart > pointer. wxWidgets 2.9 has a templated smart pointer class. It might > make sense to look at std::unique_ptr instead, but that might be a > problem on some more exotic platforms, and we'd have to make autotools > detect if it was available and possibly error if it wasn't. Ashesh had some code to implement a smart pointer a while back. I suggested he post it here, but I know he's been too busy to do much else with it. > Alternatively we could abandon pointer semantics and write our own > RAII wrapper class. This would be a large patch. Exactly. -- Dave Page Blog: http://pgsnake.blogspot.com Twitter: @pgsnake EnterpriseDB UK: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 11 February 2011 15:04, Dave Page <dpage@pgadmin.org> wrote: > Ashesh had some code to implement a smart pointer a while back. I > suggested he post it here, but I know he's been too busy to do much > else with it.container I would prefer to use a stdlib smart ptr, or a well tested third party smart ptr that we can adopt ourselves. std::auto_ptr has been in the standard for a long time, so it really isn't that unreasonable to expect it to be on supported platforms. It is totally no-throwing. On the other hand, it will soon be deprecated. There are lots of subtleties to writing a smart pointer class. I seem to recall Scott Meyers complaining about the amount of errata he had to write when he rolled his own smart pointer class for either the first or second edition of Effective C++. Granted, that probably had plenty to do with exception safety and exception neutrality, which we basically don't have to worry about, but I think some of the same concerns still apply. >> Alternatively we could abandon pointer semantics and write our own >> RAII wrapper class. This would be a large patch. > > Exactly. It also has the disadvantage of there being no reasonable way to avoid a deep copy when returning from a function without using C++0x's rvalue references. -- Regards, Peter Geoghegan
Le 11/02/2011 11:42, Julius Tuskenis a écrit : > Hello, Guillaume > > Please look at frmStatus::fillLogfileCombo(). Please note, that the set > is defined and assigned, but then as it has not enough rows to be > "interesting" the function returns 0 and exits. So "delete set;" is > never called. Can this be the issue? > Yes, it's definitely a memory leak. As I don't have the adminpack installed, I had no way to find this (other than reading the complete frmStatus code). Anyway, I pushed a fix for this. I know Peter and Dave talked about some stuff I actually didn't understand, but it seems it would take some time to do. So, I prefer having this simple and effective fix right now :) Thanks, Julius, for reporting the leak and caring enough to actually read the code :) -- Guillaumehttp://www.postgresql.frhttp://dalibo.com
2011.02.12 10:23, Guillaume Lelarge rašė: > I know Peter and Dave talked about some stuff I actually didn't > understand, but it seems it would take some time to do. So, I prefer > having this simple and effective fix right now:) Thank you. Is there a nightly build for windows? > Thanks, Julius, for reporting the leak and caring enough to actually > read the code:) No problem. It is nice to see pgAdmin uses Git for source control. I was happy I didn't have to install svn. Also it was a pleasure reading pgAdmin source - it can be an example of well written source: well named, formated and commented where needed. Great job! -- Julius Tuskenis Programavimo skyriaus vadovas UAB nSoft mob. +37068233050
On Sat, Feb 12, 2011 at 9:23 AM, Guillaume Lelarge <guillaume@lelarge.info> wrote: > I know Peter and Dave talked about some stuff I actually didn't > understand, but it seems it would take some time to do. So, I prefer > having this simple and effective fix right now :) :-) We were talking about smart pointers - basically, instead of doing something like: obj *foo = new obj(); ... <many lines of code and exit points> ... delete foo; You might do: pg_smartptr<obj> foo; ... <many lines of code and exit points> ... Note the lack of a delete - that's the point here; it's not needed because as soon as the smart pointer goes out of scope, it's destructor will delete foo. That means you don't have to remember to include the delete in each of the many exit points of the code. See also http://en.wikipedia.org/wiki/Resource_Acquisition_Is_Initialization -- Dave Page Blog: http://pgsnake.blogspot.com Twitter: @pgsnake EnterpriseDB UK: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 12 February 2011 14:22, Dave Page <dpage@pgadmin.org> wrote: > Note the lack of a delete - that's the point here; it's not needed > because as soon as the smart pointer goes out of scope, it's > destructor will delete foo. That means you don't have to remember to > include the delete in each of the many exit points of the code. > > See also http://en.wikipedia.org/wiki/Resource_Acquisition_Is_Initialization "Resource Acquisition is Initialization" is a terrible name. Some people call it "scope bound resource management", which makes a lot more sense, but isn't as widely known. -- Regards, Peter Geoghegan
On Saturday, February 12, 2011, Peter Geoghegan <peter.geoghegan86@gmail.com> wrote: > On 12 February 2011 14:22, Dave Page <dpage@pgadmin.org> wrote: >> Note the lack of a delete - that's the point here; it's not needed >> because as soon as the smart pointer goes out of scope, it's >> destructor will delete foo. That means you don't have to remember to >> include the delete in each of the many exit points of the code. >> >> See also http://en.wikipedia.org/wiki/Resource_Acquisition_Is_Initialization > > "Resource Acquisition is Initialization" is a terrible name. Couldn't agree more. -- Dave Page Blog: http://pgsnake.blogspot.com Twitter: @pgsnake EnterpriseDB UK: http://www.enterprisedb.com The Enterprise PostgreSQL Company
By the way, there is a macro based smart pointer class in wx 2.8, but that sounds like quite a bit more trouble than it's worth. -- Regards, Peter Geoghegan
Le 12/02/2011 15:22, Dave Page a écrit : > On Sat, Feb 12, 2011 at 9:23 AM, Guillaume Lelarge > <guillaume@lelarge.info> wrote: >> I know Peter and Dave talked about some stuff I actually didn't >> understand, but it seems it would take some time to do. So, I prefer >> having this simple and effective fix right now :) > > :-) > > We were talking about smart pointers - basically, instead of doing > something like: > > obj *foo = new obj(); > ... > <many lines of code and exit points> > ... > delete foo; > > You might do: > > pg_smartptr<obj> foo; > ... > <many lines of code and exit points> > ... > > Note the lack of a delete - that's the point here; it's not needed > because as soon as the smart pointer goes out of scope, it's > destructor will delete foo. That means you don't have to remember to > include the delete in each of the many exit points of the code. > Yeah, that would be really interesting. And thanks for the explanation :) > See also http://en.wikipedia.org/wiki/Resource_Acquisition_Is_Initialization > -- Guillaumehttp://www.postgresql.frhttp://dalibo.com