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

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

Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:

Reply via email to