Overhauling our interrupt handling (was Escaping from blocked send() reprised.)

Hi,

This mail is a answer to
http://archives.postgresql.org/message-id/20150110022542.GF12509%40alap3.anarazel.de
but I decided that it's a good go move it into a new thread since the
patchseries has outgrown its original target. It's invasive and touches
many arcane areas of the code, so that I think more eyes than a long
deep thread is likely to have, are a good thing.

As a short description of the goal of the patchseries:
This started out as steps on the way to be able to safely handle
terminate_backend() et al when we're reading/writing from the
client. E.g. because the client is dead and we haven't noticed yet and
are stuck writing, or because we want to implement a idle_in_transaction
timeout.

Making these changes allowed me to see that not doing significant work
in signal handlers can make code much simpler and more robust. The
number of bugs postgres had in the past that involved doing too much in
signal handlers is significant.  Thus I've since extended the
patchseries to get rid of nearly all nontrivial work in signal
handlers.

All the patches in the series up to 0008 hav ecommit messages providing
more detail. A short description of each patch follows:

0001: Replace walsender's latch with the general shared latch.

      New patch that removes ImmediateInteruptOK behaviour from walsender. I
      think that's a rather good idea, because walsender currently seems to
      assume WaitLatchOrSocket is reentrant - which I don't think is really
      guaranteed.
      Hasn't been reviewed yet, but I think it's not far from being
      committable.

0002: Use a nonblocking socket for FE/BE communication and block using
      latches.

      Has previously been reviewed by Heikki. I think Noah also had a
      look, although I'm not sure how close that was.

      I think this can be committed soon.

0003: Introduce and use infrastructure for interrupt processing during client reads.

      From here on ImmediateInterruptOK isn't set during client
      communication. Normal interrupts and sinval/async interrupts are
      processed outside of signal handlers. Especially the sinval/async
      greatly simplify the respective code.

      Has been reviewed by Heikki in an earlier version - I think I
      addressed all the review comments.

      I'd like somebody else to look at the sinval/async.c changes
      before committing. I really like the simplification this change
      allowed.

      I think this patch might not be safe without 0004 because I can't
      convince myself that it's safe to interrupt latch waits with work
      that actually might also use the same latch (sinval
      interrupts). But it's easier to review this way as 0004 is quite
      big.

0004: Process 'die' interrupts while reading/writing from the client socket.

      This is the reason Horiguchi-san started this thread.

      I think the important debate here is whether we think it's
      acceptable that there are situations (a full socket buffer, but a
      alive connection) where the client previously got an error, but
      not anymore afterwards. I think that's much better than having
      unkillable connections, but I can see others being of a different
      opinion.

0005: Move deadlock and other interrupt handling in proc.c out of signal handlers.

      I'm surprised how comparatively simple this turned out to be. For
      robustness and understanding I think this is a great improvement.

      New and not reviewed at all. Needs significant review. But works
      in my simple testcases.

0006: Don't allow immediate interupts during authentication anymore.

      So far we've set ImmediateInterruptOK to true during large parts
      of the client authentication - that's not all that pretty,
      interrupts might arrive while we're in some system routines.

      Due to patches 0003/0004 we now are able to safely serve
      interrupts during client communication which is the major are
      where we want to adhere to authentication_timeout.

      I additionally placed some CHECK_FOR_INTERRUPTS()s in some
      somewhat randomly chosen places in auth.c. Those don't completely
      get back the previous 'appruptness' (?) of reacting to
      interrupts, but that's partially for the better, because we don't
      interrupt foreign code anymore.

0007: Remove the option to service interrupts during PGSemaphoreLock().

      Due to patch 0005 there's no user of PGSemaphoreLock(lock, interruptOK = true)

      anymore, so remove the code to handle that. I find it rather odd
      that the code did a CHECK_FOR_INTERRUPTS before the semwait even
      when interruptOK was set to to false - that only happened to work
      because lwlock.c did a HOLD_INTERRUPTS() around the semaphore
      code. It's now removed entirely.

      This is a API break because the signature of PGSemaphoreLock()
      changed. But I have a hard time seing that as a problem. For one I
      don't think there's many users of PGSemaphoreLock, for another
      they should change their interrupt handling.

      New and not reviewed.

0008: Remove remnants of ImmediateInterruptOK handling.

      Now that ImmediateInterruptOK is never set to true anymore, we can
      remove related code and comments.

      New and not reviewed.

0009: WIP: lwlock: use latches

      Optional patch that removes the use of semaphores from the lwlock
      code. There's no benefit for lwlock.c itself due to this. But it
      does get rid of the last user of semaphores in a normal postgres
      build. I primarily wrote this to test the performance of latches.

      I guess we want to do this anyway.

0010: WIP: unix_latch: WIP efficiency hackery

      Nothing ready, although I think using eventfds on linux is worth
      the cost.


Questions I have about the series right now:

1) Do others agree that getting rid of ImmediateInterruptOK is
   worthwile. I personally think that we really should pursue that
   aggressively as a goal.

2) Does anybody see a significant problem with the reduction of cases
   where we immediately react to authentication_timeout? Since we still
   react immediately when we're waiting for the client (except maybe in
   sspi/kerberos?) I think that's ok. The waiting cases are slow
   ident/radius/kerberos/ldap/... servers. But we really shouldn't jump
   out of the relevant code anyway.

3) If we do everything (in corrected), upto 0009, there's no remaining
   user of semaphores in postgres, except the spinlock emulation.

   Does anybody see a need for PGPROC->sem to remain? Removing it would
   have the significant benefit that semaphores are the last thing users
   frequently need to tune to get postgres to start up when using a
   higher max_connection or multiple clusters.

   If we remove PGPROC->sem does anybody see a way to get rid of the
   semaphore code alltogether? I personally still think we should just
   gank --disable-spinlocks. Now that it's only a couple lines (in an
   preexisting patch) to rely on compiler intrinsics for spinlocks that
   doesn't sound like a big deal.  Even if not, we could decide to get
   rid of win32_sem.c...

4) There remains one user of something like ImmediateInterruptOK -
   walreceiver. It has WalRcvImmediateInterruptOK. We definitely should
   get rid of that as well, but that's a independent patch that can be
   written in the future.

5) In a future patch I think we could also handle catchup interrupts
   during other client reads, not just ReadCommand(). Does anybody see
   that as a worthwile goal? I can't remember many problems with not
   enough catchup happening, but I think someone mentioned that as a
   problem in the past.

6) Review ;)

Comments?

Greetings,

Andres Freund

--
 Andres Freund                       http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services