Thread: Win32: Minimising desktop heap usage

A discussion on -general has been ongoing in which is was discovered
that 8.3 can exhaust the desktop heap allocated to a service with ~45
connections at which point the server will crash.

Desktop heap is allocated by user32.dll and shell32 on a per-process
basis from a 512KB pool for a non-interactive logon (on XP) and 3072KB
pool for an interactive logon. The attached patch removes our direct
dependencies on these DLLs, reducing the overhead from around 9.7KB per
backend to around 3.5KB. This is back in the range we had with 8.2.

Unfortunately, there are still indirect dependencies on user32.dll which
I don't think we'll be able to do anything about. If desktop heap is
still exhausted with larger installations, the allocated heap size can
be increased in the registry.

See
http://blogs.msdn.com/ntdebugging/archive/2007/01/04/desktop-heap-overview.aspx
for more info.

Regards, Dave
Index: src/backend/port/win32/signal.c
===================================================================
RCS file: /projects/cvsroot/pgsql/src/backend/port/win32/signal.c,v
retrieving revision 1.18
diff -c -r1.18 signal.c
*** src/backend/port/win32/signal.c    5 Jan 2007 22:19:35 -0000    1.18
--- src/backend/port/win32/signal.c    23 Oct 2007 14:44:35 -0000
***************
*** 178,184 ****
      char        pipename[128];
      HANDLE        pipe;

