Re: Latch implementation that wakes on postmaster death on both win32 and Unix - Mailing list pgsql-hackers

On Tue, Jun 21, 2011 at 6:22 PM, Peter Geoghegan <peter@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