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
