Hello, attached is the new-and-far-simple version of this
patch. It no longer depends on win32 nonblocking patch since the
socket is in blocking mode again.
> On 08/28/2014 03:47 PM, Kyotaro HORIGUCHI wrote:
> > - Preventing protocol violation.
> >
> > To prevent protocol violation, secure_write sets
> > ClientConnectionLost when SIGTERM detected, then
> > internal_flush() and ProcessInterrupts() follow the
> > instruction.
>
> Oh, hang on. Now that I look at pqcomm.c more closely, it already has
> a mechanism to avoid writing a message in the middle of another
> message. See pq_putmessage and PqCommBusy. Can we rely on that?
Hmm, it gracefully returns up to ExecProcNode() and PqCommBusy is
turned off on the way at pq_putmessage() under current
implement. So PqCommBusy is already false before it runs into
ProcessInterrupts().
Allowing ImmediateInterruptOK on signalled during send(), setting
whereToSendOutput to DestNone if PqCommBusy is true will do. We
can also distinguish read and write by looking
DoingCommandRead. The ImmediateInterruptOK section can be defined
enclosing by prepare_for_client_read/client_read_end.
> > - Single pg_terminate_backend surely kills the backend.
> >
> > secure_raw_write() uses non-blocking socket and a loop of
> > select() with timeout to surely detects received
> > signal(SIGTERM).
>
> I was going to suggest using WaitLatchOrSocket instead of sleeping in
> 1 second increment, but I see that WaitLatchOrSocket() doesn't
> currently support waiting for a socket to become writeable, without
> also waiting for it to become readable. I wonder how difficult it
> would be to lift that restriction.
It seems quite difficult hearing the following discussion.
> I also wonder if it would be simpler to keep the socket in blocking
> mode after all, and just close() in the signal handler if PqCommBusy
> == true. If the signal arrives while sleeping in send(), the effect
> would be the same as with your patch. If the signal arrives while
> sending, but not sleeping, you would not get the chance to send the
> already-buffered data to the client. But maybe that's OK, whether or
> not you're blocked is not very deterministic anyway.
Hmm. We're back round to the my first patch, with immediately
close the socket, and became irrelevant to win32 layer
patch. Anyway, it sounds reasonable.
Attached patch is a quick hack patch, but it seems working as
expected at a glance.
regards,
--
Kyotaro Horiguchi
NTT Open Source Software Center
>From 7fcb6ef2e66231605e49bd51cd09d275b40cfd57 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horiguchi.kyot...@lab.ntt.co.jp>
Date: Fri, 5 Sep 2014 17:21:48 +0900
Subject: [PATCH] Simplly cutting off the socket if signalled during sending to client.
---
src/backend/libpq/be-secure.c | 14 +++++++++++---
src/backend/libpq/pqcomm.c | 6 ++++++
src/backend/tcop/postgres.c | 40 +++++++++++++++++++++-------------------
src/include/libpq/libpq.h | 1 +
4 files changed, 39 insertions(+), 22 deletions(-)
diff --git a/src/backend/libpq/be-secure.c b/src/backend/libpq/be-secure.c
index 41ec1ad..329812b 100644
--- a/src/backend/libpq/be-secure.c
+++ b/src/backend/libpq/be-secure.c
@@ -145,11 +145,11 @@ secure_raw_read(Port *port, void *ptr, size_t len)
{
ssize_t n;
- prepare_for_client_read();
+ prepare_for_client_comm();
n = recv(port->sock, ptr, len, 0);
- client_read_ended();
+ client_comm_ended();
return n;
}
@@ -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 n;
+
+ prepare_for_client_comm();
+
+ n = send(port->sock, ptr, len, 0);
+
+ client_comm_ended();
+
+ return n;
}
diff --git a/src/backend/libpq/pqcomm.c b/src/backend/libpq/pqcomm.c
index 605d891..8f84f67 100644
--- a/src/backend/libpq/pqcomm.c
+++ b/src/backend/libpq/pqcomm.c
@@ -1342,6 +1342,12 @@ pq_is_send_pending(void)
return (PqSendStart < PqSendPointer);
}
+bool
+pq_is_busy(void)
+{
+ return PqCommBusy;
+}
+
/* --------------------------------
* Message-level I/O routines begin here.
*
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index 7b5480f..7a4c483 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -303,16 +303,16 @@ InteractiveBackend(StringInfo inBuf)
*
* Even though we are not reading from a "client" process, we still want to
* respond to signals, particularly SIGTERM/SIGQUIT. Hence we must use
- * prepare_for_client_read and client_read_ended.
+ * prepare_for_client_comm and client_comm_ended.
*/
static int
interactive_getc(void)
{
int c;
- prepare_for_client_read();
+ prepare_for_client_comm();
c = getc(stdin);
- client_read_ended();
+ client_comm_ended();
return c;
}
@@ -487,7 +487,7 @@ ReadCommand(StringInfo inBuf)
}
/*
- * prepare_for_client_read -- set up to possibly block on client input
+ * prepare_for_client_comm -- set up to possibly block on client communication
*
* This must be called immediately before any low-level read from the
* client connection. It is necessary to do it at a sufficiently low level
@@ -496,32 +496,29 @@ ReadCommand(StringInfo inBuf)
* In particular there mustn't be use of malloc() or other potentially
* 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.
+ * we are waiting for input or blocking of output.
*/
void
-prepare_for_client_read(void)
+prepare_for_client_comm(void)
{
- if (DoingCommandRead)
- {
- /* Enable immediate processing of asynchronous signals */
- EnableNotifyInterrupt();
- EnableCatchupInterrupt();
+ /* Enable immediate processing of asynchronous signals */
+ EnableNotifyInterrupt();
+ EnableCatchupInterrupt();
- /* Allow cancel/die interrupts to be processed while waiting */
- ImmediateInterruptOK = true;
+ /* Allow cancel/die interrupts to be processed while waiting */
+ ImmediateInterruptOK = true;
- /* And don't forget to detect one that already arrived */
- CHECK_FOR_INTERRUPTS();
- }
+ /* And don't forget to detect one that already arrived */
+ CHECK_FOR_INTERRUPTS();
}
/*
- * client_read_ended -- get out of the client-input state
+ * client_comm_ended -- get out of the client-communicating state
*
- * This is called just after low-level reads. It must preserve errno!
+ * This is called just after low-level reads/writes. It must preserve errno!
*/
void
-client_read_ended(void)
+client_comm_ended(void)
{
if (DoingCommandRead)
{
@@ -2594,6 +2591,11 @@ die(SIGNAL_ARGS)
if (ImmediateInterruptOK && InterruptHoldoffCount == 0 &&
CritSectionCount == 0)
{
+ if (pq_is_busy() && !DoingCommandRead)
+ {
+ close(MyProcPort->sock);
+ whereToSendOutput = DestNone;
+ }
/* bump holdoff count to make ProcessInterrupts() a no-op */
/* until we are done getting ready for it */
InterruptHoldoffCount++;
diff --git a/src/include/libpq/libpq.h b/src/include/libpq/libpq.h
index 5da9d8d..c3fc5f3 100644
--- a/src/include/libpq/libpq.h
+++ b/src/include/libpq/libpq.h
@@ -62,6 +62,7 @@ extern int pq_putbytes(const char *s, size_t len);
extern int pq_flush(void);
extern int pq_flush_if_writable(void);
extern bool pq_is_send_pending(void);
+extern bool pq_is_busy(void);
extern int pq_putmessage(char msgtype, const char *s, size_t len);
extern void pq_putmessage_noblock(char msgtype, const char *s, size_t len);
extern void pq_startcopyout(void);
--
1.7.1
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
