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