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