Re: Escaping from blocked send() reprised.

Wow, thank you for the patch.

> > > 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.

Myself is satisfied by Heikki's solution, and it seems ready for
commit. But I agree with the temporarily blocked state is seen
often and it breaks even non-blocked socket. If we want to/should
avoid breaking *temporarily or not* blocked socket even for
SIGTERM, this mechanism should be used.

Which way should we take?

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center