Re: Escaping from blocked send() reprised.

On 10/03/2014 06:29 PM, Heikki Linnakangas wrote:
> On 10/03/2014 05:26 PM, Andres Freund wrote:
>> On 2014-10-03 17:12:18 +0300, Heikki Linnakangas wrote:
>>> On 09/28/2014 01:54 AM, Andres Freund wrote:
>>>> 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...
>
> I only looked at the .patch, I didn't apply it, so I didn't look at the
> context much. But I don't see any fundamental problem with it. I would
> like to have a closer look before it's committed, though.

About patch 3:

Looking closer, this design still looks OK to me. As you said yourself,
the comments need some work (e.g. the step 5. in the top comment in
async.c needs updating). And then there are a couple of FIXME and XXX
comments that need to be addressed.

The comment on PGPROC.procLatch in storage/proc.h says just this:

> Latch procLatch; /* generic latch for process */

This needs a lot more explaining. It's now used by signal handlers to
interrupt a read or write to the socket; that should be mentioned. What
else is it used for? (for waking up a backend in synchronous
replication, at least) What are the rules on when to arm it and when to
reset it?

Would it be more clear to use a separate, backend-private, latch, for
the signals? I guess that won't work, though, because sometimes we need
need to wait for a wakeup from a different process or from a signal at
the same time (SyncRepWaitForLSN() in particular). Not without adding a
variant of WaitLatch that can wait on two latches simultaneously, anyway.

The assumption in secure_raw_read that MyProc exists is pretty
surprising. I understand why it's that way, and there's a comment in
PostgresMain explaining why the socket cannot be put into non-blocking
mode earlier, but it's still a bit whacky. Not sure what to do about that.

- Heikki