Thread: Leak repairs
Hi Anoop,
I've spend a while (too long really!) tracking down the memory leaks
we've been seeing in the libpq driver. The code below is enough to see
the problems I've observed using perfmon monitoring the process's
private bytes value.
The attached patch fixes 2 leaks, each of which noticably reduce the
angle of the perfmon trace, however it's not quite flat yet, and my eyes
have gone blurry from looking! It seems to be leaking about 1Kb/per
SQLExecute/SQLFreeStmt (it was 4Kb when I started). It also removes an
unnecessary call to PQgetisnull that I noticed.
I haven't applied any of this to CVS - I'll leave that to you if you're
happy with it.
#include <windows.h>
#include <sqlext.h>
#include <stdio.h>
int main(void)
{
HENV hEnv = NULL;
HDBC hDBC = NULL;
HSTMT hStmt = NULL;
UCHAR szDSN[SQL_MAX_DSN_LENGTH] = "psqlodbc-libpq";
UCHAR szUID[64] = "postgres";
UCHAR* szPasswd = NULL;
UCHAR szSqlStr[] = "SELECT 1";
RETCODE retcode;
long loop = 0;
SQLAllocEnv (&hEnv);
SQLAllocConnect (hEnv, &hDBC);
retcode = SQLConnect (hDBC, szDSN, SQL_NTS, szUID, SQL_NTS,
szPasswd, SQL_NTS);
if (retcode == SQL_SUCCESS || retcode == SQL_SUCCESS_WITH_INFO)
{
while(loop < 100000)
{
retcode = SQLAllocStmt (hDBC, &hStmt);
retcode = SQLPrepare (hStmt, szSqlStr, sizeof (szSqlStr));
retcode = SQLExecute (hStmt);
SQLFreeStmt (hStmt, SQL_DROP);
loop++;
}
SQLDisconnect (hDBC);
}
SQLFreeConnect (hDBC);
SQLFreeEnv (hEnv);
return 0;
}
Regards, Dave.
Attachment
Hi Dave,
I will certainly look into it today. Thanks for the exhaustive checking
of the code!
Regards
Anoop
-----Original Message-----
From: pgsql-odbc-owner@postgresql.org
[mailto:pgsql-odbc-owner@postgresql.org] On Behalf Of Dave Page
Sent: Friday, July 15, 2005 7:06 PM
To: pgsql-odbc@postgresql.org
Cc: Anoop Kumar
Subject: [ODBC] Leak repairs
Hi Anoop,
I've spend a while (too long really!) tracking down the memory leaks
we've been seeing in the libpq driver. The code below is enough to see
the problems I've observed using perfmon monitoring the process's
private bytes value.
The attached patch fixes 2 leaks, each of which noticably reduce the
angle of the perfmon trace, however it's not quite flat yet, and my eyes
have gone blurry from looking! It seems to be leaking about 1Kb/per
SQLExecute/SQLFreeStmt (it was 4Kb when I started). It also removes an
unnecessary call to PQgetisnull that I noticed.
I haven't applied any of this to CVS - I'll leave that to you if you're
happy with it.
#include <windows.h>
#include <sqlext.h>
#include <stdio.h>
int main(void)
{
HENV hEnv = NULL;
HDBC hDBC = NULL;
HSTMT hStmt = NULL;
UCHAR szDSN[SQL_MAX_DSN_LENGTH] = "psqlodbc-libpq";
UCHAR szUID[64] = "postgres";
UCHAR* szPasswd = NULL;
UCHAR szSqlStr[] = "SELECT 1";
RETCODE retcode;
long loop = 0;
SQLAllocEnv (&hEnv);
SQLAllocConnect (hEnv, &hDBC);
retcode = SQLConnect (hDBC, szDSN, SQL_NTS, szUID, SQL_NTS,
szPasswd, SQL_NTS);
if (retcode == SQL_SUCCESS || retcode == SQL_SUCCESS_WITH_INFO)
{
while(loop < 100000)
{
retcode = SQLAllocStmt (hDBC, &hStmt);
retcode = SQLPrepare (hStmt, szSqlStr, sizeof (szSqlStr));
retcode = SQLExecute (hStmt);
SQLFreeStmt (hStmt, SQL_DROP);
loop++;
}
SQLDisconnect (hDBC);
}
SQLFreeConnect (hDBC);
SQLFreeEnv (hEnv);
return 0;
}
Regards, Dave.
Hi Dave,
I have tested the patch from you, and it does make the perfmon graph
less steep. I am applying the patch to CVS.
Regarding the call to PGgetisnull: My mistake! I forgot to remove it,
thanks for pointing it out.
Regards
Anoop
-----Original Message-----
From: pgsql-odbc-owner@postgresql.org
[mailto:pgsql-odbc-owner@postgresql.org] On Behalf Of Dave Page
Sent: Friday, July 15, 2005 7:06 PM
To: pgsql-odbc@postgresql.org
Cc: Anoop Kumar
Subject: [ODBC] Leak repairs
Hi Anoop,
I've spend a while (too long really!) tracking down the memory leaks
we've been seeing in the libpq driver. The code below is enough to see
the problems I've observed using perfmon monitoring the process's
private bytes value.
The attached patch fixes 2 leaks, each of which noticably reduce the
angle of the perfmon trace, however it's not quite flat yet, and my eyes
have gone blurry from looking! It seems to be leaking about 1Kb/per
SQLExecute/SQLFreeStmt (it was 4Kb when I started). It also removes an
unnecessary call to PQgetisnull that I noticed.
I haven't applied any of this to CVS - I'll leave that to you if you're
happy with it.
#include <windows.h>
#include <sqlext.h>
#include <stdio.h>
int main(void)
{
HENV hEnv = NULL;
HDBC hDBC = NULL;
HSTMT hStmt = NULL;
UCHAR szDSN[SQL_MAX_DSN_LENGTH] = "psqlodbc-libpq";
UCHAR szUID[64] = "postgres";
UCHAR* szPasswd = NULL;
UCHAR szSqlStr[] = "SELECT 1";
RETCODE retcode;
long loop = 0;
SQLAllocEnv (&hEnv);
SQLAllocConnect (hEnv, &hDBC);
retcode = SQLConnect (hDBC, szDSN, SQL_NTS, szUID, SQL_NTS,
szPasswd, SQL_NTS);
if (retcode == SQL_SUCCESS || retcode == SQL_SUCCESS_WITH_INFO)
{
while(loop < 100000)
{
retcode = SQLAllocStmt (hDBC, &hStmt);
retcode = SQLPrepare (hStmt, szSqlStr, sizeof (szSqlStr));
retcode = SQLExecute (hStmt);
SQLFreeStmt (hStmt, SQL_DROP);
loop++;
}
SQLDisconnect (hDBC);
}
SQLFreeConnect (hDBC);
SQLFreeEnv (hEnv);
return 0;
}
Regards, Dave.
> -----Original Message----- > From: Anoop Kumar [mailto:anoopk@pervasive-postgres.com] > Sent: 19 July 2005 07:35 > To: Dave Page > Cc: pgsql-odbc@postgresql.org > Subject: RE: [ODBC] Leak repairs > > Hi Dave, > > I have tested the patch from you, and it does make the perfmon graph > less steep. I am applying the patch to CVS. Great, thanks. Any joy tracking down the last little bit? > Regarding the call to PGgetisnull: My mistake! I forgot to remove it, > thanks for pointing it out. No problem. I think we should put out a second snapshot sometime, as soon as the rest of this memory leak is fixed, and the Excel problem is sorted. Any progress on that one? Also we need to expose the libpq SSL option to the user, probably on the main connection dialogue, and obviously as a connection string option. At the same time, the protocol version options can be removed. I can probably look at that if you are pushed for time - let me know what you prefer. Finally, the threading problem. After discussion with Bruce Momjian and Magnus Hagander, we think that the --enable-thread-safety configure option should be used on Windows as well as other platforms as it does do more than just enable critical sections around non-thread safe API calls. I am (as time allows!) working on fixing PostgreSQL to allow this. Regards, Dave
Have you seen this one? 1121712844: 17186: not freed: '0x404ef808|s1' (800 bytes) from 'qresult.c:421' 1121712844: 17186: not freed: '0x404f0c08|s1' (800 bytes) from 'qresult.c:421' 1121712844: 17186: not freed: '0x404fd008|s1' (3200 bytes) from 'qresult.c:421' 1121712844: 17186: not freed: '0x404fe008|s1' (3200 bytes) from 'qresult.c:421' 1121712844: 17186: not freed: '0x404ff008|s1' (3200 bytes) from 'qresult.c:421' 1121712844: 17186: not freed: '0x40500008|s1' (3200 bytes) from 'qresult.c:421' 1121712844: 17186: not freed: '0x40501008|s1' (3200 bytes) from 'qresult.c:421' Line 421 allocates a buffer into QResultClass.backend_tuples. backend_tuples will be set into NULL in QR_Destructor() with "self->backend_tuples = NULL". I suspect, that the memory leak. Marko Ristola Dave Page wrote: > > > > >>-----Original Message----- >>From: Anoop Kumar [mailto:anoopk@pervasive-postgres.com] >>Sent: 19 July 2005 07:35 >>To: Dave Page >>Cc: pgsql-odbc@postgresql.org >>Subject: RE: [ODBC] Leak repairs >> >>Hi Dave, >> >>I have tested the patch from you, and it does make the perfmon graph >>less steep. I am applying the patch to CVS. >> >> > > >Great, thanks. Any joy tracking down the last little bit? > > > >>Regarding the call to PGgetisnull: My mistake! I forgot to remove it, >>thanks for pointing it out. >> >> > >No problem. > >I think we should put out a second snapshot sometime, as soon as the >rest of this memory leak is fixed, and the Excel problem is sorted. Any >progress on that one? > >Also we need to expose the libpq SSL option to the user, probably on the >main connection dialogue, and obviously as a connection string option. >At the same time, the protocol version options can be removed. I can >probably look at that if you are pushed for time - let me know what you >prefer. > >Finally, the threading problem. After discussion with Bruce Momjian and >Magnus Hagander, we think that the --enable-thread-safety configure >option should be used on Windows as well as other platforms as it does >do more than just enable critical sections around non-thread safe API >calls. I am (as time allows!) working on fixing PostgreSQL to allow >this. > >Regards, Dave > >---------------------------(end of broadcast)--------------------------- >TIP 1: if posting/reading through Usenet, please send an appropriate > subscribe-nomail command to majordomo@postgresql.org so that your > message can get through to the mailing list cleanly > >
> -----Original Message----- > From: pgsql-odbc-owner@postgresql.org > [mailto:pgsql-odbc-owner@postgresql.org] On Behalf Of Marko Ristola > Sent: 19 July 2005 08:37 > Cc: Anoop Kumar; pgsql-odbc@postgresql.org > Subject: Re: [ODBC] Leak repairs > > > Have you seen this one? > > 1121712844: 17186: not freed: '0x404ef808|s1' (800 bytes) from > 'qresult.c:421' > 1121712844: 17186: not freed: '0x404f0c08|s1' (800 bytes) from > 'qresult.c:421' > 1121712844: 17186: not freed: '0x404fd008|s1' (3200 bytes) from > 'qresult.c:421' > 1121712844: 17186: not freed: '0x404fe008|s1' (3200 bytes) from > 'qresult.c:421' > 1121712844: 17186: not freed: '0x404ff008|s1' (3200 bytes) from > 'qresult.c:421' > 1121712844: 17186: not freed: '0x40500008|s1' (3200 bytes) from > 'qresult.c:421' > 1121712844: 17186: not freed: '0x40501008|s1' (3200 bytes) from > 'qresult.c:421' > > Line 421 allocates a buffer into QResultClass.backend_tuples. > > backend_tuples will be set into NULL in QR_Destructor() with > "self->backend_tuples = NULL". I suspect, that the memory leak. > Nicely spotted :-) That would appear to be the last of them (at least for my simple testing at the moment). I'll commit a patch shortly. BTW, I assume you got that output from the VC++ debugger - care to share how? I spent quite some time trying to persuade it to give me that and didn't get anywhere :-( Regards, Dave.
I experimented almost the hole monday with dmalloc library.
So that was not efficient. I wanted to see, wether there
is a tool, that can debug the psqlodbc library behind
UnixODBC under Linux.
I don't have any Windows development environment.
I could though install Mingw into Windows XP.
So, the dmalloc library is under Debian Linux as a package
and easy to install though.
Here is, how I did it:
test.c:
#include <dmalloc.h>
test.c linking: gcc needed option -ldmallocth (threaded version).
I modified psqlodbclibpq in the following way:
PGAPI_AllocEnv() function in environ.c:
dmalloc_debug_setup("debug=0x4f4ed03,log=liblogfile");
This activates the memory debugging. liblogfile is the file, where the
results go.
PGAPI_FreeEnv() in environ.c:
dmalloc_shutdown(); // before return of return SQL_SUCCESS
Actually this setup crashes within dlclose inside UnixODBC library.
But, the debugging information will get written into "liblogfile",
because the log will be written inside PGAPI_FreeEnv().
I did compile test.c and psqlodbclibpq with -g and without optimizations
to achieve a good debugging environment.
This was the code block within an active connect:
for (i=0; i<5; i++)
{
if
(!CHECK(SQL_HANDLE_DBC,m_conn,SQLAllocHandle(SQL_HANDLE_STMT,m_conn,&m_stmt)))
return 1;
if (!CHECK(SQL_HANDLE_STMT,m_stmt,SQLExecDirect(m_stmt,(SQLCHAR*)
"SELECT * FROM TEST1 LIMIT 100", SQL_NTS)))
return 1;
if (!CHECK(SQL_HANDLE_DBC,m_conn,SQLFreeStmt(m_stmt,SQL_DROP)))
return 1;
}
Marko Ristola
Dave Page wrote:
>
>
>
>
>>-----Original Message-----
>>From: pgsql-odbc-owner@postgresql.org
>>[mailto:pgsql-odbc-owner@postgresql.org] On Behalf Of Marko Ristola
>>Sent: 19 July 2005 08:37
>>Cc: Anoop Kumar; pgsql-odbc@postgresql.org
>>Subject: Re: [ODBC] Leak repairs
>>
>>
>>Have you seen this one?
>>
>>1121712844: 17186: not freed: '0x404ef808|s1' (800 bytes) from
>>'qresult.c:421'
>>1121712844: 17186: not freed: '0x404f0c08|s1' (800 bytes) from
>>'qresult.c:421'
>>1121712844: 17186: not freed: '0x404fd008|s1' (3200 bytes) from
>>'qresult.c:421'
>>1121712844: 17186: not freed: '0x404fe008|s1' (3200 bytes) from
>>'qresult.c:421'
>>1121712844: 17186: not freed: '0x404ff008|s1' (3200 bytes) from
>>'qresult.c:421'
>>1121712844: 17186: not freed: '0x40500008|s1' (3200 bytes) from
>>'qresult.c:421'
>>1121712844: 17186: not freed: '0x40501008|s1' (3200 bytes) from
>>'qresult.c:421'
>>
>>Line 421 allocates a buffer into QResultClass.backend_tuples.
>>
>>backend_tuples will be set into NULL in QR_Destructor() with
>>"self->backend_tuples = NULL". I suspect, that the memory leak.
>>
>>
>>
>
>Nicely spotted :-)
>
>That would appear to be the last of them (at least for my simple testing
>at the moment). I'll commit a patch shortly.
>
>BTW, I assume you got that output from the VC++ debugger - care to share
>how? I spent quite some time trying to persuade it to give me that and
>didn't get anywhere :-(
>
>Regards, Dave.
>
>---------------------------(end of broadcast)---------------------------
>TIP 9: In versions below 8.0, the planner will ignore your desire to
> choose an index scan if your joining column's datatypes do not
> match
>
>
> -----Original Message-----
> From: pgsql-odbc-owner@postgresql.org
> [mailto:pgsql-odbc-owner@postgresql.org] On Behalf Of Dave Page
> Sent: 19 July 2005 10:50
> To: Marko Ristola
> Cc: Anoop Kumar; pgsql-odbc@postgresql.org
> Subject: Re: [ODBC] Leak repairs
>
>
> That would appear to be the last of them (at least for my
> simple testing
> at the moment). I'll commit a patch shortly.
The patch below has been committed. Oddly though, I was demonstrating
the memory leak to someone here and removed the fix to show what
shouldn't happen - the perfmon trace remained flat! Unless my previous
testing was flawed in someway, it now appears that I cannot reproduce
the remaining leak I saw. Oh well...
> BTW, I assume you got that output from the VC++ debugger -
> care to share
> how? I spent quite some time trying to persuade it to give me that and
> didn't get anywhere :-(
I saw your other email on that - thanks for the info.
Regards, Dave.
Index: qresult.c
===================================================================
RCS file: /usr/local/cvsroot/psqlodbc/psqlodbc/qresult.c,v
retrieving revision 1.50
diff -u -r1.50 qresult.c
--- qresult.c 15 Jul 2005 09:53:34 -0000 1.50
+++ qresult.c 19 Jul 2005 10:00:22 -0000
@@ -156,13 +156,17 @@
TL_Destructor(self->manual_tuples);
self->manual_tuples = NULL;
}
-
+
/*
* If conn is defined, then we may have used "backend_tuples",
so in
* case we need to, free it up. Also, close the cursor.
*/
#ifdef USE_LIBPQ
- self->backend_tuples = NULL;
+ if (self->backend_tuples)
+ {
+ free(self->backend_tuples);
+ self->backend_tuples = NULL;
+ }
if (conn && conn->pgconn && CC_is_in_trans(conn))
#else
if (conn && conn->sock && CC_is_in_trans(conn))
Hi Dave,
I saw the patch just now and tested it. For me it works well and I am
getting a flat trace graph.
Regards
Anoop
-----Original Message-----
From: Dave Page [mailto:dpage@vale-housing.co.uk]
Sent: Tuesday, July 19, 2005 3:53 PM
To: Marko Ristola
Cc: Anoop Kumar; pgsql-odbc@postgresql.org
Subject: RE: [ODBC] Leak repairs
> -----Original Message-----
> From: pgsql-odbc-owner@postgresql.org
> [mailto:pgsql-odbc-owner@postgresql.org] On Behalf Of Dave Page
> Sent: 19 July 2005 10:50
> To: Marko Ristola
> Cc: Anoop Kumar; pgsql-odbc@postgresql.org
> Subject: Re: [ODBC] Leak repairs
>
>
> That would appear to be the last of them (at least for my
> simple testing
> at the moment). I'll commit a patch shortly.
The patch below has been committed. Oddly though, I was demonstrating
the memory leak to someone here and removed the fix to show what
shouldn't happen - the perfmon trace remained flat! Unless my previous
testing was flawed in someway, it now appears that I cannot reproduce
the remaining leak I saw. Oh well...
> BTW, I assume you got that output from the VC++ debugger -
> care to share
> how? I spent quite some time trying to persuade it to give me that and
> didn't get anywhere :-(
I saw your other email on that - thanks for the info.
Regards, Dave.
Index: qresult.c
===================================================================
RCS file: /usr/local/cvsroot/psqlodbc/psqlodbc/qresult.c,v
retrieving revision 1.50
diff -u -r1.50 qresult.c
--- qresult.c 15 Jul 2005 09:53:34 -0000 1.50
+++ qresult.c 19 Jul 2005 10:00:22 -0000
@@ -156,13 +156,17 @@
TL_Destructor(self->manual_tuples);
self->manual_tuples = NULL;
}
-
+
/*
* If conn is defined, then we may have used "backend_tuples",
so in
* case we need to, free it up. Also, close the cursor.
*/
#ifdef USE_LIBPQ
- self->backend_tuples = NULL;
+ if (self->backend_tuples)
+ {
+ free(self->backend_tuples);
+ self->backend_tuples = NULL;
+ }
if (conn && conn->pgconn && CC_is_in_trans(conn))
#else
if (conn && conn->sock && CC_is_in_trans(conn))