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(at)2ndquadrant(dot)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