Thread: Re: 8.2.3: Server crashes on Windows using Eclipse/Junit
> > Taking this one to -hackers once and for all now... > > > > Can you try the attached patch? See how many backends you can get up to. > > Regression tests run just fine, and I've run multiple pgbench runs with > 3 and 4 sessions of 100 connections each*, with pgAdmin monitoring > things at the same time. Saw up to 403 simultanteous connections in > pg_stat_activity, and the system remained stable and responsive, albeit > somewhat slower than normal. What was the memory space consumption of the postmaster process, and compared to without the patch? VM size in taskmgr should show that I think, and should show a much smaller footprint now.. /Magnus
Magnus Hagander wrote: > VM size in taskmgr should show that I think, and should show a much > smaller footprint now.. With patch - 4,492K Without patch: 28,224K Thats with 3 x 100 pgbench connections. /D
Dave Page wrote: > Magnus Hagander wrote: >> VM size in taskmgr should show that I think, and should show a much >> smaller footprint now.. > > With patch - 4,492K > Without patch: 28,224K > > Thats with 3 x 100 pgbench connections. That's nice! But. That can't be address space usage, it has to be actual memory usage. Since each thread should chew up 4Mb of address space, and there's at least two threads in there :-) So looking at the VM column was obviously not correct. * looks up some docs* Right. You need to look at VM size in *process explorer*. VM size in task manager has nothing to do with VM size, it's the private bytes :-S And there is no way to see that info from task manager, I think. PE is your friend. Anyway. Other than a refresher on those, I'd be interested in two other important parts: * How many threads does it reach when you have 300 active backends? * Is there a handle leak? meaning once your 300 backends have exited, does the number of handles in the process drop down to the same value it had before? (sorry, wish I was in a position to run these tests myself, but I'm not right now) //Magnus
Magnus Hagander wrote: > Right. You need to look at VM size in *process explorer*. VM size in > task manager has nothing to do with VM size, it's the private bytes :-S > And there is no way to see that info from task manager, I think. PE is > your friend. > > > Anyway. Other than a refresher on those, I'd be interested in two other > important parts: > * How many threads does it reach when you have 300 active backends? > * Is there a handle leak? meaning once your 300 backends have exited, > does the number of handles in the process drop down to the same value it > had before? Without patch: VM: 1,322,792K Idle threads: 6 Peak threads: 306 Handles at start: 576 Handles at end: 576 With patch: VM: 98,088K Idle threads: 3 Peak threads: 7 Handles at start: 576 Handles at end: 585 (585 again after second run). /D
Dave Page wrote:
> Magnus Hagander wrote:
>> Right. You need to look at VM size in *process explorer*. VM size in
>> task manager has nothing to do with VM size, it's the private bytes :-S
>> And there is no way to see that info from task manager, I think. PE is
>> your friend.
>>
>>
>> Anyway. Other than a refresher on those, I'd be interested in two other
>> important parts:
>> * How many threads does it reach when you have 300 active backends?
>> * Is there a handle leak? meaning once your 300 backends have exited,
>> does the number of handles in the process drop down to the same value it
>> had before?
>
> Without patch:
>
> VM: 1,322,792K
> Idle threads: 6
> Peak threads: 306
> Handles at start: 576
> Handles at end: 576
>
> With patch:
>
> VM: 98,088K
> Idle threads: 3
> Peak threads: 7
> Handles at start: 576
> Handles at end: 585 (585 again after second run).
Ah, now we're talking. That's the kind of reduction I was looking for :-)
I think the difference in handles is because the threadpool keeps some
things around. As long as it stays at 585 and comes back down after a
second run, we're fine at that - there's no leak.
Attached is an updated version of the patch, currently being tested by
both me and Dave. If it passes our tests, I'll apply this so it gets
included for broader testing in beta2.
//Magnus
Index: src/backend/postmaster/postmaster.c
===================================================================
RCS file: /projects/cvsroot/pgsql/src/backend/postmaster/postmaster.c,v
retrieving revision 1.542
diff -c -r1.542 postmaster.c
*** src/backend/postmaster/postmaster.c 26 Sep 2007 22:36:30 -0000 1.542
--- src/backend/postmaster/postmaster.c 26 Oct 2007 20:09:35 -0000
***************
*** 331,344 ****
#ifdef EXEC_BACKEND
#ifdef WIN32
- static void win32_AddChild(pid_t pid, HANDLE handle);
- static void win32_RemoveChild(pid_t pid);
static pid_t win32_waitpid(int *exitstatus);
! static DWORD WINAPI win32_sigchld_waiter(LPVOID param);
! static pid_t *win32_childPIDArray;
! static HANDLE *win32_childHNDArray;
! static unsigned long win32_numChildren = 0;
HANDLE PostmasterHandle;
#endif
--- 331,347 ----
#ifdef EXEC_BACKEND
#ifdef WIN32
static pid_t win32_waitpid(int *exitstatus);
! static void WINAPI pgwin32_deadchild_callback(PVOID lpParameter, BOOLEAN TimerOrWaitFired);
! static HANDLE win32ChildQueue;
!
! typedef struct
! {
! HANDLE waitHandle;
! HANDLE procHandle;
! DWORD procId;
! } win32_deadchild_waitinfo;
HANDLE PostmasterHandle;
#endif
***************
*** 899,914 ****
#ifdef WIN32
/*
! * Initialize the child pid/HANDLE arrays for signal handling.
*/
! win32_childPIDArray = (pid_t *)
! malloc(mul_size(NUM_BACKENDARRAY_ELEMS, sizeof(pid_t)));
! win32_childHNDArray = (HANDLE *)
! malloc(mul_size(NUM_BACKENDARRAY_ELEMS, sizeof(HANDLE)));
! if (!win32_childPIDArray || !win32_childHNDArray)
ereport(FATAL,
! (errcode(ERRCODE_OUT_OF_MEMORY),
! errmsg("out of memory")));
/*
* Set up a handle that child processes can use to check whether the
--- 902,913 ----
#ifdef WIN32
/*
! * Initialize I/O completion port used to deliver list of dead children.
*/
! win32ChildQueue = CreateIoCompletionPort(INVALID_HANDLE_VALUE, NULL, 0, 1);
! if (win32ChildQueue == NULL)
ereport(FATAL,
! (errmsg("could not create I/O completion port for child queue")));
/*
* Set up a handle that child processes can use to check whether the
***************
*** 2072,2083 ****
#define LOOPHEADER() (exitstatus = status.w_status)
#else /* WIN32 */
#define LOOPTEST() ((pid = win32_waitpid(&exitstatus)) > 0)
! /*
! * We need to do this here, and not in CleanupBackend, since this is
! * to be called on all children when we are done with them. Could move
! * to LogChildExit, but that seems like asking for future trouble...
! */
! #define LOOPHEADER() (win32_RemoveChild(pid))
#endif /* WIN32 */
#endif /* HAVE_WAITPID */
--- 2071,2077 ----
#define LOOPHEADER() (exitstatus = status.w_status)
#else /* WIN32 */
#define LOOPTEST() ((pid = win32_waitpid(&exitstatus)) > 0)
! #define LOOPHEADER()
#endif /* WIN32 */
#endif /* HAVE_WAITPID */
***************
*** 3332,3343 ****
int i;
int j;
char cmdLine[MAXPGPATH * 2];
- HANDLE childHandleCopy;
- HANDLE waiterThread;
HANDLE paramHandle;
BackendParameters *param;
SECURITY_ATTRIBUTES sa;
char paramHandleStr[32];
/* Make sure caller set up argv properly */
Assert(argc >= 3);
--- 3326,3336 ----
int i;
int j;
char cmdLine[MAXPGPATH * 2];
HANDLE paramHandle;
BackendParameters *param;
SECURITY_ATTRIBUTES sa;
char paramHandleStr[32];
+ win32_deadchild_waitinfo *childinfo;
/* Make sure caller set up argv properly */
Assert(argc >= 3);
***************
*** 3345,3359 ****
Assert(strncmp(argv[1], "--fork", 6) == 0);
Assert(argv[2] == NULL);
- /* Verify that there is room in the child list */
- if (win32_numChildren >= NUM_BACKENDARRAY_ELEMS)
- {
- elog(LOG, "no room for child entry in backend list");
- /* Report same error as for a fork failure on Unix */
- errno = EAGAIN;
- return -1;
- }
-
/* Set up shared memory for parameter passing */
ZeroMemory(&sa, sizeof(sa));
sa.nLength = sizeof(sa);
--- 3338,3343 ----
***************
*** 3463,3496 ****
return -1;
}
! if (!IsUnderPostmaster)
! {
! /* We are the Postmaster creating a child... */
! win32_AddChild(pi.dwProcessId, pi.hProcess);
! }
! /* Set up the thread to handle the SIGCHLD for this process */
! if (DuplicateHandle(GetCurrentProcess(),
! pi.hProcess,
! GetCurrentProcess(),
! &childHandleCopy,
! 0,
! FALSE,
! DUPLICATE_SAME_ACCESS) == 0)
ereport(FATAL,
! (errmsg_internal("could not duplicate child handle: error code %d",
(int) GetLastError())));
! waiterThread = CreateThread(NULL, 64 * 1024, win32_sigchld_waiter,
! (LPVOID) childHandleCopy, 0, NULL);
! if (!waiterThread)
! ereport(FATAL,
! (errmsg_internal("could not create sigchld waiter thread: error code %d",
! (int) GetLastError())));
! CloseHandle(waiterThread);
- if (IsUnderPostmaster)
- CloseHandle(pi.hProcess);
CloseHandle(pi.hThread);
return pi.dwProcessId;
--- 3447,3479 ----
return -1;
}
! /*
! * Queue a waiter for to signal when this child dies. The wait will be handled automatically
! * by an operating system thread pool.
! *
! * Note: use malloc instead of palloc, since it needs to be thread-safe
! */
! childinfo = malloc(sizeof(win32_deadchild_waitinfo));
! if (!childinfo)
! ereport(FATAL,
! (errcode(ERRCODE_OUT_OF_MEMORY),
! errmsg("out of memory")));
! childinfo->procHandle = pi.hProcess;
! childinfo->procId = pi.dwProcessId;
!
! if (!RegisterWaitForSingleObject(&childinfo->waitHandle,
! pi.hProcess,
! pgwin32_deadchild_callback,
! childinfo,
! INFINITE,
! WT_EXECUTEONLYONCE | WT_EXECUTEINWAITTHREAD))
ereport(FATAL,
! (errmsg_internal("could not register process for wait: error code %d",
(int) GetLastError())));
! /* Don't close pi.hProcess here - the wait thread needs access to it */
CloseHandle(pi.hThread);
return pi.dwProcessId;
***************
*** 4500,4636 ****
#ifdef WIN32
- /*
- * Note: The following three functions must not be interrupted (eg. by
- * signals). As the Postgres Win32 signalling architecture (currently)
- * requires polling, or APC checking functions which aren't used here, this
- * is not an issue.
- *
- * We keep two separate arrays, instead of a single array of pid/HANDLE
- * structs, to avoid having to re-create a handle array for
- * WaitForMultipleObjects on each call to win32_waitpid.
- */
-
- static void
- win32_AddChild(pid_t pid, HANDLE handle)
- {
- Assert(win32_childPIDArray && win32_childHNDArray);
- if (win32_numChildren < NUM_BACKENDARRAY_ELEMS)
- {
- win32_childPIDArray[win32_numChildren] = pid;
- win32_childHNDArray[win32_numChildren] = handle;
- ++win32_numChildren;
- }
- else
- ereport(FATAL,
- (errmsg_internal("no room for child entry with pid %lu",
- (unsigned long) pid)));
- }
-
- static void
- win32_RemoveChild(pid_t pid)
- {
- int i;
-
- Assert(win32_childPIDArray && win32_childHNDArray);
-
- for (i = 0; i < win32_numChildren; i++)
- {
- if (win32_childPIDArray[i] == pid)
- {
- CloseHandle(win32_childHNDArray[i]);
-
- /* Swap last entry into the "removed" one */
- --win32_numChildren;
- win32_childPIDArray[i] = win32_childPIDArray[win32_numChildren];
- win32_childHNDArray[i] = win32_childHNDArray[win32_numChildren];
- return;
- }
- }
-
- ereport(WARNING,
- (errmsg_internal("could not find child entry with pid %lu",
- (unsigned long) pid)));
- }
-
static pid_t
win32_waitpid(int *exitstatus)
{
/*
! * Note: Do NOT use WaitForMultipleObjectsEx, as we don't want to run
! * queued APCs here.
*/
! int index;
! DWORD exitCode;
! DWORD ret;
! unsigned long offset;
!
! Assert(win32_childPIDArray && win32_childHNDArray);
! elog(DEBUG3, "waiting on %lu children", win32_numChildren);
!
! for (offset = 0; offset < win32_numChildren; offset += MAXIMUM_WAIT_OBJECTS)
{
! unsigned long num = Min(MAXIMUM_WAIT_OBJECTS, win32_numChildren - offset);
!
! ret = WaitForMultipleObjects(num, &win32_childHNDArray[offset], FALSE, 0);
! switch (ret)
! {
! case WAIT_FAILED:
! ereport(LOG,
! (errmsg_internal("failed to wait on %lu of %lu children: error code %d",
! num, win32_numChildren, (int) GetLastError())));
! return -1;
!
! case WAIT_TIMEOUT:
! /* No children (in this chunk) have finished */
! break;
!
! default:
!
! /*
! * Get the exit code, and return the PID of, the respective
! * process
! */
! index = offset + ret - WAIT_OBJECT_0;
! Assert(index >= 0 && index < win32_numChildren);
! if (!GetExitCodeProcess(win32_childHNDArray[index], &exitCode))
! {
! /*
! * If we get this far, this should never happen, but, then
! * again... No choice other than to assume a catastrophic
! * failure.
! */
! ereport(FATAL,
! (errmsg_internal("failed to get exit code for child %lu",
! (unsigned long) win32_childPIDArray[index])));
! }
! *exitstatus = (int) exitCode;
! return win32_childPIDArray[index];
! }
}
- /* No children have finished */
return -1;
}
/*
! * Note! Code below executes on separate threads, one for
! * each child process created
*/
! static DWORD WINAPI
! win32_sigchld_waiter(LPVOID param)
{
! HANDLE procHandle = (HANDLE) param;
! DWORD r = WaitForSingleObject(procHandle, INFINITE);
! if (r == WAIT_OBJECT_0)
! pg_queue_signal(SIGCHLD);
! else
! write_stderr("could not wait on child process handle: error code %d\n",
! (int) GetLastError());
! CloseHandle(procHandle);
! return 0;
}
#endif /* WIN32 */
--- 4483,4542 ----
#ifdef WIN32
static pid_t
win32_waitpid(int *exitstatus)
{
+ DWORD dwd;
+ ULONG_PTR key;
+ OVERLAPPED* ovl;
+
/*
! * Check if there are any dead children. If there are, return the pid of the first one that died.
*/
! if (GetQueuedCompletionStatus(win32ChildQueue,&dwd,&key,&ovl,0))
{
! *exitstatus = (int)key;
! return dwd;
}
return -1;
}
/*
! * Note! Code below executes on a thread pool! All operations must
! * be thread safe! Note that elog() and friends must *not* be used.
*/
! static void WINAPI
! pgwin32_deadchild_callback(PVOID lpParameter, BOOLEAN TimerOrWaitFired)
{
! win32_deadchild_waitinfo *childinfo = (win32_deadchild_waitinfo *)lpParameter;
! DWORD exitcode;
! if (TimerOrWaitFired)
! return; /* timeout. Should never happen, since we use INFINITE as timeout value. */
! /* Remove handle from wait - required even though it's set to wait only once */
! UnregisterWaitEx(childinfo->waitHandle, NULL);
!
! if (!GetExitCodeProcess(childinfo->procHandle, &exitcode))
! {
! /*
! * Should never happen. Inform user and set a fixed exitcode.
! */
! write_stderr("could not read exitcode for process\n");
! exitcode = 255;
! }
!
! if (!PostQueuedCompletionStatus(win32ChildQueue,childinfo->procId,(ULONG_PTR)exitcode,NULL))
! write_stderr("could not post child completion status\n");
!
! /* Handle is per-process, so we close it here instead of in the originating thread */
! CloseHandle(childinfo->procHandle);
!
! free(childinfo);
!
! /* Queue SIGCHLD signal */
! pg_queue_signal(SIGCHLD);
}
#endif /* WIN32 */
Index: src/include/port/win32.h
===================================================================
RCS file: /projects/cvsroot/pgsql/src/include/port/win32.h,v
retrieving revision 1.76
diff -c -r1.76 win32.h
*** src/include/port/win32.h 25 Jul 2007 12:22:53 -0000 1.76
--- src/include/port/win32.h 26 Oct 2007 11:16:48 -0000
***************
*** 4,9 ****
--- 4,10 ----
#define WIN32_ONLY_COMPILER
#endif
+ #define _WIN32_WINNT 0x0500
/*
* Always build with SSPI support. Keep it as a #define in case
* we want a switch to disable it sometime in the future.
Magnus Hagander wrote: > Dave Page wrote: >> Magnus Hagander wrote: >>> Right. You need to look at VM size in *process explorer*. VM size in >>> task manager has nothing to do with VM size, it's the private bytes :-S >>> And there is no way to see that info from task manager, I think. PE is >>> your friend. >>> >>> >>> Anyway. Other than a refresher on those, I'd be interested in two other >>> important parts: >>> * How many threads does it reach when you have 300 active backends? >>> * Is there a handle leak? meaning once your 300 backends have exited, >>> does the number of handles in the process drop down to the same value it >>> had before? >> Without patch: >> >> VM: 1,322,792K >> Idle threads: 6 >> Peak threads: 306 >> Handles at start: 576 >> Handles at end: 576 >> >> With patch: >> >> VM: 98,088K >> Idle threads: 3 >> Peak threads: 7 >> Handles at start: 576 >> Handles at end: 585 (585 again after second run). > > Ah, now we're talking. That's the kind of reduction I was looking for :-) > > I think the difference in handles is because the threadpool keeps some > things around. As long as it stays at 585 and comes back down after a > second run, we're fine at that - there's no leak. > > Attached is an updated version of the patch, currently being tested by > both me and Dave. If it passes our tests, I'll apply this so it gets > included for broader testing in beta2. So of course, for all good patches, problems turn up :-( This patch doesn't work on mingw, at least not on all versions. The reason is that, as usual, the mingw libraries are not complete. We've dealt with this before, by dynamically loading the functions. We know this works. But I don't have time to fix that tonight, and I'll be offline much of this weekend. Now, given these great improvements, I'd very much like this in beta2, so it can get proper testing. This leaves us with a couple of choices: 1) Delay beta2 until the beginning of next week. I'll get this fixed sunday evening or monday at the latest. I know how to fix it, it's just that I don't have the time right now :( (This assumes that the plan still is to wrap beta2 today) 2) Apply the patch as-is. Then beta2 will work fine with the msvc build, which is used for the binary distribution. But it's broken on mingw until fixed, which of course includes the buildfarm stuff. Again, we know how to fix this. 2b) I apply this as-is, and someone else cleans up mingw before beta2 is wrapped. 3) We don't apply this, and wait until beta3 to have it tested. Depending on how many betas we end up with, that may leave us with very little testing before release. 2b is of course the best here, but then someone has to step up and volunteer to do that. I'm leaning towards applying the patch now, and hoping for 2b to happen. If it doesn't happen, the choice between 1 and 2 can be made when the time to wrap the beta approaches (at which point I will be offline, so I escape :-P) The patch that would go in is the one previously posted plus a couple of minor edits in comments as suggested on IM by Alvaro. Comments? //Magnus
Magnus Hagander wrote: > I'm leaning towards applying the patch now, and hoping for 2b to happen. I think we should live with the mingw BF breakage for a day or two. The patch is clearly an important improvement, but it should be as widely tested as possible. /D
On Fri, 26 Oct 2007 21:58:03 +0100
Dave Page <dpage@postgresql.org> wrote:
> Magnus Hagander wrote:
> > I'm leaning towards applying the patch now, and hoping for 2b to
> > happen.
>
> I think we should live with the mingw BF breakage for a day or two.
> The patch is clearly an important improvement, but it should be as
> widely tested as possible.
I think this makes sense. I certainly don't want to hold back Beta2 and
this patch so far is an obvious improvement.
Sincerely,
Joshua D. Drake
>
> /D
>
> ---------------------------(end of
> broadcast)--------------------------- TIP 2: Don't 'kill -9' the
> postmaster
>
--
=== The PostgreSQL Company: Command Prompt, Inc. ===
Sales/Support: +1.503.667.4564 24x7/Emergency: +1.800.492.2240
PostgreSQL solutions since 1997 http://www.commandprompt.com/ UNIQUE NOT NULL
Donate to the PostgreSQL Project: http://www.postgresql.org/about/donate
PostgreSQL Replication: http://www.commandprompt.com/products/
Dave Page <dpage@postgresql.org> writes:
> Magnus Hagander wrote:
>> I'm leaning towards applying the patch now, and hoping for 2b to happen.
> I think we should live with the mingw BF breakage for a day or two. The
> patch is clearly an important improvement, but it should be as widely
> tested as possible.
If Dave is intending to build the Windows installer as soon as beta2 is
wrapped, then I agree with this plan. But my understanding was we'd
missed his window for today and so that wouldn't happen till Monday.
Assuming that's true, the idea Bruce and I had discussed on the phone
was:
1. Wrap official beta2 tonight, so that other packagers can work from it
over the weekend;
2. Magnus fixes his patch Sunday and applies it then;
3. Dave builds the Windows installer Monday *from CVS tip*.
So the Windows version would be beta2-plus-a-little but we'd neither
hold up work on other platforms nor break anything in buildfarm.
Just an alternative thought. In any case I agree that we want
Windows testing of beta2 to include this patch.
regards, tom lane
Tom Lane wrote: > Dave Page <dpage@postgresql.org> writes: >> Magnus Hagander wrote: >>> I'm leaning towards applying the patch now, and hoping for 2b to happen. > >> I think we should live with the mingw BF breakage for a day or two. The >> patch is clearly an important improvement, but it should be as widely >> tested as possible. > > If Dave is intending to build the Windows installer as soon as beta2 is > wrapped, then I agree with this plan. But my understanding was we'd > missed his window for today and so that wouldn't happen till Monday. > Assuming that's true, the idea Bruce and I had discussed on the phone > was: > > 1. Wrap official beta2 tonight, so that other packagers can work from it > over the weekend; > > 2. Magnus fixes his patch Sunday and applies it then; > > 3. Dave builds the Windows installer Monday *from CVS tip*. > > So the Windows version would be beta2-plus-a-little but we'd neither > hold up work on other platforms nor break anything in buildfarm. > > Just an alternative thought. In any case I agree that we want > Windows testing of beta2 to include this patch. If we do that, then what we label as beta2 in the installer is *not* the same as someone who has built beta2 from source. Can't say I like that one, and I know Dave doesn't like it (he said so before going offline). I'd rather see msvc working and mingw broken for beta2 really, because then we *know* that if someone says they're doing beta2 on mingw they're misinformed... //Magnus
Magnus Hagander <magnus@hagander.net> writes:
> Attached is an updated version of the patch, currently being tested by
> both me and Dave. If it passes our tests, I'll apply this so it gets
> included for broader testing in beta2.
One question: what's this about?
> + #define _WIN32_WINNT 0x0500
This looks like it might be tightening our assumptions about which
Windows flavors we can run on. I'm not necessarily against that,
but it should be publicly discussed if it's happening.
regards, tom lane
Tom Lane wrote: > Magnus Hagander <magnus@hagander.net> writes: >> Attached is an updated version of the patch, currently being tested by >> both me and Dave. If it passes our tests, I'll apply this so it gets >> included for broader testing in beta2. > > One question: what's this about? > >> + #define _WIN32_WINNT 0x0500 > > This looks like it might be tightening our assumptions about which > Windows flavors we can run on. I'm not necessarily against that, > but it should be publicly discussed if it's happening. It enables Windows 2000-specific headers. We already require Windows 2000 to run, so it doesn't restrict us anymore than we already are. It just exposes those parts of the header files. //Magnus
Joshua D. Drake wrote: > On Fri, 26 Oct 2007 21:58:03 +0100 > Dave Page <dpage@postgresql.org> wrote: > >> Magnus Hagander wrote: >>> I'm leaning towards applying the patch now, and hoping for 2b to >>> happen. >> I think we should live with the mingw BF breakage for a day or two. >> The patch is clearly an important improvement, but it should be as >> widely tested as possible. > > I think this makes sense. I certainly don't want to hold back Beta2 and > this patch so far is an obvious improvement. I read the consensus of this thread as "apply the patch as-is and let's fix mingw as soon as we can", so this is what I've done. If I got it wrong, feel free to back out :-) //Magnus