Re: [HACKERS] [PATCHES] PATCH: Memory leaks on start-up

2003-07-27 Thread Bruce Momjian 

miscinit.c part of patch applied.  Thanks.

The other part Tom already addressed.

---



Lee Kindness wrote:
Content-Description: message body text

> Tom, happier with the attached patch?
> 
> I'd have to disagree with regards to the memory leaks not being worth
> a mention - any such leak can cause problems when the PostgreSQL
> installation is either unattended, long-living andor has very high
> connection levels. Half a kilobyte on start-up isn't negligible in
> this light.
> 
> Regards, Lee.
> 
> Tom Lane writes:
>  > Lee Kindness <[EMAIL PROTECTED]> writes:
>  > > Guys, attached is a patch to fix two memory leaks on start-up.
>  > 
>  > I do not like the changes to miscinit.c.  In the first place, it is not
>  > a "memory leak" to do a one-time allocation of state for a proc_exit
>  > function.  A bigger complaint is that your proposed change introduces
>  > fragile coupling between CreateLockFile and its callers, in order to
>  > save no resources worth mentioning.  More, it introduces an assumption
>  > that the globals directoryLockFile and socketLockFile don't change while
>  > the postmaster is running.  UnlinkLockFile should unlink the file that
>  > it was originally told to unlink, regardless of what happens to those
>  > globals.
>  > 
>  > If you are intent on spending code to free stuff just before the
>  > postmaster exits, a better fix would be for UnlinkLockFile to free its
>  > string argument after using it.
> 

