Merge lp:~laurynas-biveinis/percona-server/bug805805-1060136-1061118-5.1 into lp:percona-server/5.1

[Laurynas Biveinis]

Fix:
- bug 1060136 (safe_process.cc/safe_process.pl should not kill
  mysqld on SIGSTOP/SIGCONT);
- bug 1061118 (Patch to remove excessive fcntl() calls was never
  ported correctly to 5.1);
- bug 805805 (attaching to percona-server with gdb disconnects
  clients).

These bugs are fixed together because fixing bug 1061118 exposes bug
805805, and the testcase for the latter requires bug 1060136 to be
fixed.

For bug 1060136, the issue is that safe_process.cc is setup to kill
the child on SIGCHLD signal. But this signal may be raised when child
receives non-killing signals too, such as SIGSTOP/SIGCONT, resulting
in safe_process killing the child on such signals too. Fixed by
limiting SIGCHLD only to killing signals, by specifying the
SA_NOCLDSTOP option.

For bug 1061118, the issue is that the fcntl()-removing patch is
dependant on NO_ALARM C preprocessor define being defined. But
nothing in the build defined it. Fixed by adding it to configure.in.
If Percona Server suppported more platforms, a better fix would be to
backport SO_SNDTIMEO/SO_RCVTIMEO checks from 5.5, but at this point in
5.1 lifecycle an unconditional define is enough. In addition to
NO_DEBUG, we also define SIGNAL_WITH_VIO_CLOSE to follow 5.5. If the
latter is not defined, percona_innodb_kill_idle_trx* tests regress by
server returning query execution interrupted error instead of closing
the socket. To avoid regressing on upstream bug
http://bugs.mysql.com/bug.php?id=52633 (When building with -DNO_ALARM
on Linux, wait_timeout is effective 10x longer), backport its
vio_should_retry() too. This same backport is used by the Facebook
5.1 branch.

For bug 805805, the issue is that mysqld (with fcntl()s removed,
i.e. the above fixed) will close the the connections upon receiving
SIGSTOP/SIGCONT. This is caused by network read from socket being
cancelled with errno = EINTR on these signals. Normally this is
handled by my_real_read() retrying the read on EINTR, but the
fcntl()-removal patch has disabled this inadvertently in
my_real_read() and my_real_write(). Added a testcase
percona_signal_handling.

http://jenkins.percona.com/job/percona-server-5.1-param/432/

[Alexey Kopytov (akopytov)]

Looks good except that

1) as I wrote in bug #1061118, I don't think we should bother enabling the feature in our 5.1 binaries and then backporting the fix for http://bugs.mysql.com/bug.php?id=52633 from 5.5

We can keep the fix (and the test case) though. The test case could reference the bug #.

review: Needs Fixing

Unmerged revisions

484. By Laurynas Biveinis on 2012-10-04

Fix:
- bug 1060136 (safe_process.cc/safe_process.pl should not kill
  mysqld on SIGSTOP/SIGCONT);
- bug 1061118 (Patch to remove excessive fcntl() calls was never
  ported correctly to 5.1);
- bug 805805 (attaching to percona-server with gdb disconnects
  clients).

These bugs are fixed together because fixing bug 1061118 exposes bug
805805, and the testcase for the latter requires bug 1060136 to be
fixed.

For bug 1060136, the issue is that safe_process.cc is setup to kill
the child on SIGCHLD signal. But this signal may be raised when child
receives non-killing signals too, such as SIGSTOP/SIGCONT, resulting
in safe_process killing the child on such signals too. Fixed by
limiting SIGCHLD only to killing signals, by specifying the
SA_NOCLDSTOP option.

For bug 1061118, the issue is that the fcntl()-removing patch is
dependant on NO_ALARM C preprocessor define being defined. But
nothing in the build defined it. Fixed by adding it to configure.in.
If Percona Server suppported more platforms, a better fix would be to
backport SO_SNDTIMEO/SO_RCVTIMEO checks from 5.5, but at this point in
5.1 lifecycle an unconditional define is enough. In addition to
NO_DEBUG, we also define SIGNAL_WITH_VIO_CLOSE to follow 5.5. If the
latter is not defined, percona_innodb_kill_idle_trx* tests regress by
server returning query execution interrupted error instead of closing
the socket. To avoid regressing on upstream bug
http://bugs.mysql.com/bug.php?id=52633 (When building with -DNO_ALARM
on Linux, wait_timeout is effective 10x longer), backport its
vio_should_retry() too. This same backport is used by the Facebook
5.1 branch.

For bug 805805, the issue is that mysqld (with fcntl()s removed,
i.e. the above fixed) will close the the connections upon receiving
SIGSTOP/SIGCONT. This is caused by network read from socket being
cancelled with errno = EINTR on these signals. Normally this is
handled by my_real_read() retrying the read on EINTR, but the
fcntl()-removal patch has disabled this inadvertently in
my_real_read() and my_real_write(). Added a testcase
percona_signal_handling.

=== modified file 'Percona-Server/configure.in'
--- Percona-Server/configure.in	2012-08-20 03:14:02 +0000
+++ Percona-Server/configure.in	2012-10-04 16:16:24 +0000
@@ -2894,6 +2894,12 @@
 AC_SUBST(CC)
 AC_SUBST(GXX)
 
+# Fix Percona Server bug https://bugs.launchpad.net/percona-server/+bug/1061118
+# A complete fix would be to backport SO_SNDTIMEO/SO_RCVTIMEO tests from 5.5, but 
+# this fix is good enough for us.
+AC_DEFINE([NO_ALARM], [], [Disable alarm-based timeouts in network I/O])
+AC_DEFINE([SIGNAL_WITH_VIO_CLOSE], [], [Close VIO of the thread being killed])
+
 # Set configuration options for make_binary_distribution
 case $SYSTEM_TYPE in
   *netware*)

