Re: Escaping from blocked send() reprised.

Hi,

On 2014-10-03 17:12:18 +0300, Heikki Linnakangas wrote:
> On 09/28/2014 01:54 AM, Andres Freund wrote:
> >I've invested some more time in this:
> 
> Thanks!
> 
> In 0001, the select() codepath will not return (WL_SOCKET_READABLE |
> WL_SOCKET_WRITEABLE) on EOF or error, like the comment says and like the
> poll() path does. It only sets WL_SOCKET_READABLE if WL_SOCKET_READABLE was
> requested as a wake-event, and likewise for writeable, while the poll()
> codepath returns (WL_SOCKET_READABLE | WL_SOCKET_WRITEABLE) regardless of
> the requested wake-events. I'm not sure which is actually better - a
> separate WL_SOCKET_ERROR code might be best - but it's inconsistent as it
> is.

Hm. Right. I think we should only report the requested state. We can't
really discern wether it's a hangup, error or actually readable/writable
with select() - it just returns the socket as readable/writable as soon
as it doesn't block anymore. Where not blocking includes the connection
having gone bad.

It took me a while to figure out whether that's actually guaranteed by
the spec, but I'm pretty sure it is...

> >0002 now makes sense on its own and doesn't change anything around the
> >      interrupt handling. Oh, and it compiles without 0003.
> 
> WaitLatchOrSocket() can throw an error, so it's not totally safe to call
> that underneath OpenSSL.

Hm. Fair point.

> Admittedly the cases where it throws an error are
> "shouldn't happen" cases like "poll() failed" or "read() on self-pipe
> failed", but still. Perhaps those errors should be reclassified as FATAL;
> it's not clear you can just roll back and expect to continue running if any
> of them happens.

Fine with me.

> In secure_raw_write(), you need to save and restore errno, as
> WaitLatchOrSocket will not preserve it. If secure_raw_write() calls
> WaitLatchOrSocket(), and it returns because the latch was set, and we fall
> out of secure_raw_write, we will return -1 but the errno might not be set to
> anything sensible anymore.

Oops.

> >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.
> 
> Yeah, 1-3 seem sane.

I think 3 also needs a careful look. Have you looked through it? While
imo much less complex than before, there's some complex interactions in
the touched code. And we have terrible coverage of both catchup
interrupts and notify stuff...

Tom, do you happen to have time to look at that bit?


There's also the concern that using a latch for client communication
increases the number of syscalls for the same work. We should at least
try to quantify that...

> 4 also looks OK to me at a quick glance. It basically
> enables handling the "die" interrupt immediately, if we're blocked in a read
> or write. It won't be handled in the signal handler, but within the
> secure_read/write call anyway.

What are you thinking about the concern that it'll reduce the likelihood
of transferring the error message to the client? I tried to reduce that
by only allowing errors when write() blocks, but that's not an
infrequent event.

Thanks for looking.

Greetings,

Andres Freund

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