On Tue, Jun 21, 2011 at 6:22 PM, Peter Geoghegan <pe...@2ndquadrant.com> wrote:
> Thanks for giving this your attention Fujii. Attached patch addresses
> your concerns.
Thanks for updating the patch! I have a few comments;
+WaitLatch(volatile Latch *latch, int wakeEvents, long timeout)
+WaitLatchOrSocket(volatile Latch *latch, int wakeEvents, pgsocket
sock, long timeout)
If 'wakeEvent' is zero, we cannot get out of WaitLatch(). Something like
Assert(waitEvents != 0) is required? Or, WaitLatch() should always wait
on latch even when 'waitEvents' is zero?
In unix_latch.c, select() in WaitLatchOrSocket() checks the timeout only when
WL_TIMEOUT is set in 'wakeEvents'. OTOH, in win32_latch.c,
WaitForMultipleObjects()
in WaitLatchOrSocket() always checks the timeout even if WL_TIMEOUT is not
given. Is this intentional?
+ else if (rc == WAIT_OBJECT_0 + 2 &&
+ ((wakeEvents & WL_SOCKET_READABLE) ||
(wakeEvents & WL_SOCKET_WRITEABLE)))
Another corner case: when WL_POSTMASTER_DEATH and WL_SOCKET_READABLE
are given and 'sock' is set to PGINVALID_SOCKET, we can wrongly pass through the
above check. If this OK?
rc = WaitForMultipleObjects(numevents, events, FALSE,
(timeout >= 0) ?
(timeout / 1000) : INFINITE);
- if (rc == WAIT_FAILED)
+ if ( (wakeEvents & WL_POSTMASTER_DEATH) &&
+ !PostmasterIsAlive(true))
After WaitForMultipleObjects() detects the death of postmaster,
WaitForSingleObject()
is called in PostmasterIsAlive(). In this case, what code does
WaitForSingleObject() return?
I wonder if WaitForSingleObject() returns the code other than
WAIT_TIMEOUT and really
can detect the death of postmaster.
+ if (fcntl(postmaster_alive_fds[POSTMASTER_FD_WATCH], F_GETFL) < 0)
+ {
+ ereport(FATAL,
+ (errcode_for_socket_access(),
+ errmsg("failed to set the postmaster death watching
fd's flags:
%s", strerror(errno))));
+ }
Is the above check really required? It's harmless, but looks unnecessary.
+ errmsg( "pipe() call failed to create pipe to monitor
postmaster
death: %s", strerror(errno))));
+ errmsg("failed to set the postmaster death watching
fd's flags:
%s", strerror(errno))));
+ errmsg("failed to set the postmaster death watching
fd's flags:
%s", strerror(errno))));
'%m' should be used instead of '%s' and 'strerror(errno)'.
Regards,
--
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