> Index: src/backend/libpq/pqcomm.c
> ===
> RCS file: /projects/cvsroot/pgsql-server/src/backend/libpq/pqcomm.c,v
> retrieving revision 1.157
> diff -u -r1.157 pqcomm.c
> --- src/backend/libpq/pqcomm.c12 Jun 2003 07:36:51 -  1.157
> +++ src/backend/libpq/pqcomm.c22 Jul 2003 14:16:46 -
> @@ -363,7 +363,7 @@
>   added++;
>   }
>  
> - freeaddrinfo(addrs);
> + freeaddrinfo2(family, addrs);
>  
>   if (!added)
>   {
> Index: src/backend/utils/init/miscinit.c
> ===
> RCS file: /projects/cvsroot/pgsql-server/src/backend/utils/init/miscinit.c,v
> retrieving revision 1.104
> diff -u -r1.104 miscinit.c
> --- src/backend/utils/init/miscinit.c 27 Jun 2003 19:08:37 -  1.104
> +++ src/backend/utils/init/miscinit.c 22 Jul 2003 14:16:46 -
> @@ -673,8 +673,15 @@
>  static void
>  UnlinkLockFile(int status, Datum filename)
>  {
> - unlink((char *) DatumGetPointer(filename));
> - /* Should we complain if the unlink fails? */
> +  char *fname = (char *)DatumGetPointer(filename);
> +  if( fname != NULL )
> +{
> +  if( unlink(fname) != 0 )
> + {
> +   /* Should we complain if the unlink fails? */
> + }
> +  free(fname);
> +}
>  }
>  
>  /*

> 
> ---(end of broadcast)---
> TIP 1: subscribe and unsubscribe commands go 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 9: the planner will ignore your desire to choose an index scan if your
  joining column's datatypes do not match

Re: [HACKERS] [PATCHES] PATCH: Memory leaks on start-up

2003-07-27 Thread Tom Lane 
Bruce Momjian <[EMAIL PROTECTED]> writes:
> This patch no longer applies cleanly.  The call is now:
>   freeaddrinfo_all(hint.ai_family, addrs);
> Would you please submit a new patch, or is it no longer required?

That part of the patch is not needed (it is the same as what I changed,
except that while I was at it I renamed freeaddrinfo2 to the hopefully
more mnemonic freeaddrinfo_all).  The change in miscinit should still
work though.

regards, tom lane

---(end of broadcast)---
TIP 1: subscribe and unsubscribe commands go to [EMAIL PROTECTED]

Re: [HACKERS] [PATCHES] PATCH: Memory leaks on start-up

2003-07-26 Thread Bruce Momjian 

This patch no longer applies cleanly.  The call is now:

freeaddrinfo_all(hint.ai_family, addrs);

Would you please submit a new patch, or is it no longer required?  There
were two fixed in your patch.

Thanks.

---

Lee Kindness wrote:
Content-Description: message body text

> Tom, happier with the attached patch?
> 
> I'd have to disagree with regards to the memory leaks not being worth
> a mention - any such leak can cause problems when the PostgreSQL
> installation is either unattended, long-living andor has very high
> connection levels. Half a kilobyte on start-up isn't negligible in
> this light.
> 
> Regards, Lee.
> 
> Tom Lane writes:
>  > Lee Kindness <[EMAIL PROTECTED]> writes:
>  > > Guys, attached is a patch to fix two memory leaks on start-up.
>  > 
>  > I do not like the changes to miscinit.c.  In the first place, it is not
>  > a "memory leak" to do a one-time allocation of state for a proc_exit
>  > function.  A bigger complaint is that your proposed change introduces
>  > fragile coupling between CreateLockFile and its callers, in order to
>  > save no resources worth mentioning.  More, it introduces an assumption
>  > that the globals directoryLockFile and socketLockFile don't change while
>  > the postmaster is running.  UnlinkLockFile should unlink the file that
>  > it was originally told to unlink, regardless of what happens to those
>  > globals.
>  > 
>  > If you are intent on spending code to free stuff just before the
>  > postmaster exits, a better fix would be for UnlinkLockFile to free its
>  > string argument after using it.
> 

> Index: src/backend/libpq/pqcomm.c
> ===
> RCS file: /projects/cvsroot/pgsql-server/src/backend/libpq/pqcomm.c,v
> retrieving revision 1.157
> diff -u -r1.157 pqcomm.c
> --- src/backend/libpq/pqcomm.c12 Jun 2003 07:36:51 -  1.157
> +++ src/backend/libpq/pqcomm.c22 Jul 2003 14:16:46 -
> @@ -363,7 +363,7 @@
>   added++;
>   }
>  
> - freeaddrinfo(addrs);
> + freeaddrinfo2(family, addrs);
>  
>   if (!added)
>   {
> Index: src/backend/utils/init/miscinit.c
> ===
> RCS file: /projects/cvsroot/pgsql-server/src/backend/utils/init/miscinit.c,v
> retrieving revision 1.104
> diff -u -r1.104 miscinit.c
> --- src/backend/utils/init/miscinit.c 27 Jun 2003 19:08:37 -  1.104
> +++ src/backend/utils/init/miscinit.c 22 Jul 2003 14:16:46 -
> @@ -673,8 +673,15 @@
>  static void
>  UnlinkLockFile(int status, Datum filename)
>  {
> - unlink((char *) DatumGetPointer(filename));
> - /* Should we complain if the unlink fails? */
> +  char *fname = (char *)DatumGetPointer(filename);
> +  if( fname != NULL )
> +{
> +  if( unlink(fname) != 0 )
> + {
> +   /* Should we complain if the unlink fails? */
> + }
> +  free(fname);
> +}
>  }
>  
>  /*

> 
> ---(end of broadcast)---
> TIP 1: subscribe and unsubscribe commands go 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 5: Have you checked our extensive FAQ?

   http://www.postgresql.org/docs/faqs/FAQ.html

Re: [HACKERS] [PATCHES] PATCH: Memory leaks on start-up

2003-07-23 Thread Bruce Momjian 

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

http://momjian.postgresql.org/cgi-bin/pgpatches

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

---


Lee Kindness wrote:
Content-Description: message body text

> Tom, happier with the attached patch?
> 
> I'd have to disagree with regards to the memory leaks not being worth
> a mention - any such leak can cause problems when the PostgreSQL
> installation is either unattended, long-living andor has very high
> connection levels. Half a kilobyte on start-up isn't negligible in
> this light.
> 
> Regards, Lee.
> 
> Tom Lane writes:
>  > Lee Kindness <[EMAIL PROTECTED]> writes:
>  > > Guys, attached is a patch to fix two memory leaks on start-up.
>  > 
>  > I do not like the changes to miscinit.c.  In the first place, it is not
>  > a "memory leak" to do a one-time allocation of state for a proc_exit
>  > function.  A bigger complaint is that your proposed change introduces
>  > fragile coupling between CreateLockFile and its callers, in order to
>  > save no resources worth mentioning.  More, it introduces an assumption
>  > that the globals directoryLockFile and socketLockFile don't change while
>  > the postmaster is running.  UnlinkLockFile should unlink the file that
>  > it was originally told to unlink, regardless of what happens to those
>  > globals.
>  > 
>  > If you are intent on spending code to free stuff just before the
>  > postmaster exits, a better fix would be for UnlinkLockFile to free its
>  > string argument after using it.
> 

> Index: src/backend/libpq/pqcomm.c
> ===
> RCS file: /projects/cvsroot/pgsql-server/src/backend/libpq/pqcomm.c,v
> retrieving revision 1.157
> diff -u -r1.157 pqcomm.c
> --- src/backend/libpq/pqcomm.c12 Jun 2003 07:36:51 -  1.157
> +++ src/backend/libpq/pqcomm.c22 Jul 2003 14:16:46 -
> @@ -363,7 +363,7 @@
>   added++;
>   }
>  
> - freeaddrinfo(addrs);
> + freeaddrinfo2(family, addrs);
>  
>   if (!added)
>   {
> Index: src/backend/utils/init/miscinit.c
> ===
> RCS file: /projects/cvsroot/pgsql-server/src/backend/utils/init/miscinit.c,v
> retrieving revision 1.104
> diff -u -r1.104 miscinit.c
> --- src/backend/utils/init/miscinit.c 27 Jun 2003 19:08:37 -  1.104
> +++ src/backend/utils/init/miscinit.c 22 Jul 2003 14:16:46 -
> @@ -673,8 +673,15 @@
>  static void
>  UnlinkLockFile(int status, Datum filename)
>  {
> - unlink((char *) DatumGetPointer(filename));
> - /* Should we complain if the unlink fails? */
> +  char *fname = (char *)DatumGetPointer(filename);
> +  if( fname != NULL )
> +{
> +  if( unlink(fname) != 0 )
> + {
> +   /* Should we complain if the unlink fails? */
> + }
> +  free(fname);
> +}
>  }
>  
>  /*

> 
> ---(end of broadcast)---
> TIP 1: subscribe and unsubscribe commands go 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 9: the planner will ignore your desire to choose an index scan if your
  joining column's datatypes do not match

Re: [PATCHES] PATCH: Memory leaks on start-up

2003-07-22 Thread Tom Lane 
Lee Kindness <[EMAIL PROTECTED]> writes:
> I'd have to disagree with regards to the memory leaks not being worth
> a mention - any such leak can cause problems when the PostgreSQL
> installation is either unattended, long-living andor has very high
> connection levels.

I don't see how.  We are talking about two strings, no more, no less,
that live for exactly the duration of the postmaster run.  Explain to me
how any of your above conditions will affect this code in the slightest?

If UnlinkLockFile ever got invoked before postmaster exit, then this
would be worth doing, and I'll accept the change as a matter of
future-proofing that routine against such use.  But on the argument of
preventing resource leakage today, this is just a waste of code space.

regards, tom lane

---(end of broadcast)---
TIP 4: Don't 'kill -9' the postmaster

Re: [PATCHES] PATCH: Memory leaks on start-up

2003-07-22 Thread Lee Kindness 
Tom, happier with the attached patch?

I'd have to disagree with regards to the memory leaks not being worth
a mention - any such leak can cause problems when the PostgreSQL
installation is either unattended, long-living andor has very high
connection levels. Half a kilobyte on start-up isn't negligible in
this light.

Regards, Lee.

Tom Lane writes:
 > Lee Kindness <[EMAIL PROTECTED]> writes:
 > > Guys, attached is a patch to fix two memory leaks on start-up.
 > 
 > I do not like the changes to miscinit.c.  In the first place, it is not
 > a "memory leak" to do a one-time allocation of state for a proc_exit
 > function.  A bigger complaint is that your proposed change introduces
 > fragile coupling between CreateLockFile and its callers, in order to
 > save no resources worth mentioning.  More, it introduces an assumption
 > that the globals directoryLockFile and socketLockFile don't change while
 > the postmaster is running.  UnlinkLockFile should unlink the file that
 > it was originally told to unlink, regardless of what happens to those
 > globals.
 > 
 > If you are intent on spending code to free stuff just before the
 > postmaster exits, a better fix would be for UnlinkLockFile to free its
 > string argument after using it.

Index: src/backend/libpq/pqcomm.c
===
RCS file: /projects/cvsroot/pgsql-server/src/backend/libpq/pqcomm.c,v
retrieving revision 1.157
diff -u -r1.157 pqcomm.c
--- src/backend/libpq/pqcomm.c  12 Jun 2003 07:36:51 -  1.157
+++ src/backend/libpq/pqcomm.c  22 Jul 2003 14:16:46 -
@@ -363,7 +363,7 @@
added++;
}
 
-   freeaddrinfo(addrs);
+   freeaddrinfo2(family, addrs);
 
if (!added)
{
Index: src/backend/utils/init/miscinit.c
===
RCS file: /projects/cvsroot/pgsql-server/src/backend/utils/init/miscinit.c,v
retrieving revision 1.104
diff -u -r1.104 miscinit.c
--- src/backend/utils/init/miscinit.c   27 Jun 2003 19:08:37 -  1.104
+++ src/backend/utils/init/miscinit.c   22 Jul 2003 14:16:46 -
@@ -673,8 +673,15 @@
 static void
 UnlinkLockFile(int status, Datum filename)
 {
-   unlink((char *) DatumGetPointer(filename));
-   /* Should we complain if the unlink fails? */
+  char *fname = (char *)DatumGetPointer(filename);
+  if( fname != NULL )
+{
+  if( unlink(fname) != 0 )
+   {
+ /* Should we complain if the unlink fails? */
+   }
+  free(fname);
+}
 }
 
 /*

---(end of broadcast)---
TIP 7: don't forget to increase your free space map settings

Re: [PATCHES] PATCH: Memory leaks on start-up

2003-07-22 Thread Tom Lane 
Lee Kindness <[EMAIL PROTECTED]> writes:
> Guys, attached is a patch to fix two memory leaks on start-up.

I do not like the changes to miscinit.c.  In the first place, it is not
a "memory leak" to do a one-time allocation of state for a proc_exit
function.  A bigger complaint is that your proposed change introduces
fragile coupling between CreateLockFile and its callers, in order to
save no resources worth mentioning.  More, it introduces an assumption
that the globals directoryLockFile and socketLockFile don't change while
the postmaster is running.  UnlinkLockFile should unlink the file that
it was originally told to unlink, regardless of what happens to those
globals.

If you are intent on spending code to free stuff just before the
postmaster exits, a better fix would be for UnlinkLockFile to free its
string argument after using it.

regards, tom lane

---(end of broadcast)---
TIP 5: Have you checked our extensive FAQ?

   http://www.postgresql.org/docs/faqs/FAQ.html

[PATCHES] PATCH: Memory leaks on start-up

2003-07-22 Thread Lee Kindness 
Guys, attached is a patch to fix two memory leaks on start-up. The
first is when freeaddrinfo has been used to free memory allocated by
getaddrinfo2 (so freeaddrinfo2 should be used). The second is 2 leaks
when creating the lock (PostgreSQL and socket) files.

The diff is against last night's CVS HEAD.

L.

diff -cr pgsql.old/src/backend/libpq/pqcomm.c pgsql/src/backend/libpq/pqcomm.c
*** pgsql.old/src/backend/libpq/pqcomm.c2003-06-12 08:36:51.0 +0100
--- pgsql/src/backend/libpq/pqcomm.c2003-07-21 22:58:39.0 +0100
***
*** 363,369 
added++;
}
  
!   freeaddrinfo(addrs);
  
if (!added)
{
--- 363,369 
added++;
}
  
!   freeaddrinfo2(family, addrs);
  
if (!added)
{
diff -cr pgsql.old/src/backend/utils/init/miscinit.c 
pgsql/src/backend/utils/init/miscinit.c
*** pgsql.old/src/backend/utils/init/miscinit.c 2003-06-27 20:08:37.0 +0100
--- pgsql/src/backend/utils/init/miscinit.c 2003-07-22 00:08:39.0 +0100
***
*** 842,848 
/*
 * Arrange for automatic removal of lockfile at proc_exit.
 */
