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

On 16 June 2011 16:30, Heikki Linnakangas
<heikki(dot)linnakangas(at)enterprisedb(dot)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(at)phlo(dot)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