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

On Sat, Jun 25, 2011 at 10:41 AM, Peter Geoghegan <peter@2ndquadrant.com> wrote:
> Attached is patch that addresses Fujii's third and most recent set of concerns.

Thanks for updating the patch!

> I think that Heikki is currently taking another look at my work,
> because he indicates in a new message to the list a short time ago
> that while reviewing my patch, he realised that there may be an
> independent problem with silent_mode. I will wait for his remarks
> before producing another version of the patch that incorporates those
> two small changes.

Yes, we should wait for the comments from Heikki. But, I have another
comments;

InitPostmasterDeathWatchHandle() can be static function because it's
used only in postmaster.c.

ReleasePostmasterDeathWatchHandle() can call ereport(FATAL) before
StartChildProcess() or BackendStartup() calls on_exit_reset() and resets
MyProcPid. This looks unsafe. If that ereport(FATAL) is unfortunately
called, a process other than postmaster would perform the postmaster's
proc-exit handlers. And that ereport(FATAL) would report wrong pid
when %p is specified in log_line_prefix. What about closing the pipe in
ClosePostmasterPorts() and removing ReleasePostmasterDeathWatchHandle()?

+    /*
+     * Set O_NONBLOCK to allow checking for the fd's presence with a select() call
+     */
+    if (fcntl(postmaster_alive_fds[POSTMASTER_FD_WATCH], F_SETFL, O_NONBLOCK))
+    {
+        ereport(FATAL,
+            (errcode_for_socket_access(),
+             errmsg("failed to set the postmaster death watching fd's flags: %m")));
+    }

I don't think that the pipe fd needs to be set to non-blocking mode
since we don't read or write on it.

http://developer.postgresql.org/pgdocs/postgres/error-style-guide.html
According to the error style guide, I think that it's better to change the
following messages:

+             errmsg( "pipe() call failed to create pipe to monitor postmaster
death: %m")));

"could not create pipe for monitoring postmaster death: %m"

+             errmsg("failed to close file descriptor associated with
postmaster death in child process")));

"could not close postmaster pipe: %m"

Regards,

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