Sorry, It tha patch contains a silly bug. Please find the
attatched one.
> 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.
>From 11da4bc3c214490671d27379910a667f06cc35af Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <[email protected]>
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 | 53 ++++++++++++++++++++---------------------
src/include/libpq/libpq.h | 1 +
4 files changed, 44 insertions(+), 30 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..15627c3 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,44 +496,38 @@ 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)
- {
- int save_errno = errno;
+ int save_errno = errno;
- ImmediateInterruptOK = false;
+ ImmediateInterruptOK = false;
- DisableNotifyInterrupt();
- DisableCatchupInterrupt();
+ DisableNotifyInterrupt();
+ DisableCatchupInterrupt();
- errno = save_errno;
- }
+ errno = save_errno;
}
@@ -2594,6 +2588,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 ([email protected])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
