Re: Overhauling our interrupt handling - Mailing list pgsql-hackers

Hello, I'd synced up this at last.

I think I should finilize my commitfest item for this issue, with
.. "Rejected"?

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

It sounds pretty nice.

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

Deesn't this patchset containing per-socket basis non-blocking
control for win32? It should make the code (above the win32
socket layer itself) more simpler.

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


This patch yields a code a bit confusion like following.

| secure_raw_write(Port *port, const void *ptr, size_t len)
| {
..
|         w = WaitLatchOrSocket(MyLatch,
|         if (w & WL_LATCH_SET)
...
|                 errno = EINTR;
|         else if (w & WL_SOCKET_WRITEABLE)
|                 goto wloop;
|
|         errno = save_errno;

The errno set when WL_LATCH_SET always vanishes. Specifically,
the EINTR set by SIGTERM(WL_LATCH_SET) is overwritten by
EAGAIN. As the result, pg_terminte_backend() cannot kill the
backend till the write blocking is released. errno = save_errno
should be the alternative of the line "errno = EINTR" and I
confirmed that the change leads to the desirable (as of me)
behavior.


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

Simplly as a comment on style, this patch introduces checks of
ClientAuthInProgress twice successively into
ProcessInterrupts(). Isn't it better to make it like following?

| /* As in quickdie, ...
| if (ClientAuthInProgress)
| {
|    if (whereToSendOutput == DestRemote) whereToSendOutput = DestNone;
|    ereport(FATAL,

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

walreceiver.c still has WalRcvImmediateInterruptOK as mentioned
below, apart from whether it should be changed or not, the
following comment remains.

| * This is very much like what regular backends do with ImmediateInterruptOK,
| * ProcessInterrupts() etc.

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

Just as my two cents, I perfectly agree with you. The code after
it dissapears looks quite cleaner and the less number of states
makes it more understandable.

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

I don't have definite answer for it, but from the point that
stucking or congesting during authentication can raise the number
of immature backend process unboundedly, it might be more
preferable if we can administratively or automatically kill them
for the case.

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

The less tuning point, the more usability, I suppose, as long as
no loss of function.

>    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


regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center