=== modified file 'Percona-Server/mysql-test/lib/My/SafeProcess/safe_process.cc'
--- Percona-Server/mysql-test/lib/My/SafeProcess/safe_process.cc	2011-06-30 15:37:13 +0000
+++ Percona-Server/mysql-test/lib/My/SafeProcess/safe_process.cc	2012-10-04 16:16:24 +0000
@@ -153,11 +153,14 @@
   pid_t own_pid= getpid();
   pid_t parent_pid= getppid();
   bool nocore = false;
+  struct sigaction sigchld_action;
 
+  sigchld_action.sa_handler= handle_signal;
+  sigchld_action.sa_flags= SA_NOCLDSTOP;
   /* Install signal handlers */
   signal(SIGTERM, handle_signal);
   signal(SIGINT,  handle_signal);
-  signal(SIGCHLD, handle_signal);
+  sigaction(SIGCHLD, &sigchld_action, NULL);
   signal(SIGABRT, handle_abort);
 
   sprintf(safe_process_name, "safe_process[%ld]", (long) own_pid);

=== added file 'Percona-Server/mysql-test/r/percona_signal_handling.result'
--- Percona-Server/mysql-test/r/percona_signal_handling.result	1970-01-01 00:00:00 +0000
+++ Percona-Server/mysql-test/r/percona_signal_handling.result	2012-10-04 16:16:24 +0000
@@ -0,0 +1,3 @@
+SELECT 2+2;
+2+2
+4

=== added file 'Percona-Server/mysql-test/t/percona_signal_handling.test'
--- Percona-Server/mysql-test/t/percona_signal_handling.test	1970-01-01 00:00:00 +0000
+++ Percona-Server/mysql-test/t/percona_signal_handling.test	2012-10-04 16:16:24 +0000
@@ -0,0 +1,8 @@
+--source include/not_windows.inc
+
+let $mysqld_pid_file=`SELECT @@GLOBAL.pid_file`;
+
+system kill -STOP `cat $mysqld_pid_file`;
+system kill -CONT `cat $mysqld_pid_file`;
+
+SELECT 2+2;

=== modified file 'Percona-Server/sql/net_serv.cc'
--- Percona-Server/sql/net_serv.cc	2011-11-24 02:01:19 +0000
+++ Percona-Server/sql/net_serv.cc	2012-10-04 16:16:24 +0000
@@ -617,11 +617,12 @@
     if ((long) (length= vio_write(net->vio,pos,(size_t) (end-pos))) <= 0)
     {
       my_bool interrupted = vio_should_retry(net->vio);
-#if !defined(NO_ALARM) && !defined(__WIN__)
+#if !defined(__WIN__)
       if ((interrupted || length == 0) && !thr_alarm_in_use(&alarmed))
       {
         if (!thr_alarm(&alarmed, net->write_timeout, &alarm_buff))
         {                                       /* Always true for client */
+#if !defined(NO_ALARM)
 	  my_bool old_mode;
 	  while (vio_blocking(net->vio, TRUE, &old_mode) < 0)
 	  {
@@ -639,6 +640,7 @@
 #endif
 	    goto end;
 	  }
+#endif /* !defined(NO_ALARM) */
 	  retry_count=0;
 	  continue;
 	}
@@ -820,7 +822,7 @@
 
 	  DBUG_PRINT("info",("vio_read returned %ld  errno: %d",
 			     (long) length, vio_errno(net->vio)));
-#if !defined(NO_ALARM) && (!defined(__WIN__) || defined(MYSQL_SERVER))
+#if !defined(__WIN__) || defined(MYSQL_SERVER)
 	  /*
 	    We got an error that there was no data on the socket. We now set up
 	    an alarm to not 'read forever', change the socket to non blocking
@@ -830,6 +832,7 @@
 	  {
 	    if (!thr_alarm(&alarmed,net->read_timeout,&alarm_buff)) /* Don't wait too long */
 	    {
+#if !defined(NO_ALARM)
 	      my_bool old_mode;
 	      while (vio_blocking(net->vio, TRUE, &old_mode) < 0)
 	      {
@@ -852,6 +855,7 @@
 #endif
 		goto end;
 	      }
+#endif /* !defined(NO_ALARM) */
 	      retry_count=0;
 	      continue;
 	    }

=== modified file 'Percona-Server/vio/viosocket.c'
--- Percona-Server/vio/viosocket.c	2012-02-17 19:02:17 +0000
+++ Percona-Server/vio/viosocket.c	2012-10-04 16:16:24 +0000
@@ -250,11 +250,20 @@
 
 
 my_bool
-vio_should_retry(Vio * vio __attribute__((unused)))
+vio_should_retry(Vio * vio)
 {
   int en = socket_errno;
-  return (en == SOCKET_EAGAIN || en == SOCKET_EINTR ||
-	  en == SOCKET_EWOULDBLOCK);
+  /*
+    man 2 read write
+    EAGAIN or EWOULDBLOCK when a socket is a non-blocking mode means
+    that the read/write would block.
+    man 7 socket
+    EAGAIN or EWOULDBLOCK when a socket is in a blocking mode means
+    that the corresponding receiving or sending timeout was reached.
+  */
+  return  en == SOCKET_EINTR ||
+          (!vio_is_blocking(vio) &&
+            (en == SOCKET_EAGAIN || en == SOCKET_EWOULDBLOCK));
 }