Re: Escaping from blocked send() reprised.

On 2014-09-27 21:12:43 +0200, Andres Freund wrote:
> On 2014-09-03 15:09:54 +0300, Heikki Linnakangas wrote:
> > On 09/03/2014 12:23 AM, Andres Freund wrote:
> > >On 2014-09-02 17:21:03 -0400, Tom Lane wrote:
> > >>Heikki Linnakangas <hlinnakangas@vmware.com> writes:
> > >>>I was going to suggest using WaitLatchOrSocket instead of sleeping in 1
> > >>>second increment, but I see that WaitLatchOrSocket() doesn't currently
> > >>>support waiting for a socket to become writeable, without also waiting
> > >>>for it to become readable. I wonder how difficult it would be to lift
> > >>>that restriction.
> > >>
> > >>My recollection is that there was a reason for that, but I don't recall
> > >>details any more.
> > >
> > >http://git.postgresql.org/pg/commitdiff/e42a21b9e6c9b9e6346a34b62628d48ff2fc6ddf
> > >
> > >In my prototype I've changed the API that errors set both
> > >READABLE/WRITABLE. Seems to work....
> >
> > Andres, would you mind posting the WIP patch you have? That could be a
> > better foundation for this patch.
>
> Sorry, I missed this message and only cought up when reading your CF
> status mail. I've attached three patches:
>
> 0001: Allows WaitLatchOrSocket(WL_WRITABLE) without WL_READABLE. I've
>       tested the poll() and select() implementations on linux and
>       blindly patched up windows.
> 0002: Put the socket the backend uses to communicate with the client
>       into nonblocking mode as soon as latches are ready and use latches
>       to wait. This probably doesn't work correctly without 0003, but
>       seems easier to review separately.
> 0003: Don't do sinval catchup and notify processing in signal
>       handlers. It's quite cool that it worked that well so far, but it
>       requires some complicated code and is rather fragile. 0002 allows
>       to move that out of signal handlers and just use a latch
>       there. This seems remarkably simpler:
>        4 files changed, 69 insertions(+), 229 deletions(-)
>
> These aren't ready for commit, especially not 0003, but I think they are
> quite a good foundation for getting rid of the blocking in send(). I
> haven't added any interrupt processing after interrupted writes, but
> marked the relevant places with XXXs.
>
> With regard to 0002, I dislike the current need to do interrupt
> processing both in be-secure.c and be-secure-openssl.c. I guess we could
> solve that by returning something like EINTR from the ssl routines when
> they need further reads/writes and do all the processing in one place in
> be-secure.c.
>
> There's also some cleanup in 0002/0003 needed:
> prepare_for_client_read()/client_read_ended() aren't needed in that form
> anymore and should probably rather be something like
> CHECK_FOR_READ_INTERRUPT() or similar.  Similarly the
> EnableCatchupInterrupt()/DisableCatchupInterrupt() in autovacuum.c is
> pretty ugly.
>
> Btw, be-secure.c is really not a good name anymore...
>
> What do you think?

I've invested some more time in this:
0002 now makes sense on its own and doesn't change anything around the
     interrupt handling. Oh, and it compiles without 0003.
0003 Sinval/notify processing got simplified further. There really isn't
     any need for DisableNotifyInterrupt/DisableCatchupInterrupt
     anymore. Also begin_client_read/client_read_ended don't make much
     sense anymore. Instead introduce ProcessClientReadInterrupt (which
     wants a better name).
There's also a very WIP
0004 Allows secure_read/write be interrupted when ProcDiePending is
     set. All of that happens via the latch mechanism, nothing happens
     inside signal handlers. So I do think it's quite an improvement
     over what's been discussed in this thread.
     But it (and the other approaches) do noticeably increase the
     likelihood of clients not getting the error message if the client
     isn't actually dead. The likelihood of write() being blocked
     *temporarily* due to normal bandwidth constraints is quite high
     when you consider COPY FROM and similar. Right now we'll wait till
     we can put the error message into the socket afaics.

1-3 need some serious comment work, but I think the approach is
basically sound. I'm much, much less sure about allowing send() to be
interrupted.

Greetings,

Andres Freund

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