!   on_proc_exit(UnlinkLockFile, PointerGetDatum(strdup(filename)));
  
return true;/* Success! */
  }
--- 842,848 
/*
 * Arrange for automatic removal of lockfile at proc_exit.
 */
!   on_proc_exit(UnlinkLockFile, PointerGetDatum(filename));
  
return true;/* Success! */
  }
***
*** 850,875 
  bool
  CreateDataDirLockFile(const char *datadir, bool amPostmaster)
  {
!   charlockfile[MAXPGPATH];
! 
!   snprintf(lockfile, sizeof(lockfile), "%s/postmaster.pid", datadir);
!   if (!CreateLockFile(lockfile, amPostmaster, true, datadir))
!   return false;
!   /* Save name of lockfile for RecordSharedMemoryInLockFile */
!   strcpy(directoryLockFile, lockfile);
return true;
  }
  
  bool
  CreateSocketLockFile(const char *socketfile, bool amPostmaster)
  {
!   charlockfile[MAXPGPATH];
! 
!   snprintf(lockfile, sizeof(lockfile), "%s.lock", socketfile);
!   if (!CreateLockFile(lockfile, amPostmaster, false, socketfile))
!   return false;
!   /* Save name of lockfile for TouchSocketLockFile */
!   strcpy(socketLockFile, lockfile);
return true;
  }
  
--- 850,873 
  bool
  CreateDataDirLockFile(const char *datadir, bool amPostmaster)
  {
!   snprintf(directoryLockFile, sizeof(directoryLockFile), "%s/postmaster.pid", 
datadir);
!   if (!CreateLockFile(directoryLockFile, amPostmaster, true, datadir))
! {
!   *directoryLockFile = '\0';
!   return false;
! }
return true;
  }
  
  bool
  CreateSocketLockFile(const char *socketfile, bool amPostmaster)
  {
!   snprintf(socketLockFile, sizeof(socketLockFile), "%s.lock", socketfile);
!   if (!CreateLockFile(socketLockFile, amPostmaster, false, socketfile))
! {
!   *socketLockFile = '\0';
!   return false;
! }
return true;
  }
  

---(end of broadcast)---
TIP 8: explain analyze is your friend