Horton

Re: Unportable implementation of background worker start

Tom Lane <tgl@sss.pgh.pa.us>

View thread View on postgr.es
From: Tom Lane <tgl@sss.pgh.pa.us>
To: Alvaro Herrera <alvherre@2ndquadrant.com>
Cc: Andres Freund <andres@anarazel.de>, pgsql-hackers@postgresql.org
Date: 2017-04-21T16:50:04Z
Lists: pgsql-hackers

Attachments

Attached is a lightly-tested draft patch that converts the postmaster to
use a WaitEventSet for waiting in ServerLoop.  I've got mixed emotions
about whether this is the direction to proceed, though.  It adds at least
a couple of kernel calls per postmaster signal delivery, and probably to
every postmaster connection acceptance (ServerLoop iteration), to fix
problems that are so corner-casey that we've never even known we had them
till now.

I'm looking longingly at pselect(2), ppoll(2), and epoll_pwait(2), which
would solve the problem without need for a self-pipe.  But the latter two
are Linux-only, and while pselect does exist in recent POSIX editions,
it's nonetheless got portability issues.  Googling suggests that on a
number of platforms, pselect is non-atomic, ie it's nothing but a
wrapper for sigprocmask/select/sigprocmask, which would mean that the
race condition I described in my previous message still exists.

Despite that, a pretty attractive proposal is to do, essentially,

#ifdef HAVE_PSELECT
        selres = pselect(nSockets, &rmask, NULL, NULL, &timeout, &UnBlockSig);
#else
        PG_SETMASK(&UnBlockSig);
        selres = select(nSockets, &rmask, NULL, NULL, &timeout);
        PG_SETMASK(&BlockSig);
#endif

This fixes the race on platforms where pselect exists and is correctly
implemented, and we're no worse off than before where that's not true.

The other component of the problem is the possibility that select() will
restart if the signal is marked SA_RESTART.  (Presumably that would apply
to pselect too.)  I am thinking that maybe the answer is "if it hurts,
don't do it" --- that is, in the postmaster maybe we shouldn't use
SA_RESTART, at least not for these signals.

A different line of thought is to try to provide a bulletproof solution,
but push the implementation problems down into latch.c --- that is, the
goal would be to provide a pselect-like variant of WaitEventSetWait that
is guaranteed to return if interrupted, as opposed to the current behavior
where it's guaranteed not to.  But that seems like quite a bit of work.

Whether or not we decide to change over the postmaster.c code, I think
it'd likely be a good idea to apply most or all of the attached changes
in latch.c.  Setting CLOEXEC on the relevant FDs is clearly a good thing,
and the other changes will provide some safety if some preloaded extension
decides to create a latch in the postmaster process.

			regards, tom lane

Commits

  1. Cope with glibc too old to have epoll_create1().

  2. Make latch.c more paranoid about child-process cases.

  3. Allow multiple bgworkers to be launched per postmaster iteration.

  4. Revert "Use pselect(2) not select(2), if available, to wait in postmaster's loop."

  5. Use pselect(2) not select(2), if available, to wait in postmaster's loop.

  6. Run the postmaster's signal handlers without SA_RESTART.

  7. Fix postmaster's handling of fork failure for a bgworker process.

  8. Partially revert commit 536d47bd9d5fce8d91929bee3128fa1d08dbcc57.

  9. Avoid depending on non-POSIX behavior of fcntl(2).

  10. Remove long-obsolete catering for platforms without F_SETFD/FD_CLOEXEC.