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

I'm a bit disappointed that no one has commented on this yet. I would
have appreciated some preliminary feedback.

Anyway, I've added it to CommitFest 2011-06.

--
Peter Geoghegan       http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training and Services

On 24.05.2011 23:43, Peter Geoghegan wrote:

Attached is the latest revision of the latch implementation that
monitors postmaster death, plus the archiver client that now relies on
that new functionality and thereby works well without a tight
PostmasterIsAlive() polling loop.

The Unix-stuff looks good to me at a first glance.

The lifesign terminology has been dropped. We now close() the file
descriptor that represents "ownership" - the write end of our
anonymous pipe - in each child backend directly in the forking
machinery (the thin fork() wrapper for the non-EXEC_BACKEND case),
through a call to ReleasePostmasterDeathWatchHandle(). We don't have
to do that on Windows, and we don't.

There's one reference left to "life sign" in comments. (FWIW, I don't
have a problem with that terminology myself)

Disappointingly, and despite a big effort, there doesn't seem to be a
way to have the win32 WaitForMultipleObjects() call wake on postmaster
death in addition to everything else in the same way that select()
does, so there are now two blocking calls, each in a thread of its own
(when the latch code is interested in postmaster death - otherwise,
it's single threaded as before).

The threading stuff (in particular, the fact that we used a named pipe
in a thread where the name of the pipe comes from the process PID) is
inspired by win32 signal emulation, src/backend/port/win32/signal.c .

That's a pity, all those threads and named pipes are a bit gross for a
safety mechanism like this.

Looking at the MSDN docs again, can't you simply include
PostmasterHandle in the WaitForMultipleObjects() call to have it return
when the process dies? It should be possible to mix different kind of
handles in one call, including process handles. Does it not work as
advertised?

You can easily observe that it works as advertised on Windows by
starting Postgres with archiving, using task manager to monitor
processes, and doing the following to the postmaster (assuming it has
a PID of 1234). This is the Windows equivalent of kill -9 :

C:\Users\Peter>taskkill /pid 1234 /F

You'll see that it takes about a second for the archiver to exit. All
processes exit.

Hmm, shouldn't the archiver exit almost instantaneously now that there's
no polling anymore?

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

On 26 May 2011 11:22, Heikki Linnakangas
<heikki.linnakangas@enterprisedb.com> wrote:

The Unix-stuff looks good to me at a first glance.

Good.

There's one reference left to "life sign" in comments. (FWIW, I don't have a
problem with that terminology myself)

Should have caught that one. Removed.

Looking at the MSDN docs again, can't you simply include PostmasterHandle in
the WaitForMultipleObjects() call to have it return when the process dies?
It should be possible to mix different kind of handles in one call,
including process handles. Does it not work as advertised?

Uh, I might have done that, had I been aware of PostmasterHandle. I
tried various convoluted ways to make it do what ReadFile() did for
me, before finally biting the bullet and just using ReadFile() in a
separate thread. I've tried adding PostmasterHandle though, and it
works well - it appears to behave exactly the same as my original
implementation.

This simplifies things considerably. Now, on win32, things are
actually simpler than on Unix.

You'll see that it takes about a second for the archiver to exit. All
processes exit.

Hmm, shouldn't the archiver exit almost instantaneously now that there's no
polling anymore?

Actually, just one "lagger" process sometimes remains that takes maybe
as long as a second, a bit longer than the others. I assumed that it
was the archiver, but I was probably wrong. I also didn't see that
very consistently.

Attached revision doesn't use any threads or pipes on win32. It's far
neater there. I'm still seeing that "lagger" process (which is an
overstatement) at times, so I guess it is normal. On Windows, there is
no detailed PS output, so I actually don't know what the lagger
process is, and no easy way to determine that immediately occurs to
me.

--
Peter Geoghegan       http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training and Services

On Thu, May 26, 2011 at 11:58 AM, Peter Geoghegan <peter@2ndquadrant.com> wrote:

