Re: Latch implementation that wakes on postmaster death on both win32 and Unix

On Mon, Jun 20, 2011 at 1:00 AM, Peter Geoghegan <peter@2ndquadrant.com> wrote:
> I took another look at this this evening, and realised that my
> comments could be a little clearer.
>
> Attached revision cleans them up a bit.

Since I'm not familiar with Windows, I haven't read the code related
to Windows. But
the followings are my comments on your patch.

+		if (wakeEvents & WL_POSTMASTER_DEATH)
+		{
+			FD_SET(postmaster_alive_fds[POSTMASTER_FD_WATCH], &input_mask);
+			if (postmaster_alive_fds[POSTMASTER_FD_WATCH] > hifd)
+				hifd = postmaster_alive_fds[POSTMASTER_FD_WATCH];
+		}
 		hifd = selfpipe_readfd;

'hifd' should be initialized to 'selfpipe_readfd' before the above
'if' block. Otherwise,
'hifd = postmaster_alive_fds[POSTMASTER_FD_WATCH]' might have no effect.

+			time_t		 curtime = time(NULL);
+			unsigned int timeout_secs  = (unsigned int) PGARCH_AUTOWAKE_INTERVAL -
+					(unsigned int) (curtime - last_copy_time);
+			WaitLatch(&mainloop_latch, WL_LATCH_SET | WL_TIMEOUT |
WL_POSTMASTER_DEATH, timeout_secs * 1000000L);

Why does the archive still need to wake up periodically?

+	flags |= FNONBLOCK;
+	if (fcntl(postmaster_alive_fds[POSTMASTER_FD_WATCH], F_SETFL, FNONBLOCK))

Is the variable 'flag' really required? It's not used by fcntl() to
set the fd nonblocking.

Is FNONBLOCK equal to O_NONBLOCK? If yes, we should use O_NONBLOCK
for the sake of consistency? In other code (e.g., noblock.c), O_NONBLOCK is used
rather than FNONBLOCK.

+			WaitLatchOrSocket(&MyWalSnd->latch,
+							  WL_LATCH_SET | WL_SOCKET_READABLE | (pq_is_send_pending()?
WL_SOCKET_WRITEABLE:0) |  WL_TIMEOUT,
+							  MyProcPort->sock,

I think that it's worth that walsender checks the postmaster death event. No?

Regards,

-- 
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center