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
diff --git a/src/backend/libpq/be-secure.c b/src/backend/libpq/be-secure.c
index 41ec1ad..049e5b1 100644
--- a/src/backend/libpq/be-secure.c
+++ b/src/backend/libpq/be-secure.c
@@ -178,5 +178,13 @@ secure_write(Port *port, void *ptr, size_t len)
ssize_t
secure_raw_write(Port *port, const void *ptr, size_t len)
{
- return send(port->sock, ptr, len, 0);
+ ssize_t result;
+
+ prepare_for_client_write();
+
+ result = send(port->sock, ptr, len, 0);
+
+ client_write_ended();
+
+ return result;
}
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index 61f17bf..138060b 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -497,6 +497,17 @@ ReadCommand(StringInfo inBuf)
* non-reentrant libc functions. This restriction makes it safe for us
* to allow interrupt service routines to execute nontrivial code while
* we are waiting for input.
+ *
+ * When waiting in the main loop, we can process any interrupt immediately
+ * in the signal handler. In any other read from the client, like in a COPY
+ * FROM STDIN, we can't safely process a query cancel signal, because we might
+ * be in the middle of sending a message to the client, and jumping out would
+ * violate the protocol. Or rather, pqcomm.c would detect it and refuse to
+ * send any more messages to the client. But handling a SIGTERM is OK, because
+ * we're terminating the backend and don't need to send any more messages
+ * anyway. That means that we might not be able to send an error message to
+ * the client, but that seems better than waiting indefinitely, in case the
+ * client is not responding.
*/
void
prepare_for_client_read(void)
@@ -513,6 +524,15 @@ prepare_for_client_read(void)
/* And don't forget to detect one that already arrived */
CHECK_FOR_INTERRUPTS();
}
+ else
+ {
+ /* Allow die interrupts to be processed while waiting */
+ ImmediateDieOK = true;
+
+ /* And don't forget to detect one that already arrived */
+ if (ProcDiePending)
+ CHECK_FOR_INTERRUPTS();
+ }
}
/*
@@ -534,6 +554,35 @@ client_read_ended(void)
errno = save_errno;
}
+ else
+ ImmediateDieOK = false;
+}
+
+/*
+ * prepare_for_client_write -- set up to possibly block on client output
+ *
+ * Like prepare_client_read, but for writing.
+ */
+void
+prepare_for_client_write(void)
+{
+ /* Allow die interrupts to be processed while waiting */
+ ImmediateDieOK = true;
+
+ /* And don't forget to detect one that already arrived */
+ if (ProcDiePending)
+ CHECK_FOR_INTERRUPTS();
+}
+
+/*
+ * client_read_ended -- get out of the client-output state
+ *
+ * This is called just after low-level writes.
+ */
+void
+client_write_ended(void)
+{
+ ImmediateDieOK = false;
}
@@ -2591,8 +2640,8 @@ die(SIGNAL_ARGS)
* If it's safe to interrupt, and we're waiting for input or a lock,
* service the interrupt immediately
*/
- if (ImmediateInterruptOK && InterruptHoldoffCount == 0 &&
- CritSectionCount == 0)
+ if ((ImmediateInterruptOK || ImmediateDieOK) &&
+ InterruptHoldoffCount == 0 && CritSectionCount == 0)
{
/* bump holdoff count to make ProcessInterrupts() a no-op */
/* until we are done getting ready for it */
@@ -2792,8 +2841,8 @@ RecoveryConflictInterrupt(ProcSignalReason reason)
* If it's safe to interrupt, and we're waiting for input or a lock,
* service the interrupt immediately
*/
- if (ImmediateInterruptOK && InterruptHoldoffCount == 0 &&
- CritSectionCount == 0)
+ if ((ImmediateInterruptOK || (ImmediateDieOK && ProcDiePending)) &&
+ InterruptHoldoffCount == 0 && CritSectionCount == 0)
{
/* bump holdoff count to make ProcessInterrupts() a no-op */
/* until we are done getting ready for it */
diff --git a/src/backend/utils/init/globals.c b/src/backend/utils/init/globals.c
index be74835..24523f9 100644
--- a/src/backend/utils/init/globals.c
+++ b/src/backend/utils/init/globals.c
@@ -31,6 +31,7 @@ volatile bool QueryCancelPending = false;
volatile bool ProcDiePending = false;
volatile bool ClientConnectionLost = false;
volatile bool ImmediateInterruptOK = false;
+volatile bool ImmediateDieOK = false;
volatile uint32 InterruptHoldoffCount = 0;
volatile uint32 CritSectionCount = 0;
diff --git a/src/include/miscadmin.h b/src/include/miscadmin.h
index 2ba9885..9528936 100644
--- a/src/include/miscadmin.h
+++ b/src/include/miscadmin.h
@@ -81,6 +81,7 @@ extern volatile bool ClientConnectionLost;
/* these are marked volatile because they are examined by signal handlers: */
extern PGDLLIMPORT volatile bool ImmediateInterruptOK;
+extern PGDLLIMPORT volatile bool ImmediateDieOK;
extern PGDLLIMPORT volatile uint32 InterruptHoldoffCount;
extern PGDLLIMPORT volatile uint32 CritSectionCount;
diff --git a/src/include/tcop/tcopprot.h b/src/include/tcop/tcopprot.h
index 60f7532..c288bdd 100644
--- a/src/include/tcop/tcopprot.h
+++ b/src/include/tcop/tcopprot.h
@@ -69,6 +69,8 @@ extern void RecoveryConflictInterrupt(ProcSignalReason reason); /* called from S
* handler */
extern void prepare_for_client_read(void);
extern void client_read_ended(void);
+extern void prepare_for_client_write(void);
+extern void client_write_ended(void);
extern void process_postgres_switches(int argc, char *argv[],
GucContext ctx, const char **dbname);
extern void PostgresMain(int argc, char *argv[],
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
