Re: Escaping from blocked send() reprised.

On 2014-09-28 00:54:21 +0200, Andres Freund wrote:
> On 2014-09-27 21:12:43 +0200, Andres Freund wrote:
> > On 2014-09-03 15:09:54 +0300, Heikki Linnakangas wrote:
> > 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.

Kyatoro, could you check whether you can achieve what you want using
0004?

It's imo pretty clear that a fair amount of base work needs to be done
and there's been a fair amount of progress made this fest. I think this
can now be marked returned with feedback.

Greetings,

Andres Freund

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