FAQ
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.

On second thought, it is reasonable for the patch to be evaluated with
the archiver changes. Any problems that we'll have with latch changes
are likely problems that all WL_POSTMASTER_DEATH latch clients will
have, so we might as well include the simplest such client initially.
Once I have buy-in on the latch changes, the archiver work becomes
uncontroversial, I think.

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.

I've handled the non-win32 EXEC_BACKEND case, which I understand just
exists for testing purposes. I've done the usual BackendParameters
stuff.

A ReleasePostmasterDeathWatchHandle() call is unnecessary on win32
(the function doesn't exist there - the need to call it on Unix is a
result of its implementation). I'd like to avoid having calls to it in
each auxiliary process. It should be called in a single sweet spot
that doesn't put any burden on child process authors to remember to
call it themselves.

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 .

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.

Thoughts?

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

Search Discussions

  • Peter Geoghegan at May 26, 2011 at 9:40 am
    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
  • Florian Pflug at Jun 16, 2011 at 2:50 pm

    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
  • Heikki Linnakangas at May 26, 2011 at 10:22 am

    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
  • Peter Geoghegan at May 26, 2011 at 12:16 pm

    On 26 May 2011 11:22, Heikki Linnakangas 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
  • Dave Page at May 26, 2011 at 12:23 pm

    On Thu, May 26, 2011 at 11:58 AM, Peter Geoghegan 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
  • Peter Geoghegan at Jun 16, 2011 at 12:07 pm
    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
  • Heikki Linnakangas at Jun 16, 2011 at 12:15 pm

    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
  • Peter Geoghegan at Jun 16, 2011 at 12:42 pm

    On 16 June 2011 13:15, Heikki Linnakangas 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
  • Heikki Linnakangas at Jun 16, 2011 at 2:28 pm

    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
  • Peter Geoghegan at Jun 16, 2011 at 2:49 pm

    On 16 June 2011 15:27, Heikki Linnakangas 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
  • Alvaro Herrera at Jun 16, 2011 at 4:01 pm

    Excerpts from Peter Geoghegan's message of jue jun 16 08:42:39 -0400 2011:
    On 16 June 2011 13:15, Heikki Linnakangas
    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
  • Heikki Linnakangas at Jun 16, 2011 at 3:31 pm
    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
  • Peter Geoghegan at Jun 17, 2011 at 6:56 pm

    On 16 June 2011 16:30, Heikki Linnakangas 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 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
  • Peter Geoghegan at Jun 19, 2011 at 4:00 pm
    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
  • Fujii Masao at Jun 20, 2011 at 4:53 am

    On Mon, Jun 20, 2011 at 1:00 AM, Peter Geoghegan 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
  • Peter Geoghegan at Jun 21, 2011 at 9:22 am
    Thanks for giving this your attention Fujii. Attached patch addresses
    your concerns.
    On 20 June 2011 05:53, Fujii Masao 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
  • Fujii Masao at Jun 22, 2011 at 3:54 am

    On Tue, Jun 21, 2011 at 6:22 PM, Peter Geoghegan 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
  • Peter Geoghegan at Jun 22, 2011 at 12:11 pm
    Attached patch addresses Fujii's more recent concerns.
    On 22 June 2011 04:54, Fujii Masao 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
  • Fujii Masao at Jun 24, 2011 at 11:30 am

    On Wed, Jun 22, 2011 at 9:11 PM, Peter Geoghegan 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
  • Peter Geoghegan at Jun 24, 2011 at 12:08 pm

    On 24 June 2011 12:30, Fujii Masao wrote:
    +#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().
    I see no reason to take the risk that it might at some point - I've moved it.
    +#ifndef WIN32
    +int postmaster_alive_fds[2];
    +#endif

    postmaster_alive_fds[] should be initialized to "{-1, -1}"?
    Yes, they should. That works better.

    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.

    --
    Peter Geoghegan       http://www.2ndQuadrant.com/
    PostgreSQL Development, 24x7 Support, Training and Services
  • Peter Geoghegan at Jun 25, 2011 at 1:41 am
    Attached is patch that addresses Fujii's third and most recent set of concerns.

    --
    Peter Geoghegan       http://www.2ndQuadrant.com/
    PostgreSQL Development, 24x7 Support, Training and Services
  • Fujii Masao at Jun 30, 2011 at 6:36 am

    On Sat, Jun 25, 2011 at 10:41 AM, Peter Geoghegan 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
  • Heikki Linnakangas at Jun 30, 2011 at 7:58 am

    On 30.06.2011 09:36, Fujii Masao wrote:
    On Sat, Jun 25, 2011 at 10:41 AM, Peter Geogheganwrote:
    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;
    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.

    --
    Heikki Linnakangas
    EnterpriseDB http://www.enterprisedb.com
  • Peter Geoghegan at Jun 30, 2011 at 9:47 am

    On 30 June 2011 08:58, Heikki Linnakangas 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 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
  • Robert Haas at Jun 30, 2011 at 12:20 pm

    On Thu, Jun 30, 2011 at 5:47 AM, Peter Geoghegan wrote:
    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.
    I like Fujii's rephrasing - we don't usually mention the name of the
    system call.

    --
    Robert Haas
    EnterpriseDB: http://www.enterprisedb.com
    The Enterprise PostgreSQL Company
  • Heikki Linnakangas at Jul 4, 2011 at 4:00 pm
    Ok, here's a new patch, addressing the issues Fujii raised, and with a
    bunch of stylistic changes of my own. Also, I committed a patch to
    remove silent_mode, so the fork_process() changes are now gone. I'm
    going to sleep over this and review once again tomorrow, and commit if
    it still looks good to me and no-one else reports new issues.

    There's two small issues left:

    I don't like the names POSTMASTER_FD_WATCH and POSTMASTER_FD_OWN. At a
    quick glance, it's not at all clear which is which. I couldn't come up
    with better names, so for now I just added some comments to clarify
    that. I would find WRITE/READ more clear, but to make sense of that you
    need to how the pipe is used. Any suggestions or opinions on that?

    The BUGS section of Linux man page for select(2) says:
    Under Linux, select() may report a socket file descriptor as "ready for
    reading", while nevertheless a subsequent read blocks. This could for
    example happen when data has arrived but upon examination has wrong
    checksum and is discarded. There may be other circumstances in which a
    file descriptor is spuriously reported as ready. Thus it may be safer
    to use O_NONBLOCK on sockets that should not block.
    So in theory, on Linux you might WaitLatch might sometimes incorrectly
    return WL_POSTMASTER_DEATH. None of the callers check for
    WL_POSTMASTER_DEATH return code, they call PostmasterIsAlive() before
    assuming the postmaster has died, so that won't affect correctness at
    the moment. I doubt that scenario can even happen in our case, select()
    on a pipe that is never written to. But maybe we should add add an
    assertion to WaitLatch to assert that if select() reports that the
    postmaster pipe has been closed, PostmasterIsAlive() also returns false.

    --
    Heikki Linnakangas
    EnterpriseDB http://www.enterprisedb.com
  • Florian Pflug at Jul 4, 2011 at 4:36 pm

    On Jul4, 2011, at 17:53 , Heikki Linnakangas wrote:
    Under Linux, select() may report a socket file descriptor as "ready for
    reading", while nevertheless a subsequent read blocks. This could for
    example happen when data has arrived but upon examination has wrong
    checksum and is discarded. There may be other circumstances in which a
    file descriptor is spuriously reported as ready. Thus it may be safer
    to use O_NONBLOCK on sockets that should not block.
    So in theory, on Linux you might WaitLatch might sometimes incorrectly return WL_POSTMASTER_DEATH. None of the callers check for WL_POSTMASTER_DEATH return code, they call PostmasterIsAlive() before assuming the postmaster has died, so that won't affect correctness at the moment. I doubt that scenario can even happen in our case, select() on a pipe that is never written to. But maybe we should add add an assertion to WaitLatch to assert that if select() reports that the postmaster pipe has been closed, PostmasterIsAlive() also returns false.
    The correct solution would be to read() from the pipe after select()
    returns, and only return WL_POSTMASTER_DEATCH if the read doesn't return
    EAGAIN. To prevent that read() from blocking if the read event was indeed
    spurious, O_NONBLOCK must be set on the pipe but that patch does that already.

    Btw, with the death-watch / life-sign / whatever infrastructure in place,
    shouldn't PostmasterIsAlive() be using that instead of getppid() / kill(0)?

    best regards,
    Florian Pflug
  • Fujii Masao at Jul 5, 2011 at 2:59 am

    On Tue, Jul 5, 2011 at 1:36 AM, Florian Pflug wrote:
    On Jul4, 2011, at 17:53 , Heikki Linnakangas wrote:
    Under Linux, select() may report a socket file descriptor as "ready for
    reading",  while nevertheless a subsequent read blocks.  This could for
    example happen when data has arrived but  upon  examination  has  wrong
    checksum and is discarded.  There may be other circumstances in which a
    file descriptor is spuriously reported as ready.  Thus it may be  safer
    to use O_NONBLOCK on sockets that should not block.
    So in theory, on Linux you might WaitLatch might sometimes incorrectly return WL_POSTMASTER_DEATH. None of the callers check for WL_POSTMASTER_DEATH return code, they call PostmasterIsAlive() before assuming the postmaster has died, so that won't affect correctness at the moment. I doubt that scenario can even happen in our case, select() on a pipe that is never written to. But maybe we should add add an assertion to WaitLatch to assert that if select() reports that the postmaster pipe has been closed, PostmasterIsAlive() also returns false.
    The correct solution would be to read() from the pipe after select()
    returns, and only return WL_POSTMASTER_DEATCH if the read doesn't return
    EAGAIN. To prevent that read() from blocking if the read event was indeed
    spurious, O_NONBLOCK must be set on the pipe but that patch does that already.
    +1

    The syslogger read() from the pipe after select(), then it thinks EOF
    has arrived and
    there is no longer write-side process if the return value of read() is
    just zero.

    Regards,

    --
    Fujii Masao
    NIPPON TELEGRAPH AND TELEPHONE CORPORATION
    NTT Open Source Software Center
  • Peter Geoghegan at Jul 4, 2011 at 9:11 pm

    On 4 July 2011 16:53, Heikki Linnakangas wrote:
    Ok, here's a new patch, addressing the issues Fujii raised, and with a bunch
    of stylistic changes of my own. Also, I committed a patch to remove
    silent_mode, so the fork_process() changes are now gone. I'm going to sleep
    over this and review once again tomorrow, and commit if it still looks good
    to me and no-one else reports new issues.
    Looks good.
    I don't like the names POSTMASTER_FD_WATCH and POSTMASTER_FD_OWN. At a quick
    glance, it's not at all clear which is which. I couldn't come up with better
    names, so for now I just added some comments to clarify that. I would find
    WRITE/READ more clear, but to make sense of that you need to how the pipe is
    used. Any suggestions or opinions on that?
    We could bikeshed about that until the cows come home, but we're not
    going to come up with names that make the purpose of each evident at a
    glance - it's too involved. If we could, we would have thought of them
    already. Besides, I've probably already written all the client code
    those macros will ever have.
    On 4 July 2011 17:36, Florian Pflug wrote:
    On Jul4, 2011, at 17:53 , Heikki Linnakangas wrote:
    Under Linux, select() may report a socket file descriptor as "ready for
    reading",  while nevertheless a subsequent read blocks.  This could for
    example happen when data has arrived but  upon  examination  has  wrong
    checksum and is discarded.  There may be other circumstances in which a
    file descriptor is spuriously reported as ready.  Thus it may be  safer
    to use O_NONBLOCK on sockets that should not block.
    So in theory, on Linux you might WaitLatch might sometimes incorrectly return WL_POSTMASTER_DEATH. None of the callers check for WL_POSTMASTER_DEATH return code, they call PostmasterIsAlive() before assuming the postmaster has died, so that won't affect correctness at the moment. I doubt that scenario can even happen in our case, select() on a pipe that is never written to. But maybe we should add add an assertion to WaitLatch to assert that if select() reports that the postmaster pipe has been closed, PostmasterIsAlive() also returns false.
    The correct solution would be to read() from the pipe after select()
    returns, and only return WL_POSTMASTER_DEATCH if the read doesn't return
    EAGAIN. To prevent that read() from blocking if the read event was indeed
    spurious, O_NONBLOCK must be set on the pipe but that patch does that already.
    Let's have some perspective on this. We're talking about a highly
    doubtful chance that latches may wake when they shouldn't. Latches are
    typically expected to wake up for a variety of reasons, and if that
    occurred in the archiver's case with my patch applied, I think we'd
    just go asleep again without anything happening. It seems likely that
    latch client code in general will never trip up on this, as long as
    its not exclusively relying on the waitLatch() return value to report
    pm death.

    Maybe we should restore the return value of WaitLatch to its previous
    format (so it doesn't return a bitmask)? That way, we don't report
    that the Postmaster died, and therefore clients are required to call
    PostmasterIsAlive() to be sure. In any case, I'm in favour of the
    assertion.
    Btw, with the death-watch / life-sign / whatever infrastructure in place,
    shouldn't PostmasterIsAlive() be using that instead of getppid() / kill(0)?
    Hmm, maybe. That seems like a separate issue though, that can be
    addressed with another patch. It does have the considerable
    disadvantage of making Heikki's proposed assertion failure useless. Is
    the implementation of PostmasterIsAlive() really a problem at the
    moment?

    --
    Peter Geoghegan       http://www.2ndQuadrant.com/
    PostgreSQL Development, 24x7 Support, Training and Services
  • Florian Pflug at Jul 4, 2011 at 9:42 pm

    On Jul4, 2011, at 23:11 , Peter Geoghegan wrote:
    On 4 July 2011 17:36, Florian Pflug wrote:
    On Jul4, 2011, at 17:53 , Heikki Linnakangas wrote:
    Under Linux, select() may report a socket file descriptor as "ready for
    reading", while nevertheless a subsequent read blocks. This could for
    example happen when data has arrived but upon examination has wrong
    checksum and is discarded. There may be other circumstances in which a
    file descriptor is spuriously reported as ready. Thus it may be safer
    to use O_NONBLOCK on sockets that should not block.
    So in theory, on Linux you might WaitLatch might sometimes incorrectly return WL_POSTMASTER_DEATH. None of the callers check for WL_POSTMASTER_DEATH return code, they call PostmasterIsAlive() before assuming the postmaster has died, so that won't affect correctness at the moment. I doubt that scenario can even happen in our case, select() on a pipe that is never written to. But maybe we should add add an assertion to WaitLatch to assert that if select() reports that the postmaster pipe has been closed, PostmasterIsAlive() also returns false.
    The correct solution would be to read() from the pipe after select()
    returns, and only return WL_POSTMASTER_DEATCH if the read doesn't return
    EAGAIN. To prevent that read() from blocking if the read event was indeed
    spurious, O_NONBLOCK must be set on the pipe but that patch does that already.
    Let's have some perspective on this. We're talking about a highly
    doubtful chance that latches may wake when they shouldn't.
    Yeah, as long as there's just a spurious wake up, sure. However,
    having WaitLatch() indicate a postmaster death in that case seems
    dangerous. Some caller, sooner or later, is bound to get it wrong,
    i.e. forget to re-check PostmasterIsAlive.
    Maybe we should restore the return value of WaitLatch to its previous
    format (so it doesn't return a bitmask)? That way, we don't report
    that the Postmaster died, and therefore clients are required to call
    PostmasterIsAlive() to be sure.
    That'd solve the issue too.
    In any case, I'm in favour of the assertion.
    I don't really see the value in that assertion. It'd cause spurious
    assertion failures in the case of spurious events reported by select().
    If we do expect such event, we should close the hole instead of asserting.
    If we don't, then what's the point of the assert.

    BTW, do we currently retry the select() on EINTR (meaning a signal has
    arrived)? If we don't, that'd be an additional source of spurious returns
    from select.
    Btw, with the death-watch / life-sign / whatever infrastructure in place,
    shouldn't PostmasterIsAlive() be using that instead of getppid() / kill(0)?
    Hmm, maybe. That seems like a separate issue though, that can be
    addressed with another patch. It does have the considerable
    disadvantage of making Heikki's proposed assertion failure useless. Is
    the implementation of PostmasterIsAlive() really a problem at the
    moment?
    I'm not sure that there is currently a guarantee that PostmasterIsAlive
    will returns false immediately after select() indicates postmaster
    death. If e.g. the postmaster's parent is still running (which happens
    for example if you launch postgres via daemontools), the re-parenting of
    backends to init might not happen until the postmaster zombie has been
    vanquished by its parent's call of waitpid(). It's not entirely
    inconceivable for getppid() to then return the (dead) postmaster's pid
    until that waitpid() call has occurred.

    But agreed, this is probably best handled by a separate patch.

    best regards,
    Florian Pflug
  • Peter Geoghegan at Jul 5, 2011 at 12:11 am

    On 4 July 2011 22:42, Florian Pflug wrote:
    If we do expect such event, we should close the hole instead of asserting.
    If we don't, then what's the point of the assert.
    You can say the same thing about any assertion. I'm not going to
    attempt to close the hole because I don't believe that there is one. I
    would be happy to see your "read() from the pipe after select()" test
    asserted though.
    BTW, do we currently retry the select() on EINTR (meaning a signal has
    arrived)? If we don't, that'd be an additional source of spurious returns
    from select.
    Why might it be? WaitLatch() is currently documented to potentially
    have its timeout invalidated by the process receiving a signal, which
    is the exact opposite problem. We do account for this within the
    archiver calling code though, and I remark upon it in a comment there.
    I'm not sure that there is currently a guarantee that PostmasterIsAlive
    will returns false immediately after select() indicates postmaster
    death. If e.g. the postmaster's parent is still running (which happens
    for example if you launch postgres via daemontools), the re-parenting of
    backends to init might not happen until the postmaster zombie has been
    vanquished by its parent's call of waitpid(). It's not entirely
    inconceivable for getppid() to then return the (dead) postmaster's pid
    until that waitpid() call has occurred.
    Yes, this did occur to me - it's hard to reason about what exactly
    happens here, and probably impossible to have the behaviour guaranteed
    across platforms, however unlikely it seems. I'd like to hear what
    Heikki has to say about asserting or otherwise verifying postmaster
    death in the case of apparent postmaster death wake-up.

    --
    Peter Geoghegan       http://www.2ndQuadrant.com/
    PostgreSQL Development, 24x7 Support, Training and Services
  • Heikki Linnakangas at Jul 5, 2011 at 6:49 am

    On 05.07.2011 00:42, Florian Pflug wrote:
    On Jul4, 2011, at 23:11 , Peter Geoghegan wrote:
    On 4 July 2011 17:36, Florian Pflugwrote:
    Btw, with the death-watch / life-sign / whatever infrastructure in place,
    shouldn't PostmasterIsAlive() be using that instead of getppid() / kill(0)?
    Hmm, maybe. That seems like a separate issue though, that can be
    addressed with another patch. It does have the considerable
    disadvantage of making Heikki's proposed assertion failure useless. Is
    the implementation of PostmasterIsAlive() really a problem at the
    moment?
    I'm not sure that there is currently a guarantee that PostmasterIsAlive
    will returns false immediately after select() indicates postmaster
    death. If e.g. the postmaster's parent is still running (which happens
    for example if you launch postgres via daemontools), the re-parenting of
    backends to init might not happen until the postmaster zombie has been
    vanquished by its parent's call of waitpid(). It's not entirely
    inconceivable for getppid() to then return the (dead) postmaster's pid
    until that waitpid() call has occurred.
    Good point, and testing shows that that is exactly what happens at least
    on Linux (see attached test program). So, as the code stands, the
    children will go into a busy loop until the grandparent calls waitpid().
    That's not good.

    In that light, I agree we should replace kill() in PostmasterIsAlive()
    with read() on the pipe. It would react faster than the kill()-based
    test, which seems like a good thing. Or perhaps do both, and return
    false if either test says the postmaster is dead.

    --
    Heikki Linnakangas
    EnterpriseDB http://www.enterprisedb.com
  • Peter Geoghegan at Jul 5, 2011 at 9:28 am

    On 5 July 2011 07:49, Heikki Linnakangas wrote:
    Good point, and testing shows that that is exactly what happens at least on
    Linux (see attached test program). So, as the code stands, the children will
    go into a busy loop until the grandparent calls waitpid(). That's not good.

    In that light, I agree we should replace kill() in PostmasterIsAlive() with
    read() on the pipe. It would react faster than the kill()-based test, which
    seems like a good thing. Or perhaps do both, and return false if either test
    says the postmaster is dead.
    Hmm. Why assume that the opposite problem doesn't exist? What if the
    kill-based test is faster than the read() on the pipe on some platform
    or under some confluence of events?

    I suggest that we agree on a standard for determining whether or not
    the postmaster is dead and stick to it - that's already the case on
    Windows. Since that standard cannot be the kill() based test, because
    that would make a postmaster death aware latch implementation
    impossible, it has to be the read() test proposed by Florian.

    --
    Peter Geoghegan       http://www.2ndQuadrant.com/
    PostgreSQL Development, 24x7 Support, Training and Services
  • Peter Geoghegan at Jul 7, 2011 at 5:42 pm
    I now think that we shouldn't change the return value format from the
    most recent revisions of the patch (i.e. returning a bitfield). We
    should leave it as-is, while documenting that it's possible, although
    extremely unlikely, for it to incorrectly report Postmaster death, and
    that clients therefore have a onus to check that themselves using
    PostmasterIsAlive(). We already provide fairly weak guarantees as to
    the validity of that return value ("Note that if multiple wake-up
    conditions are true, there is no guarantee that we return all of them
    in one call, but we will return at least one"). Making them a bit
    weaker still seems acceptable.

    In addition, we'd change the implementation of PostmasterIsAlive() to
    /just/ perform the read() test as already described.

    I'm not concerned about the possibility of spurious extra cycles of
    auxiliary process event loops - should I be?

    --
    Peter Geoghegan       http://www.2ndQuadrant.com/
    PostgreSQL Development, 24x7 Support, Training and Services
  • Robert Haas at Jul 7, 2011 at 6:29 pm

    On Thu, Jul 7, 2011 at 1:41 PM, Peter Geoghegan wrote:
    I now think that we shouldn't change the return value format from the
    most recent revisions of the patch (i.e. returning a bitfield). We
    should leave it as-is, while documenting that it's possible, although
    extremely unlikely, for it to incorrectly report Postmaster death, and
    that clients therefore have a onus to check that themselves using
    PostmasterIsAlive(). We already provide fairly weak guarantees as to
    the validity of that return value ("Note that if multiple wake-up
    conditions are true, there is no guarantee that we return all of them
    in one call, but we will return at least one"). Making them a bit
    weaker still seems acceptable.
    I agree - that seems like a good way to handle it.
    In addition, we'd change the implementation of PostmasterIsAlive() to
    /just/ perform the read() test as already described.

    I'm not concerned about the possibility of spurious extra cycles of
    auxiliary process event loops - should I be?
    A tight loop would be bad, but an occasional spurious wake-up seems harmless.

    --
    Robert Haas
    EnterpriseDB: http://www.enterprisedb.com
    The Enterprise PostgreSQL Company
  • Peter Geoghegan at Jul 8, 2011 at 9:57 am

    On 7 July 2011 19:15, Robert Haas wrote:
    I'm not concerned about the possibility of spurious extra cycles of
    auxiliary process event loops - should I be?
    A tight loop would be bad, but an occasional spurious wake-up seems harmless.
    We should also assert !PostmasterIsAlive() from within the latch code
    after waking due to apparent Postmaster death. The reason that I don't
    want to follow Florian's suggestion to check it in production is that
    I don't know what to do if the postmaster turns out to be alive. Why
    is it more reasonable to try again than to just return? If the
    spurious wake-up thing was a problem that we could actually reproduce,
    then maybe I'd have an opinion on it. As it stands, our entire basis
    for thinking this may be a problem is the sentence "There may be other
    circumstances in which a file descriptor is spuriously reported as
    ready". That seems rather flimsy.

    Anyone that still has any misgivings about this will probably feel
    better once the assertion is never reported to fail on any of the
    diverse systems that PostgreSQL will be tested on in advance of the
    9.2 release.

    --
    Peter Geoghegan       http://www.2ndQuadrant.com/
    PostgreSQL Development, 24x7 Support, Training and Services
  • Florian Pflug at Jul 8, 2011 at 10:59 am

    On Jul8, 2011, at 11:57 , Peter Geoghegan wrote:
    On 7 July 2011 19:15, Robert Haas wrote:
    I'm not concerned about the possibility of spurious extra cycles of
    auxiliary process event loops - should I be?
    A tight loop would be bad, but an occasional spurious wake-up seems harmless.
    We should also assert !PostmasterIsAlive() from within the latch code
    after waking due to apparent Postmaster death. The reason that I don't
    want to follow Florian's suggestion to check it in production is that
    I don't know what to do if the postmaster turns out to be alive. Why
    is it more reasonable to try again than to just return?
    I'd say return, but don't indicate postmaster death in the return value
    if PostmasterIsAlive() returns true. Or don't call PostmasterIsAlive() in
    WaitLatch(), and return indicating postmaster death whenever select()
    says so, and put the burden of re-checking on the callers.

    I agree that retrying isn't all that reasonable.
    If the
    spurious wake-up thing was a problem that we could actually reproduce,
    then maybe I'd have an opinion on it. As it stands, our entire basis
    for thinking this may be a problem is the sentence "There may be other
    circumstances in which a file descriptor is spuriously reported as
    ready". That seems rather flimsy.
    Flimsy or not, it pretty clearly warns us not to depend on there being
    no spurious wake ups. Whether or not we know how to actually produce
    there is IMHO largely irrelevant - what matters is whether the guarantees
    given by select() match the expectations of our code. Which, according to
    the cited passage, they currently don't.
    Anyone that still has any misgivings about this will probably feel
    better once the assertion is never reported to fail on any of the
    diverse systems that PostgreSQL will be tested on in advance of the
    9.2 release.
    I'm not so convinced that WaitLatch() will get exercised much on
    assert-enabled builds. But I might very well be wrong there...

    best regards,
    Florian Pflug
  • Heikki Linnakangas at Jul 8, 2011 at 12:40 pm

    On 08.07.2011 13:58, Florian Pflug wrote:
    On Jul8, 2011, at 11:57 , Peter Geoghegan wrote:
    On 7 July 2011 19:15, Robert Haaswrote:
    I'm not concerned about the possibility of spurious extra cycles of
    auxiliary process event loops - should I be?
    A tight loop would be bad, but an occasional spurious wake-up seems harmless.
    We should also assert !PostmasterIsAlive() from within the latch code
    after waking due to apparent Postmaster death. The reason that I don't
    want to follow Florian's suggestion to check it in production is that
    I don't know what to do if the postmaster turns out to be alive. Why
    is it more reasonable to try again than to just return?
    I'd say return, but don't indicate postmaster death in the return value
    if PostmasterIsAlive() returns true. Or don't call PostmasterIsAlive() in
    WaitLatch(), and return indicating postmaster death whenever select()
    says so, and put the burden of re-checking on the callers.
    I put the burden on the callers. Removing the return value from
    WaitLatch() altogether just makes life unnecessarily difficult for
    callers that could safely use that information, even if you sometimes
    get spurious wakeups. In particular, the coding in pgarch.c is nicer if
    you can simply check the return code for WL_TIMEOUT, rather than call
    time(NULL) to figure out if the timeout was reached.

    Attached is a new version of this patch. PostmasterIsAlive() now uses
    read() on the pipe instead of kill().

    --
    Heikki Linnakangas
    EnterpriseDB http://www.enterprisedb.com
  • Peter Geoghegan at Jul 8, 2011 at 1:11 pm

    On 8 July 2011 13:40, Heikki Linnakangas wrote:
    I put the burden on the callers. Removing the return value from WaitLatch()
    altogether just makes life unnecessarily difficult for callers that could
    safely use that information, even if you sometimes get spurious wakeups. In
    particular, the coding in pgarch.c is nicer if you can simply check the
    return code for WL_TIMEOUT, rather than call time(NULL) to figure out if the
    timeout was reached. +1
    Attached is a new version of this patch. PostmasterIsAlive() now uses read()
    on the pipe instead of kill().
    The consensus so far is that in practice spurious wake-ups in
    auxiliary process event loops won't a problem. You may want to wait
    for others to weigh in here.

    This comment in pgarch.c is slightly malformed - note the quote:

    /*
    * Sleep until a signal is received, or until a poll is forced by
    ' PGARCH_AUTOWAKE_INTERVAL having passed since last_copy_time, or
    * until postmaster dies.
    */

    Other than that, I suggest you commit v8 as-is.

    Incidentally, I like that this removes the amDirectChild argument to
    PostmasterIsAlive() - an added benefit.

    --
    Peter Geoghegan       http://www.2ndQuadrant.com/
    PostgreSQL Development, 24x7 Support, Training and Services
  • Heikki Linnakangas at Jul 8, 2011 at 1:16 pm

    On 08.07.2011 16:11, Peter Geoghegan wrote:
    Incidentally, I like that this removes the amDirectChild argument to
    PostmasterIsAlive() - an added benefit.
    amDirectChild==false has actually been dead code for years. But the new
    pipe method would work for a non-direct child too as long as the pipe fd
    is inherited by the non-direct child, should we need that in the future
    again.

    --
    Heikki Linnakangas
    EnterpriseDB http://www.enterprisedb.com
  • Florian Pflug at Jul 8, 2011 at 2:58 pm

    On Jul8, 2011, at 14:40 , Heikki Linnakangas wrote:
    On 08.07.2011 13:58, Florian Pflug wrote:
    On Jul8, 2011, at 11:57 , Peter Geoghegan wrote:
    On 7 July 2011 19:15, Robert Haaswrote:
    I'm not concerned about the possibility of spurious extra cycles of
    auxiliary process event loops - should I be?
    A tight loop would be bad, but an occasional spurious wake-up seems harmless.
    We should also assert !PostmasterIsAlive() from within the latch code
    after waking due to apparent Postmaster death. The reason that I don't
    want to follow Florian's suggestion to check it in production is that
    I don't know what to do if the postmaster turns out to be alive. Why
    is it more reasonable to try again than to just return?
    I'd say return, but don't indicate postmaster death in the return value
    if PostmasterIsAlive() returns true. Or don't call PostmasterIsAlive() in
    WaitLatch(), and return indicating postmaster death whenever select()
    says so, and put the burden of re-checking on the callers.
    I put the burden on the callers. Removing the return value from WaitLatch() altogether just makes life unnecessarily difficult for callers that could safely use that information, even if you sometimes get spurious wakeups. In particular, the coding in pgarch.c is nicer if you can simply check the return code for WL_TIMEOUT, rather than call time(NULL) to figure out if the timeout was reached.

    Attached is a new version of this patch. PostmasterIsAlive() now uses read() on the pipe instead of kill().
    I did notice a few (very minor) loose ends...

    SyncRepWaitForLSN() says
    /*
    * Wait on latch for up to 60 seconds. This allows us to check for
    * postmaster death regularly while waiting. Note that timeout here
    * does not necessarily release from loop.
    */
    WaitLatch(&MyProc->waitLatch, 60000000L);

    I guess that 60-second timeout is unnecessary now that we'll wake up
    on postmaster death anyway.

    Also, none of the callers of WaitLatch() seems to actually inspect
    the WL_POSTMASTER_DEATH bit of the result. We might want to make
    their !PostmasterIsAlive() check conditional on the WL_POSTMASTER_DEATH
    bit being set. At least in the case of SyncRepWaitForLSN(), it seems
    that avoiding the extra read() syscall might be beneficial.

    Maybe these cleanups would better be done in a separate patch, though...

    best regards,
    Florian Pflug
  • Peter Geoghegan at Jul 8, 2011 at 4:05 pm

    On 8 July 2011 15:58, Florian Pflug wrote:
    SyncRepWaitForLSN() says
    /*
    * Wait on latch for up to 60 seconds. This allows us to check for
    * postmaster death regularly while waiting. Note that timeout here
    * does not necessarily release from loop.
    */
    WaitLatch(&MyProc->waitLatch, 60000000L);

    I guess that 60-second timeout is unnecessary now that we'll wake up
    on postmaster death anyway.
    We won't wake up on Postmaster death here, because we haven't asked to
    - not yet, anyway. We're just using the new interface here for that
    one function call in v8.
    Also, none of the callers of WaitLatch() seems to actually inspect
    the WL_POSTMASTER_DEATH bit of the result. We might want to make
    their !PostmasterIsAlive() check conditional on the WL_POSTMASTER_DEATH
    bit being set. At least in the case of SyncRepWaitForLSN(), it seems
    that avoiding the extra read() syscall might be beneficial.
    I don't think so. Postmaster death is an anomaly, so why bother with
    any sort of optimisation for that case? Also, that's exactly the sort
    of thing that we're trying to caution callers against doing with this
    comment:

    "That should be rare in practice, but the caller should not use the
    return value for anything critical, re-checking the situation with
    PostmasterIsAlive() or read() on a socket if necessary."

    You might say that the only reason we even bother reporting postmaster
    death in the returned bitfield is because there is an expectation that
    it will report it, given that we use the same masks on wakeEvents to
    inform the function what events we'll actually be waiting on for the
    call.

    --
    Peter Geoghegan       http://www.2ndQuadrant.com/
    PostgreSQL Development, 24x7 Support, Training and Services
  • Florian Pflug at Jul 8, 2011 at 4:16 pm

    On Jul8, 2011, at 17:56 , Peter Geoghegan wrote:
    On 8 July 2011 15:58, Florian Pflug wrote:
    SyncRepWaitForLSN() says
    /*
    * Wait on latch for up to 60 seconds. This allows us to check for
    * postmaster death regularly while waiting. Note that timeout here
    * does not necessarily release from loop.
    */
    WaitLatch(&MyProc->waitLatch, 60000000L);

    I guess that 60-second timeout is unnecessary now that we'll wake up
    on postmaster death anyway.
    We won't wake up on Postmaster death here, because we haven't asked to
    - not yet, anyway. We're just using the new interface here for that
    one function call in v8.
    Oh, Right. I still think it'd might be worthwhile to convert it, but
    but not in this patch.
    Also, none of the callers of WaitLatch() seems to actually inspect
    the WL_POSTMASTER_DEATH bit of the result. We might want to make
    their !PostmasterIsAlive() check conditional on the WL_POSTMASTER_DEATH
    bit being set. At least in the case of SyncRepWaitForLSN(), it seems
    that avoiding the extra read() syscall might be beneficial.
    I don't think so. Postmaster death is an anomaly, so why bother with
    any sort of optimisation for that case? Also, that's exactly the sort
    of thing that we're trying to caution callers against doing with this
    comment:

    "That should be rare in practice, but the caller should not use the
    return value for anything critical, re-checking the situation with
    PostmasterIsAlive() or read() on a socket if necessary."
    Uh, I phrased that badly. What I meant was doing

    if ((result & WL_POSTMASTER_DEATH) && (!PostmasterIsAlive())

    instead of

    if (!PostmasterIsAlive)

    It seems that currently SyncRepWaitForLSN() will execute
    PostmasterIsAlive() after every wake up. But actually it only needs
    to do that if WaitLatch() sets WL_POSTMASTER_DEATH. Usually we wouldn't
    care, but in the case of SyncRepWaitForLSN() I figures that we might.
    It's in the code path of COMMIT (in the case of synchronous replication)
    after all...

    We'd not optimize the case of a dead postmaster, but the case of
    an live one. Which I do hope is the common case ;-)
    You might say that the only reason we even bother reporting postmaster
    death in the returned bitfield is because there is an expectation that
    it will report it, given that we use the same masks on wakeEvents to
    inform the function what events we'll actually be waiting on for the
    call.
    I kinda guessed that to be the reason after reading the latest patch ;-)

    best regards,
    Florian Pflug
  • Heikki Linnakangas at Jul 8, 2011 at 4:17 pm

    On 08.07.2011 18:56, Peter Geoghegan wrote:
    On 8 July 2011 15:58, Florian Pflugwrote:
    Also, none of the callers of WaitLatch() seems to actually inspect
    the WL_POSTMASTER_DEATH bit of the result. We might want to make
    their !PostmasterIsAlive() check conditional on the WL_POSTMASTER_DEATH
    bit being set. At least in the case of SyncRepWaitForLSN(), it seems
    that avoiding the extra read() syscall might be beneficial.
    I don't think so. Postmaster death is an anomaly, so why bother with
    any sort of optimisation for that case?
    We currently call PostmasterIsAlive() on every iteration of the loop,
    and what Florian is saying is that it would be more efficient to only
    call PostmasterIsAlive() if WaitLatch() reports that the postmaster has
    died. That's an optimization that would help the case where postmaster
    has not died. However, I don't think it makes any difference in practice
    because the loop usually only iterates once, and there's break
    statements to exit the loop as soon as one of the other exit conditions
    are satisfied.

    I just committed the v8 of the patch, BTW, after fixing the comment typo
    you pointed out. Thanks!

    --
    Heikki Linnakangas
    EnterpriseDB http://www.enterprisedb.com
  • Peter Geoghegan at Jul 8, 2011 at 4:27 pm

    On 8 July 2011 17:10, Heikki Linnakangas wrote:
    I just committed the v8 of the patch, BTW, after fixing the comment typo you
    pointed out. Thanks!
    Great, thanks.

    Also, thank you Florian and Fujii.

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

Related Discussions

Discussion Navigation
viewthread | post
Discussion Overview
grouppgsql-hackers @
categoriespostgresql
postedMay 24, '11 at 8:44p
activeJul 8, '11 at 4:27p
posts46
users7
websitepostgresql.org...
irc#postgresql

People

Translate

site design / logo © 2021 Grokbase