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

On 01/15/2015 03:03 AM, Andres Freund wrote:
> 0002: Use a nonblocking socket for FE/BE communication and block using
>        latches.

s/suceeds/succeeds/

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

> +             * Check for interrupts here, in addition to secure_write(),
> +             * because a interrupted write in secure_raw_write() can possibly
> +             * only return to here, not to secure_write().
>               */
> +            ProcessClientWriteInterrupt(true);

Took me a while to parse that sentence. How about:

Check for interrupts here, in addition to secure_write(), because an 
interrupted write in secure_raw_write() will return here, and we cannot 
return to secure_write() until we've written something.

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

> @@ -799,6 +791,7 @@ ProcKill(int code, Datum arg)
>      proc = MyProc;
>      MyProc = NULL;
>      DisownLatch(&proc->procLatch);
> +    SetLatch(MyLatch);
>
>      SpinLockAcquire(ProcStructLock);
>
> @@ -868,6 +861,7 @@ AuxiliaryProcKill(int code, Datum arg)
>      proc = MyProc;
>      MyProc = NULL;
>      DisownLatch(&proc->procLatch);
> +    SetLatch(MyLatch);
>
>      SpinLockAcquire(ProcStructLock);

These SetLatch calls are unnecessary. SwitchBackToLocalLatch() already 
sets the latch. (According to the comment in SwitchToSharedLatch() even 
that should not be necessary.)

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

In commit message: s/interupts/interrupts/.

> @@ -80,9 +73,6 @@ md5_crypt_verify(const Port *port, const char *role, char *client_pass,
>      if (*shadow_pass == '\0')
>          return STATUS_ERROR;    /* empty password */
>
> -    /* Re-enable immediate response to SIGTERM/SIGINT/timeout interrupts */
> -    ImmediateInterruptOK = true;
> -    /* And don't forget to detect one that already arrived */
>      CHECK_FOR_INTERRUPTS();
>
>      /*

That lone CHECK_FOR_INTERRUPTS() that remains looks pretty randomly 
placed now that the "ImmediateInterruptOK = true" is gone. I would just 
remove it. Add one to the caller if it's needed, but it probably isn't.

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

s/don't/doesn't/ in commit message.

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

Yes.

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

Yeah, I think that's OK. Doing those things with 
ImmediateInterruptOK=true was quite scary, and we need to stop doing 
that. It would be nice to have a wholesale solution for those lookups 
but I don't see one. So all the ident/radius/kerberos/ldap lookups will 
need to be done in a non-blocking way to have them react to the timeout. 
That requires some work, but we can leave that to a followup patch, if 
someone wants to write it.

Overall, 1-8 look good to me, except for that one incomplete comment in 
ProcessClientWriteInterrupt() I mentioned in a separate email. I haven't 
reviewed patches 9 and 10 yet.

- Heikki