Re: Escaping from blocked send() reprised.

On 09/09/2014 01:31 PM, Kyotaro HORIGUCHI wrote:
> Hi, I added and edited some comments.
>
>> Sorry, It tha patch contains a silly bug. Please find the
>> attatched one.

I must say this scares the heck out of me. The current code goes through 
some trouble to not throw an error while in a recv() send(). For 
example, you removed the DoingCommandRead check from 
prepare_for_client_read(). There's an comment in postgres.c that says this:

> 		/*
> 		 * (2) Allow asynchronous signals to be executed immediately if they
> 		 * come in while we are waiting for client input. (This must be
> 		 * conditional since we don't want, say, reads on behalf of COPY FROM
> 		 * STDIN doing the same thing.)
> 		 */
> 		DoingCommandRead = true;

With the patch, we do allow asynchronous signals to be processed while 
blocked in COPY FROM STDIN. Maybe that's OK, but I don't feel 
comfortable just changing it. (the comment is now wrong, of course)

This patch also enables processing query cancel signals while blocked, 
not just SIGTERM. That's not good; we might be in the middle of sending 
a message, and we cannot just error out of that or we might violate the 
fe/be protocol. That's OK with a SIGTERM as you're terminating the 
connection anyway, and we have the PqCommBusy safeguard in place that 
prevents us from sending broken messages to the client, but that's not 
good enough if we wanted to keep the backend alive, as we won't be able 
to send anything to the client anymore.

BTW, we've been talking about blocking in send(), but this patch also 
let's a recv() in e.g. COPY FROM STDIN to be interrupted. That's 
probably a good thing; surely you have exactly the same issues with that 
as with send(). But I didn't realize we had a problem with that too.


In summary, this patch is not ready as it is, but I think we can fix it. 
The key question is: is it safe to handle SIGTERM in the signal handler, 
calling the exit-handlers and exiting the backend, when blocked in a 
recv() or send()? It's a change in the pqcomm.c API; most pqcomm.c 
functions have not thrown errors or processed interrupts before. But 
looking at the callers, I think it's safe, and there isn't actually any 
comments explicitly saying that pqcomm.c will never throw errors.

I propose the attached patch. It adds a new flag ImmediateDieOK, which 
is a weaker form of ImmediateInterruptOK that only allows handling a 
pending die-signal in the signal handler.

Robert, others, do you see a problem with this?

Over IM, Robert pointed out that it's not safe to jump out of a signal 
handler with siglongjmp, when we're inside library calls, like in a 
callback called by OpenSSL. But even with current master branch, that's 
exactly what we do. In secure_raw_read(), we set ImmediateInterruptOK = 
true, which means that any incoming signal will be handled directly in 
the signal handler, which can mean elog(ERROR). Should we be worried? 
OpenSSL might get confused if control never returns to the SSL_read() or 
SSL_write() function that called secure_raw_read().

- Heikki