Attached revision doesn't use any threads or pipes on win32. It's far
neater there. I'm still seeing that "lagger" process (which is an
overstatement) at times, so I guess it is normal. On Windows, there is
no detailed PS output, so I actually don't know what the lagger
process is, and no easy way to determine that immediately occurs to
me.

Process Explorer might help you there:
http://technet.microsoft.com/en-us/sysinternals/bb896653

--
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

I had another quick look-over this patch, and realised that I made a
minor mistake:

+void
+ReleasePostmasterDeathWatchHandle(void)
+{
+	/* MyProcPid won't have been set yet */
+	Assert(PostmasterPid != getpid());
+	/* Please don't ask twice */
+	Assert(postmaster_alive_fds[POSTMASTER_FD_OWN] != -1);
+	/* Release parent's ownership fd - only postmaster should hold it */
+	if (close(postmaster_alive_fds[ POSTMASTER_FD_OWN]))
+	{
+		ereport(FATAL,
+			(errcode_for_socket_access(),
+			 errmsg("Failed to close file descriptor associated with
Postmaster death in child process %d", MyProcPid)));
+	}
+	postmaster_alive_fds[POSTMASTER_FD_OWN] = -1;
+}
+

MyProcPid is used in this errmsg, and as noted in the first comment,
it isn't expected to be initialised when
ReleasePostmasterDeathWatchHandle() is called. Therefore, MyProcPid
should be replaced with a call to getpid(), just as it is for
Assert(PostmasterPid != getpid()).

I suppose that you could take the view that MyProcPid ought to be
initialised before the function is called, but I thought this was the
least worst way. Better to do it this way than to touch all the
different ways in which MyProcPid might be initialised, I suspect.

--
Peter Geoghegan       http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training and Services

On 16.06.2011 15:07, Peter Geoghegan wrote:

I had another quick look-over this patch, and realised that I made a
minor mistake:

+void
+ReleasePostmasterDeathWatchHandle(void)
+{
+	/* MyProcPid won't have been set yet */
+	Assert(PostmasterPid != getpid());
+	/* Please don't ask twice */
+	Assert(postmaster_alive_fds[POSTMASTER_FD_OWN] != -1);
+	/* Release parent's ownership fd - only postmaster should hold it */
+	if (close(postmaster_alive_fds[ POSTMASTER_FD_OWN]))
+	{
+		ereport(FATAL,
+			(errcode_for_socket_access(),
+			 errmsg("Failed to close file descriptor associated with
Postmaster death in child process %d", MyProcPid)));
+	}
+	postmaster_alive_fds[POSTMASTER_FD_OWN] = -1;
+}
+

MyProcPid is used in this errmsg, and as noted in the first comment,
it isn't expected to be initialised when
ReleasePostmasterDeathWatchHandle() is called. Therefore, MyProcPid
should be replaced with a call to getpid(), just as it is for
Assert(PostmasterPid != getpid()).

I suppose that you could take the view that MyProcPid ought to be
initialised before the function is called, but I thought this was the
least worst way. Better to do it this way than to touch all the
different ways in which MyProcPid might be initialised, I suspect.

Hmm, I'm not sure having the pid in that error message is too useful in
the first place. The process was just spawned, and it will die at that
error. When you try to debug that sort of error, what you would compare
the pid with? And you can include the pid in log_line_prefix if it turns
out to be useful after all.

PS. error messages should begin with lower-case letter.

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

On 16 June 2011 13:15, Heikki Linnakangas
<heikki.linnakangas@enterprisedb.com> wrote:

Hmm, I'm not sure having the pid in that error message is too useful in the
first place. The process was just spawned, and it will die at that error.
When you try to debug that sort of error, what you would compare the pid
with? And you can include the pid in log_line_prefix if it turns out to be
useful after all.

All fair points. FWIW, I think it's pretty unlikely that anyone will
ever see this error message.

--
Peter Geoghegan       http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training and Services

Peter Geoghegan wrote:

--- 247,277 ----
* do that), and the select() will return immediately.
*/
drainSelfPipe();
! 		if (latch->is_set && (wakeEvents & WL_LATCH_SET))
!  		{
! 			result |= WL_LATCH_SET;
! 			found = true;
! 			/*
! 			 * Leave loop immediately, avoid blocking again.
! 			 * Since latch is set, no other factor could have
! 			 * coincided that could make us wake up
! 			 * independently of the latch being set, so no
! 			 * need to worry about having missed something.
! 			 */
break;
}

I don't understand that comment. Why can't e.g postmaster death happen
at the same time as a latch is set? I think the code is fine as it is,
we just need to document that if there are several events that would
wake up WaitLatch(), we make no promise that we return all of them in
the return value. I believe all the callers would be fine with that.

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

On 16 June 2011 15:27, Heikki Linnakangas
<heikki.linnakangas@enterprisedb.com> wrote:

I don't understand that comment. Why can't e.g postmaster death happen at
the same time as a latch is set? I think the code is fine as it is, we just
need to document that if there are several events that would wake up
WaitLatch(), we make no promise that we return all of them in the return
value. I believe all the callers would be fine with that.

I see your perspective...there is a window for the Postmaster to die
after the latch is set, but before it returns control to clients, and
this won't be reported. I would argue that Postmaster death didn't
actually coincide with the latch being set, because it didn't actually
cause the select() to unblock, and therefore we don't have a
responsibility to report it. Even if that view doesn't stand up to
scrutiny, and it may not, it is a fairly academic point, because, as
you say, it's unlikely that clients will ever much care. I'd be happy
to document that we make no promises, on the off chance that some
future caller might care.

--
Peter Geoghegan       http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training and Services

On May26, 2011, at 11:25 , Peter Geoghegan wrote:

I'm a bit disappointed that no one has commented on this yet. I would
have appreciated some preliminary feedback.

I noticed to your patch doesn't seem to register a SIGIO handler, i.e.
it doesn't use async IO machinery (or rather a tiny part thereof) to
get asynchronously notified if the postmaster dies.

If that is on purpose, you can remove the fsetown() call, as it serves
no purpose without such a handler I think. Or, you might want to add
such a signal handler, and make it simply do "kill(getpid(), SIGTERM)".

best regards,
Florian Pflug

This patch breaks silent_mode=on. In silent_mode, postmaster forks early
on, to detach from the controlling tty. It uses fork_process() for that,
which with patch closes the write end of the postmaster-alive pipe, but
that's wrong because the child becomes the postmaster process.

On a stylistic note, the "extern" declaration in unix_latch.c is ugly,
extern declarations should be in header files. Come to think of it, I
feel the Init- and ReleasePostmasterDeathWatchHandle() functions should
go to postmaster.c. postmaster_alive_fds[] and PostmasterHandle serve
the same purpose, declaration and initialization of both should be kept
together, perhaps by moving the initialization of PostmasterHandle into
Init- and ReleasePostmasterDeathWatchHandle().

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

Excerpts from Peter Geoghegan's message of jue jun 16 08:42:39 -0400 2011:

On 16 June 2011 13:15, Heikki Linnakangas
<heikki.linnakangas@enterprisedb.com> wrote:

Hmm, I'm not sure having the pid in that error message is too useful in the
first place. The process was just spawned, and it will die at that error.
When you try to debug that sort of error, what you would compare the pid
with? And you can include the pid in log_line_prefix if it turns out to be
useful after all.

All fair points. FWIW, I think it's pretty unlikely that anyone will
ever see this error message.

... in which case the getpid() call is not that expensive anyway.

I agree that the PID should be part of the log_line_prefix though, which
is why I was trying to propose we include it (or the session ID) in the
default log_line_prefix along with a suitable timestamp.

--
Álvaro Herrera <alvherre@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

On 16 June 2011 16:30, Heikki Linnakangas
<heikki.linnakangas@enterprisedb.com> wrote:

This patch breaks silent_mode=on. In silent_mode, postmaster forks early on,
to detach from the controlling tty. It uses fork_process() for that, which
with patch closes the write end of the postmaster-alive pipe, but that's
wrong because the child becomes the postmaster process.

Attached patch revision addresses that issue. There is a thin
macro-based wrapper around fork_process(), depending on whether or not
it is desirable to ReleasePostmasterDeathWatchHandle() after forking.
All callers to fork_process() are unchanged.

On a stylistic note, the "extern" declaration in unix_latch.c is ugly,
extern declarations should be in header files.

Just an oversight.

Come to think of it, I feel
the Init- and ReleasePostmasterDeathWatchHandle() functions should go to
postmaster.c. postmaster_alive_fds[] and PostmasterHandle serve the same
purpose, declaration and initialization of both should be kept together,
perhaps by moving the initialization of PostmasterHandle into Init- and
ReleasePostmasterDeathWatchHandle().

I've removed the "no coinciding wakeEvents" comment that you objected
to (or clarified that other wakeEvents can coincide), and have
documented the fact that we make no guarantees about reporting all
events that caused a latch wake-up. We will report at least one
though.

I've moved Init- and ReleasePostmasterDeathWatchHandle() into postmaster.c .

I have to disagree with the idea of moving initialisation of
PostmasterHandle into InitPostmasterDeathWatchHandle(). Both Init-,
and Release- functions, which only exist on Unix builds, initialise
and subsequently release the watching handle. There's a symmetry to
it. If we created a win32 InitPostmasterDeathWatchHandle(), we'd have
no reason to create a win32 Release-, so the symmetry would be lost.
Also, PostmasterHandle does not exist for the express purpose of latch
clients monitoring postmaster death, unlike postmaster_alive_fds[] -
it existed before now. I guess I don't feel too strongly about it
though. It just doesn't seem like a maintainability win.

On 16 June 2011 15:49, Florian Pflug <fgp@phlo.org> wrote:

I noticed to your patch doesn't seem to register a SIGIO handler, i.e.
it doesn't use async IO machinery (or rather a tiny part thereof) to
get asynchronously notified if the postmaster dies.

If that is on purpose, you can remove the fsetown() call, as it serves
no purpose without such a handler I think. Or, you might want to add
such a signal handler, and make it simply do "kill(getpid(), SIGTERM)".

It is on purpose - I'm not interested in asynchronous notification for
the time being at least, because it doesn't occur to me how we can
handle that failure usefully in an asynchronous fashion. Anyway, that
code has been simplified, and my intent clarified. Thanks.

--
Peter Geoghegan       http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training and Services

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.

--
Peter Geoghegan       http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training and Services

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

Thanks for giving this your attention Fujii. Attached patch addresses
your concerns.

On 20 June 2011 05:53, Fujii Masao <masao.fujii@gmail.com> wrote:

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

That's an oversight that I should have caught. Fixed.

Why does the archive still need to wake up periodically?

That is consistent with its earlier behaviour..."she wakes up
occasionally to allow herself to be proactive". This comment does not
refer to the frequent updates that currently occur within the tight
polling loop. I think any concern about that would apply equally to
the original, unpatched code.

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

Yes, it's superfluous. Removed.

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.

FNONBLOCK is just an alias for O_NONBLOCK, so it seems reasonable to
be consistent in which variant we use. I have found suggestions that
it might break the build on OSX, so if that's true there's an
excellent reason to prefer the latter.

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

It does check it, but only in the same way that it always has (a tight
polling loop). I would like to make walsender use the new
functionality. That is another patch though, that I thought best to
have independently reviewed, only when this patch is committed. I've
only made the walsender use the new interface, changing as little as
possible and not affecting walsender's behaviour, as a stopgap towards
that patch.

--
Peter Geoghegan       http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training and Services

On Tue, Jun 21, 2011 at 6:22 PM, Peter Geoghegan <peter@2ndquadrant.com> wrote:

Thanks for giving this your attention Fujii. Attached patch addresses
your concerns.

Thanks for updating the patch! I have a few comments;

+WaitLatch(volatile Latch *latch, int wakeEvents, long timeout)
+WaitLatchOrSocket(volatile Latch *latch, int wakeEvents, pgsocket
sock, long timeout)

If 'wakeEvent' is zero, we cannot get out of WaitLatch(). Something like
Assert(waitEvents != 0) is required? Or, WaitLatch() should always wait
on latch even when 'waitEvents' is zero?

In unix_latch.c, select() in WaitLatchOrSocket() checks the timeout only when
WL_TIMEOUT is set in 'wakeEvents'. OTOH, in win32_latch.c,
WaitForMultipleObjects()
in WaitLatchOrSocket() always checks the timeout even if WL_TIMEOUT is not
given. Is this intentional?

+		else if (rc == WAIT_OBJECT_0 + 2 &&
+				 ((wakeEvents & WL_SOCKET_READABLE) || (wakeEvents & WL_SOCKET_WRITEABLE)))

Another corner case: when WL_POSTMASTER_DEATH and WL_SOCKET_READABLE
are given and 'sock' is set to PGINVALID_SOCKET, we can wrongly pass through the
above check. If this OK?

		rc = WaitForMultipleObjects(numevents, events, FALSE,
 							   (timeout >= 0) ? (timeout / 1000) : INFINITE);
-		if (rc == WAIT_FAILED)
+		if ( (wakeEvents & WL_POSTMASTER_DEATH) &&
+			 !PostmasterIsAlive(true))

After WaitForMultipleObjects() detects the death of postmaster,
WaitForSingleObject()
is called in PostmasterIsAlive(). In this case, what code does
WaitForSingleObject() return?
I wonder if WaitForSingleObject() returns the code other than
WAIT_TIMEOUT and really
can detect the death of postmaster.

+	if (fcntl(postmaster_alive_fds[POSTMASTER_FD_WATCH], F_GETFL) < 0)
+	{
+		ereport(FATAL,
+			(errcode_for_socket_access(),
+			 errmsg("failed to set the postmaster death watching fd's flags:
%s", strerror(errno))));
+	}

Is the above check really required? It's harmless, but looks unnecessary.

+			 errmsg( "pipe() call failed to create pipe to monitor postmaster
death: %s", strerror(errno))));
+			 errmsg("failed to set the postmaster death watching fd's flags:
%s", strerror(errno))));
+			 errmsg("failed to set the postmaster death watching fd's flags:
%s", strerror(errno))));

'%m' should be used instead of '%s' and 'strerror(errno)'.

Regards,

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

Attached patch addresses Fujii's more recent concerns.

On 22 June 2011 04:54, Fujii Masao <masao.fujii@gmail.com> wrote:

+WaitLatch(volatile Latch *latch, int wakeEvents, long timeout)
+WaitLatchOrSocket(volatile Latch *latch, int wakeEvents, pgsocket
sock, long timeout)

If 'wakeEvent' is zero, we cannot get out of WaitLatch(). Something like
Assert(waitEvents != 0) is required? Or, WaitLatch() should always wait
on latch even when 'waitEvents' is zero?

Well, not waking when the client has not specified an event to wake on
is the correct thing to do in that case. It would also be inherently
undesirable, so I'd be happy to guard against it using an assertion.
Both implementations now use one.

In unix_latch.c, select() in WaitLatchOrSocket() checks the timeout only when
WL_TIMEOUT is set in 'wakeEvents'. OTOH, in win32_latch.c,
WaitForMultipleObjects()
in WaitLatchOrSocket() always checks the timeout even if WL_TIMEOUT is not
given. Is this intentional?

No, it's a mistake. Fixed.

+               else if (rc == WAIT_OBJECT_0 + 2 &&
+                                ((wakeEvents & WL_SOCKET_READABLE) || (wakeEvents & WL_SOCKET_WRITEABLE)))

Another corner case: when WL_POSTMASTER_DEATH and WL_SOCKET_READABLE
are given and 'sock' is set to PGINVALID_SOCKET, we can wrongly pass through the
above check. If this OK?

I see your point - Assert(sock != PGINVALID_SOCKET) could be violated.
We fix the issue now by setting and checking a bool that simply
indicates that we're interested in sockets.

               rc = WaitForMultipleObjects(numevents, events, FALSE,
                                                          (timeout >= 0) ? (timeout / 1000) : INFINITE);
-               if (rc == WAIT_FAILED)
+               if ( (wakeEvents & WL_POSTMASTER_DEATH) &&
+                        !PostmasterIsAlive(true))

After WaitForMultipleObjects() detects the death of postmaster,
WaitForSingleObject()
is called in PostmasterIsAlive(). In this case, what code does
WaitForSingleObject() return?
I wonder if WaitForSingleObject() returns the code other than
WAIT_TIMEOUT and really
can detect the death of postmaster.

As noted up-thread, the fact that the archiver does wake and finish on
Postmaster death can be clearly observed on Windows. I'm not sure why
you wonder that, as this is fairly standard use of
PostmasterIsAlive(). I've verified that the waitLatch() call
correctly reports Postmaster death in its return code on Windows, and
indeed that it actually wakes up.

Are you suggesting that there should be a defensive else if{ } for the
case where PostmasterIsAlive() incorrectly reports that the PM is
alive due to some implementation related race-condition, and we've
already considered every other possibility? Well, I suppose that's not
necessary, because we will loop until we find a reason - it's okay to
miss it the first time around, because whatever caused
WaitForMultipleObjects() to wake up will cause it to immediately
return for the next iteration.

In any case, we don't rely on the PostmasterIsAlive() call at all
anymore, so it doesn't matter. We just look at rc's value now, as we
do for every other case, though it's a bit trickier when checking
Postmaster death. Similarly, we don't have a PostmasterIsAlive() call
within the unix latch implementation anymore.

+       if (fcntl(postmaster_alive_fds[POSTMASTER_FD_WATCH], F_GETFL) < 0)
+       {
+               ereport(FATAL,
+                       (errcode_for_socket_access(),
+                        errmsg("failed to set the postmaster death watching fd's flags:
%s", strerror(errno))));
+       }

Is the above check really required? It's harmless, but looks unnecessary.

Yes, it's not possible for it to detect an error condition now. Removed.

'%m' should be used instead of '%s' and 'strerror(errno)'.

It is of course better to use the simpler, built-in facility. Fixed.

--
Peter Geoghegan       http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training and Services

On Wed, Jun 22, 2011 at 9:11 PM, Peter Geoghegan <peter@2ndquadrant.com> wrote:

               rc = WaitForMultipleObjects(numevents, events, FALSE,
                                                          (timeout >= 0) ? (timeout / 1000) : INFINITE);
-               if (rc == WAIT_FAILED)
+               if ( (wakeEvents & WL_POSTMASTER_DEATH) &&
+                        !PostmasterIsAlive(true))

After WaitForMultipleObjects() detects the death of postmaster,
WaitForSingleObject()
is called in PostmasterIsAlive(). In this case, what code does
WaitForSingleObject() return?
I wonder if WaitForSingleObject() returns the code other than
WAIT_TIMEOUT and really
can detect the death of postmaster.

As noted up-thread, the fact that the archiver does wake and finish on
Postmaster death can be clearly observed on Windows. I'm not sure why
you wonder that, as this is fairly standard use of
PostmasterIsAlive().

Because, if PostmasterHandle is an auto-reset event object, its event state
would be automatically reset just after WaitForMultipleObjects() detects
the postmaster death event, I was afraid. But your observation proved that
my concern was not right.

I have another comments:

+#ifndef WIN32
+	/*
+	 * Initialise mechanism that allows waiting latch clients
+	 * to wake on postmaster death, to finish their
+	 * remaining business
+	 */
+	InitPostmasterDeathWatchHandle();
+#endif

Calling this function before creating TopMemoryContext looks unsafe. What if
the function calls ereport(FATAL)?

That ereport() can be called before postgresql.conf is read, i.e., before GUCs
for error reporting are set. Is this OK? If not,
InitPostmasterDeathWatchHandle()
should be moved after SelectConfigFiles().

+#ifndef WIN32
+int postmaster_alive_fds[2];
+#endif

postmaster_alive_fds[] should be initialized to "{-1, -1}"?

Regards,

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