!     wsprintf(pipename, "\\\\.\\pipe\\pgsignal_%d", (int) pid);

      pipe = CreateNamedPipe(pipename, PIPE_ACCESS_DUPLEX,
                         PIPE_TYPE_MESSAGE | PIPE_READMODE_MESSAGE | PIPE_WAIT,
--- 178,184 ----
      char        pipename[128];
      HANDLE        pipe;

!     snprintf(pipename, sizeof(pipename) - 1, "\\\\.\\pipe\\pgsignal_%u", (int) pid);

      pipe = CreateNamedPipe(pipename, PIPE_ACCESS_DUPLEX,
                         PIPE_TYPE_MESSAGE | PIPE_READMODE_MESSAGE | PIPE_WAIT,
***************
*** 251,257 ****
      char        pipename[128];
      HANDLE        pipe = pgwin32_initial_signal_pipe;

!     wsprintf(pipename, "\\\\.\\pipe\\pgsignal_%d", GetCurrentProcessId());

      for (;;)
      {
--- 251,257 ----
      char        pipename[128];
      HANDLE        pipe = pgwin32_initial_signal_pipe;

!     snprintf(pipename, sizeof(pipename) - 1, "\\\\.\\pipe\\pgsignal_%u", GetCurrentProcessId());

      for (;;)
      {
Index: src/port/kill.c
===================================================================
RCS file: /projects/cvsroot/pgsql/src/port/kill.c,v
retrieving revision 1.8
diff -c -r1.8 kill.c
*** src/port/kill.c    5 Jan 2007 22:20:02 -0000    1.8
--- src/port/kill.c    23 Oct 2007 14:44:35 -0000
***************
*** 38,44 ****
          errno = EINVAL;
          return -1;
      }
!     wsprintf(pipename, "\\\\.\\pipe\\pgsignal_%i", pid);
      if (!CallNamedPipe(pipename, &sigData, 1, &sigRet, 1, &bytes, 1000))
      {
          if (GetLastError() == ERROR_FILE_NOT_FOUND)
--- 38,44 ----
          errno = EINVAL;
          return -1;
      }
!     snprintf(pipename, sizeof(pipename) - 1, "\\\\.\\pipe\\pgsignal_%u", pid);
      if (!CallNamedPipe(pipename, &sigData, 1, &sigRet, 1, &bytes, 1000))
      {
          if (GetLastError() == ERROR_FILE_NOT_FOUND)
Index: src/port/path.c
===================================================================
RCS file: /projects/cvsroot/pgsql/src/port/path.c,v
retrieving revision 1.71
diff -c -r1.71 path.c
*** src/port/path.c    5 Jan 2007 22:20:02 -0000    1.71
--- src/port/path.c    23 Oct 2007 14:47:29 -0000
***************
*** 628,637 ****
      strlcpy(ret_path, pwd->pw_dir, MAXPGPATH);
      return true;
  #else
!     char        tmppath[MAX_PATH];

      ZeroMemory(tmppath, sizeof(tmppath));
!     if (SHGetFolderPath(NULL, CSIDL_APPDATA, NULL, 0, tmppath) != S_OK)
          return false;
      snprintf(ret_path, MAXPGPATH, "%s/postgresql", tmppath);
      return true;
--- 628,643 ----
      strlcpy(ret_path, pwd->pw_dir, MAXPGPATH);
      return true;
  #else
!     char        *tmppath=0;

      ZeroMemory(tmppath, sizeof(tmppath));
!
!     /*
!      * Note: We use getenv here because the more modern SHGetSpecialFolderPath()
!      * will force us to link with shell32.lib which eats valuable desktop heap.
!      */
!     tmppath = getenv("APPDATA");
!     if (!tmppath)
          return false;
      snprintf(ret_path, MAXPGPATH, "%s/postgresql", tmppath);
      return true;

Dave Page <dpage@postgresql.org> writes:
> [ get rid of wsprintf in favor of snprintf ]

+1 for not depending on nonstandard subroutines without need.
But please note the standard locution is snprintf(buf, sizeof(buf), ...
Not sizeof() - 1.

> !     char        *tmppath=0;

Please use NULL not 0 ... actually, since the variable is
unconditionally assigned to just below, I'd say this should just be
"char *tmppath;".  I don't like useless initializations: if the compiler
is not smart enough to discard them then they waste cycles, and they
also increase the risk of bugs-of-omission in future changes.  If
someone were to change things around so that tmppath didn't get assigned
to in one code path, then the compiler would complain about use of an
uninitialized variable --- *unless* you've written a useless
initialization such as the above, in which case the mistake might pass
unnoticed for awhile.  So don't initialize a local variable unless
you're giving it an actually meaningful value.

> !     /*
> !      * Note: We use getenv here because the more modern SHGetSpecialFolderPath()
> !      * will force us to link with shell32.lib which eats valuable desktop heap.
> !      */
> !     tmppath = getenv("APPDATA");

Hmm, is there any functional downside to this?  I suppose
SHGetSpecialFolderPath does some things that getenv does not...

Otherwise it looks good...

            regards, tom lane

Tom Lane wrote:
> Dave Page <dpage@postgresql.org> writes:
>> [ get rid of wsprintf in favor of snprintf ]
>
> +1 for not depending on nonstandard subroutines without need.
> But please note the standard locution is snprintf(buf, sizeof(buf), ...
> Not sizeof() - 1.

Noted.

>> !     char        *tmppath=0;
>
> Please use NULL not 0 ... actually, since the variable is
> unconditionally assigned to just below, I'd say this should just be
> "char *tmppath;".  I don't like useless initializations: if the compiler
> is not smart enough to discard them then they waste cycles, and they
> also increase the risk of bugs-of-omission in future changes.  If
> someone were to change things around so that tmppath didn't get assigned
> to in one code path, then the compiler would complain about use of an
> uninitialized variable --- *unless* you've written a useless
> initialization such as the above, in which case the mistake might pass
> unnoticed for awhile.  So don't initialize a local variable unless
> you're giving it an actually meaningful value.

The downside is that we see a useless warning that, if sufficiently
multiplied, might make it hard to see something we are interested in.

In this case, not initialising it will also create the first 'accepted'
warning in VC++ in our code :-(. All the others come from Kerberos.

Modified in the attached patch however.

>> !     /*
>> !      * Note: We use getenv here because the more modern SHGetSpecialFolderPath()
>> !      * will force us to link with shell32.lib which eats valuable desktop heap.
>> !      */
>> !     tmppath = getenv("APPDATA");
>
> Hmm, is there any functional downside to this?  I suppose
> SHGetSpecialFolderPath does some things that getenv does not...

A good percentage of the special folder paths you might query with
SHGetSpecialFolderPath() are not actually in the environment. APPDATA
certainly is on XP, 2K3 and Vista though, and I've found MS KB articles
referring to it being on 2K as well.

Regards, Dave.
Index: src/backend/port/win32/signal.c
===================================================================
RCS file: /projects/cvsroot/pgsql/src/backend/port/win32/signal.c,v
retrieving revision 1.18
diff -c -r1.18 signal.c
*** src/backend/port/win32/signal.c    5 Jan 2007 22:19:35 -0000    1.18
--- src/backend/port/win32/signal.c    23 Oct 2007 14:44:35 -0000
***************
*** 178,184 ****
      char        pipename[128];
      HANDLE        pipe;

!     wsprintf(pipename, "\\\\.\\pipe\\pgsignal_%d", (int) pid);

      pipe = CreateNamedPipe(pipename, PIPE_ACCESS_DUPLEX,
                         PIPE_TYPE_MESSAGE | PIPE_READMODE_MESSAGE | PIPE_WAIT,
--- 178,184 ----
      char        pipename[128];
      HANDLE        pipe;

!     snprintf(pipename, sizeof(pipename), "\\\\.\\pipe\\pgsignal_%u", (int) pid);

      pipe = CreateNamedPipe(pipename, PIPE_ACCESS_DUPLEX,
                         PIPE_TYPE_MESSAGE | PIPE_READMODE_MESSAGE | PIPE_WAIT,
***************
*** 251,257 ****
      char        pipename[128];
      HANDLE        pipe = pgwin32_initial_signal_pipe;

!     wsprintf(pipename, "\\\\.\\pipe\\pgsignal_%d", GetCurrentProcessId());

      for (;;)
      {
--- 251,257 ----
      char        pipename[128];
      HANDLE        pipe = pgwin32_initial_signal_pipe;

!     snprintf(pipename, sizeof(pipename), "\\\\.\\pipe\\pgsignal_%u", GetCurrentProcessId());

      for (;;)
      {
Index: src/port/kill.c
===================================================================
RCS file: /projects/cvsroot/pgsql/src/port/kill.c,v
retrieving revision 1.8
diff -c -r1.8 kill.c
*** src/port/kill.c    5 Jan 2007 22:20:02 -0000    1.8
--- src/port/kill.c    23 Oct 2007 14:44:35 -0000
***************
*** 38,44 ****
          errno = EINVAL;
          return -1;
      }
!     wsprintf(pipename, "\\\\.\\pipe\\pgsignal_%i", pid);
      if (!CallNamedPipe(pipename, &sigData, 1, &sigRet, 1, &bytes, 1000))
      {
          if (GetLastError() == ERROR_FILE_NOT_FOUND)
--- 38,44 ----
          errno = EINVAL;
          return -1;
      }
!     snprintf(pipename, sizeof(pipename), "\\\\.\\pipe\\pgsignal_%u", pid);
      if (!CallNamedPipe(pipename, &sigData, 1, &sigRet, 1, &bytes, 1000))
      {
          if (GetLastError() == ERROR_FILE_NOT_FOUND)
Index: src/port/path.c
===================================================================
RCS file: /projects/cvsroot/pgsql/src/port/path.c,v
retrieving revision 1.71
diff -c -r1.71 path.c
*** src/port/path.c    5 Jan 2007 22:20:02 -0000    1.71
--- src/port/path.c    23 Oct 2007 14:47:29 -0000
***************
*** 628,637 ****
      strlcpy(ret_path, pwd->pw_dir, MAXPGPATH);
      return true;
  #else
!     char        tmppath[MAX_PATH];

      ZeroMemory(tmppath, sizeof(tmppath));
!     if (SHGetFolderPath(NULL, CSIDL_APPDATA, NULL, 0, tmppath) != S_OK)
          return false;
      snprintf(ret_path, MAXPGPATH, "%s/postgresql", tmppath);
      return true;
--- 628,643 ----
      strlcpy(ret_path, pwd->pw_dir, MAXPGPATH);
      return true;
  #else
!     char        *tmppath;

      ZeroMemory(tmppath, sizeof(tmppath));
!
!     /*
!      * Note: We use getenv here because the more modern SHGetSpecialFolderPath()
!      * will force us to link with shell32.lib which eats valuable desktop heap.
!      */
!     tmppath = getenv("APPDATA");
!     if (!tmppath)
          return false;
      snprintf(ret_path, MAXPGPATH, "%s/postgresql", tmppath);
      return true;

Dave Page <dpage@postgresql.org> writes:
> Tom Lane wrote:
>> So don't initialize a local variable unless
>> you're giving it an actually meaningful value.

> The downside is that we see a useless warning that, if sufficiently
> multiplied, might make it hard to see something we are interested in.

[ looks again... ]  Actually, I think you just proved my point for me.
The ZeroMemory call should go away, no?  (Does this mean that the
Windows builds fail to detect dereferencing NULL?  Bad if so.)

>> Hmm, is there any functional downside to this?  I suppose
>> SHGetSpecialFolderPath does some things that getenv does not...

> A good percentage of the special folder paths you might query with
> SHGetSpecialFolderPath() are not actually in the environment. APPDATA
> certainly is on XP, 2K3 and Vista though, and I've found MS KB articles
> referring to it being on 2K as well.

OK, in that case seems no problem.

            regards, tom lane

"Tom Lane" <tgl@sss.pgh.pa.us> writes:

>> !     /*
>> !      * Note: We use getenv here because the more modern SHGetSpecialFolderPath()
>> !      * will force us to link with shell32.lib which eats valuable desktop heap.
>> !      */
>> !     tmppath = getenv("APPDATA");
>
> Hmm, is there any functional downside to this?  I suppose
> SHGetSpecialFolderPath does some things that getenv does not...

The functional difference appears to be that the environment variable is set
on startup (or login?) and doesn't necessarily have the most up to date value
if it's been changed. But it's not something you're likely to change and you
can always adjust the environment variable manually to fix the problem.

--
  Gregory Stark
  EnterpriseDB          http://www.enterprisedb.com

Tom Lane wrote:
> [ looks again... ]  Actually, I think you just proved my point for me.
> The ZeroMemory call should go away, no?

Yup, quite correct. v3 attached.

/D
Index: src/backend/port/win32/signal.c
===================================================================
RCS file: /projects/cvsroot/pgsql/src/backend/port/win32/signal.c,v
retrieving revision 1.18
diff -c -r1.18 signal.c
*** src/backend/port/win32/signal.c    5 Jan 2007 22:19:35 -0000    1.18
--- src/backend/port/win32/signal.c    23 Oct 2007 16:02:41 -0000
***************
*** 178,184 ****
      char        pipename[128];
      HANDLE        pipe;

!     wsprintf(pipename, "\\\\.\\pipe\\pgsignal_%d", (int) pid);

      pipe = CreateNamedPipe(pipename, PIPE_ACCESS_DUPLEX,
                         PIPE_TYPE_MESSAGE | PIPE_READMODE_MESSAGE | PIPE_WAIT,
--- 178,184 ----
      char        pipename[128];
      HANDLE        pipe;

!     snprintf(pipename, sizeof(pipename), "\\\\.\\pipe\\pgsignal_%u", (int) pid);

      pipe = CreateNamedPipe(pipename, PIPE_ACCESS_DUPLEX,
                         PIPE_TYPE_MESSAGE | PIPE_READMODE_MESSAGE | PIPE_WAIT,
***************
*** 251,257 ****
      char        pipename[128];
      HANDLE        pipe = pgwin32_initial_signal_pipe;

!     wsprintf(pipename, "\\\\.\\pipe\\pgsignal_%d", GetCurrentProcessId());

      for (;;)
      {
--- 251,257 ----
      char        pipename[128];
      HANDLE        pipe = pgwin32_initial_signal_pipe;

!     snprintf(pipename, sizeof(pipename), "\\\\.\\pipe\\pgsignal_%u", GetCurrentProcessId());

      for (;;)
      {
Index: src/port/kill.c
===================================================================
RCS file: /projects/cvsroot/pgsql/src/port/kill.c,v
retrieving revision 1.8
diff -c -r1.8 kill.c
*** src/port/kill.c    5 Jan 2007 22:20:02 -0000    1.8
--- src/port/kill.c    23 Oct 2007 16:02:41 -0000
***************
*** 38,44 ****
          errno = EINVAL;
          return -1;
      }
!     wsprintf(pipename, "\\\\.\\pipe\\pgsignal_%i", pid);
      if (!CallNamedPipe(pipename, &sigData, 1, &sigRet, 1, &bytes, 1000))
      {
          if (GetLastError() == ERROR_FILE_NOT_FOUND)
--- 38,44 ----
          errno = EINVAL;
          return -1;
      }
!     snprintf(pipename, sizeof(pipename), "\\\\.\\pipe\\pgsignal_%u", pid);
      if (!CallNamedPipe(pipename, &sigData, 1, &sigRet, 1, &bytes, 1000))
      {
          if (GetLastError() == ERROR_FILE_NOT_FOUND)
Index: src/port/path.c
===================================================================
RCS file: /projects/cvsroot/pgsql/src/port/path.c,v
retrieving revision 1.71
diff -c -r1.71 path.c
*** src/port/path.c    5 Jan 2007 22:20:02 -0000    1.71
--- src/port/path.c    23 Oct 2007 16:04:42 -0000
***************
*** 628,637 ****
      strlcpy(ret_path, pwd->pw_dir, MAXPGPATH);
      return true;
  #else
!     char        tmppath[MAX_PATH];

!     ZeroMemory(tmppath, sizeof(tmppath));
!     if (SHGetFolderPath(NULL, CSIDL_APPDATA, NULL, 0, tmppath) != S_OK)
          return false;
      snprintf(ret_path, MAXPGPATH, "%s/postgresql", tmppath);
      return true;
--- 628,641 ----
      strlcpy(ret_path, pwd->pw_dir, MAXPGPATH);
      return true;
  #else
!     char        *tmppath;

!     /*
!      * Note: We use getenv here because the more modern SHGetSpecialFolderPath()
!      * will force us to link with shell32.lib which eats valuable desktop heap.
!      */
!     tmppath = getenv("APPDATA");
!     if (!tmppath)
          return false;
      snprintf(ret_path, MAXPGPATH, "%s/postgresql", tmppath);
      return true;

Gregory Stark wrote:
> "Tom Lane" <tgl@sss.pgh.pa.us> writes:
>
>>> !     /*
>>> !      * Note: We use getenv here because the more modern SHGetSpecialFolderPath()
>>> !      * will force us to link with shell32.lib which eats valuable desktop heap.
>>> !      */
>>> !     tmppath = getenv("APPDATA");
>> Hmm, is there any functional downside to this?  I suppose
>> SHGetSpecialFolderPath does some things that getenv does not...
>
> The functional difference appears to be that the environment variable is set
> on startup (or login?) and doesn't necessarily have the most up to date value
> if it's been changed. But it's not something you're likely to change and you
> can always adjust the environment variable manually to fix the problem.

If you're hacking the registry to change it then you've only got
yourself to blame if you don't update the environment as well imho.

I also wouldn't be at all surprised if many apps build paths containing
the value returned by SHGetSpecialFolderPath() (or %APPDATA%) at startup
and then use that value from then on rather than repeatedly calling the
API function. It's not like you'd expect the value to change at all
often, if ever.

/D

Tom Lane wrote:
> Dave Page <dpage@postgresql.org> writes:
>> Tom Lane wrote:
>>> So don't initialize a local variable unless
>>> you're giving it an actually meaningful value.
>
>> The downside is that we see a useless warning that, if sufficiently
>> multiplied, might make it hard to see something we are interested in.
>
> [ looks again... ]  Actually, I think you just proved my point for me.
> The ZeroMemory call should go away, no?  (Does this mean that the
> Windows builds fail to detect dereferencing NULL?  Bad if so.)

Windows builds don't fail to detect that in genereal. ZeroMemory,
however, has a protection specifically against being passed NULL input,
IIRC.

//Magnus

Dave Page wrote:
> Tom Lane wrote:
>> [ looks again... ]  Actually, I think you just proved my point for me.
>> The ZeroMemory call should go away, no?
>
> Yup, quite correct. v3 attached.

Great job tracking this down!

Patch looks good from here (after the fixes per Tom).

Applied, many thanks!

//Magnus