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

On 30 June 2011 08:58, Heikki Linnakangas
<heikki(dot)linnakangas(at)enterprisedb(dot)com> wrote:

> Here's a WIP patch with some mostly cosmetic changes I've done this far. I
> haven't tested the Windows code at all yet. It seems that no-one is
> objecting to removing silent_mode altogether, so I'm going to do that before
> committing this patch.

I'm mostly happy with the changes you've made, but I note:

Fujii is of course correct in pointing out that
InitPostmasterDeathWatchHandle() should be a static function.

s/the implementation, as described in unix_latch.c/the implementation/
- This is my mistake. I see no reason to mention the .c file. Use
ctags.

Minor niggle, but there is a little errant whitespace at the top of
fork_process.c.

(wakeEvents & WL_TIMEOUT) != 0 -- I was going to note that this was
redundant, but then I remembered that stupid MSVC warning, which I
wouldn't have seen here because I didn't use it for my Windows build
due to an infuriating issue with winflex (Our own Cygwin built version
of flex for windows). I wouldn't have expected that when it was set to
build C though. I'm not sure exactly why it isn't necessary in other
places where we're (arguably) doing the same thing.

On 30 June 2011 07:36, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:

> 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()?

Hmm. That is a valid criticism. I'd rather move the
ReleasePostmasterDeathWatchHandle() call into ClosePostmasterPorts()
though.

> 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:

I don't think that the way I've phrased my error messages is
inconsistent with that style guide, excepty perhaps the pipe()
reference, but if you feel it's important to try and use "could not",
I have no objections.

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