Re: Overhauling our interrupt handling

Hi,

On 2015-01-15 15:05:08 +0900, Kyotaro HORIGUCHI wrote:
> Hello, I'd synced up this at last.
>
> I think I should finilize my commitfest item for this issue, with
> .. "Rejected"?

Fine with me.

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

I don't think so - we still rely on it unfortunately.

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

Ugh, that's the result stupid last minute "cleanup". You're right.

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

Hm, yes.

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

Yep. As mentioned below, it doesn't use the same infrastructure, so I'd
rather treat this separately. This set is more than big enough.

Thanks for looking!

Greetings,

Andres Freund

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