Skip site navigation (1) Skip section navigation (2)

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

From: Fujii Masao <masao(dot)fujii(at)gmail(dot)com>
To: Peter Geoghegan <peter(at)2ndquadrant(dot)com>
Cc: Heikki Linnakangas <heikki(dot)linnakangas(at)enterprisedb(dot)com>, PG Hackers <pgsql-hackers(at)postgresql(dot)org>, Florian Pflug <fgp(at)phlo(dot)org>
Subject: Re: Latch implementation that wakes on postmaster death on both win32 and Unix
Date: 2011-06-30 06:36:17
Message-ID: BANLkTinr8KecEkTug4tOaEAsfR1z0wA64A@mail.gmail.com (view raw or flat)
Thread:
Lists: pgsql-hackers
On Sat, Jun 25, 2011 at 10:41 AM, Peter Geoghegan <peter(at)2ndquadrant(dot)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

In response to

Responses

pgsql-hackers by date

Next:From: Jun IshidukaDate: 2011-06-30 06:38:45
Subject: Re: Online base backup from the hot-standby
Previous:From: Pavel StehuleDate: 2011-06-30 06:36:01
Subject: ToDo: list of active channels

Privacy Policy | About PostgreSQL
Copyright © 1996-2014 The PostgreSQL Global Development Group