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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
