Re: Escaping from blocked send() reprised. - Mailing list pgsql-hackers

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