>From 3e9bc8fdd7a68f8e16e88860a8f305c72baf66e9 Mon Sep 17 00:00:00 2001 From: Andres Freund Date: Sun, 28 Sep 2014 00:22:39 +0200 Subject: [PATCH 04/10] Process 'die' interrupts while reading/writing from the client socket. Up to now it was impossible to terminate a backend that was trying to send data to the client when the socket's buffer was already full. While the send() call itself might have gotten interrupted by signals on some platforms, we just immediately retried. Change things so that that we process 'die' events immediately when the appropriate signal arrives during both recv/send. We can't react to query cancels at that point, because we might loose sync with the client as we could be in the middle of writing a message. We don't interrupt writes if the write buffer isn't full as that would lead to far fewer error messages reaching clients. Per discussion with Kyotaro HORIGUCHI and Heikki Linnakangas Disccussion: 20140927191243.GD5423@alap3.anarazel.de --- src/backend/libpq/be-secure-openssl.c | 7 ++++-- src/backend/libpq/be-secure.c | 37 +++++++++++++++------------- src/backend/tcop/postgres.c | 46 +++++++++++++++++++++++++++++++++++ src/include/tcop/tcopprot.h | 1 + 4 files changed, 72 insertions(+), 19 deletions(-) diff --git a/src/backend/libpq/be-secure-openssl.c b/src/backend/libpq/be-secure-openssl.c index 3a70f43..3f7680f 100644 --- a/src/backend/libpq/be-secure-openssl.c +++ b/src/backend/libpq/be-secure-openssl.c @@ -634,10 +634,13 @@ wloop: /* Don't retry if the socket is in nonblocking mode. */ if (port->noblock) break; + /* - * XXX: We'll, at some later point, likely want to add interrupt - * processing here. + * Check for interrupts here, in addition to secure_write(), + * because a interrupted write in secure_raw_write() can possibly + * only return to here, not to secure_write(). */ + ProcessClientWriteInterrupt(true); goto wloop; case SSL_ERROR_SYSCALL: /* leave it to caller to ereport the value of errno */ diff --git a/src/backend/libpq/be-secure.c b/src/backend/libpq/be-secure.c index f96b8f3..cf0709a 100644 --- a/src/backend/libpq/be-secure.c +++ b/src/backend/libpq/be-secure.c @@ -224,18 +224,17 @@ retry: n = secure_raw_write(port, ptr, len); } - /* - * XXX: We'll, at some later point, likely want to add interrupt - * processing here. - */ - - /* - * Retry after processing interrupts. This can be triggered even though we - * don't check for latch set's during writing yet, because SSL - * renegotiations might have required reading from the socket. - */ + /* retry after processing interrupts */ if (n < 0 && errno == EINTR) { + /* + * Process interrupts that happened while (or before) writing. We only + * process interrupts if we got interrupted while writing. In other + * cases it's better to allow the interrupts to be handled at higher + * layers. + */ + ProcessClientWriteInterrupt(!port->noblock); + goto retry; } @@ -262,16 +261,20 @@ wloop: int w; int save_errno = errno; - /* - * XXX: We'll, at some later point, likely want to add interrupt - * processing here. If set we'd not retry directly, but return. That - * way we don't do anything while (possibly) inside a ssl library. - */ w = WaitLatchOrSocket(MyLatch, - WL_SOCKET_WRITEABLE, + WL_LATCH_SET | WL_SOCKET_WRITEABLE, port->sock, 0); - if (w & WL_SOCKET_WRITEABLE) + if (w & WL_LATCH_SET) + { + ResetLatch(MyLatch); + /* + * Force a return, so interrupts can be processed when not + * (possibly) underneath a ssl library. + */ + errno = EINTR; + } + else if (w & WL_SOCKET_WRITEABLE) { goto wloop; } diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c index fa1aeb5..75ec53e 100644 --- a/src/backend/tcop/postgres.c +++ b/src/backend/tcop/postgres.c @@ -520,10 +520,56 @@ ProcessClientReadInterrupt(void) if (notifyInterruptPending) ProcessNotifyInterrupt(); } + else if (ProcDiePending) + { + /* + * We're dying. It's safe (and sane) to handle that now. + */ + CHECK_FOR_INTERRUPTS(); + } errno = save_errno; } +/* + * ProcessClientWriteInterrupt() - Process interrupts specific to client writes + * + * This is called just after low-level writes. That might be after the read + * finished successfully, or it was interrupted via interrupt. 'blocked' tells + * us whether the + * + * Must preserve errno! + */ +void +ProcessClientWriteInterrupt(bool blocked) +{ + int save_errno = errno; + + Assert(InterruptHoldoffCount == 0 && CritSectionCount == 0); + + /* + * We only want to process the interrupt here if socket writes are + * blocking to increase the chance to get an error message to the + * client. If we're not blocked there'll soon be a + * CHECK_FOR_INTERRUPTS(). But if we're blocked we'll never get out of + * that situation if the client has died. + */ + if (ProcDiePending && blocked) + { + /* + * We're dying. It's safe (and sane) to handle that now. But we don't + * want to send the client the error message as that a) would possibly + * block again b) would possibly lead to sending an error message to + * the client, while we already started to send something else. + */ + if (whereToSendOutput == DestRemote) + whereToSendOutput = DestNone; + + CHECK_FOR_INTERRUPTS(); + } + + errno = save_errno; +} /* * Do raw parsing (only). diff --git a/src/include/tcop/tcopprot.h b/src/include/tcop/tcopprot.h index fe8c725..fc04a3e 100644 --- a/src/include/tcop/tcopprot.h +++ b/src/include/tcop/tcopprot.h @@ -68,6 +68,7 @@ extern void FloatExceptionHandler(SIGNAL_ARGS) __attribute__((noreturn)); extern void RecoveryConflictInterrupt(ProcSignalReason reason); /* called from SIGUSR1 * handler */ extern void ProcessClientReadInterrupt(void); +extern void ProcessClientWriteInterrupt(bool blocked); extern void process_postgres_switches(int argc, char *argv[], GucContext ctx, const char **dbname); -- 2.0.0.rc2.4.g1dc51c6.dirty