Merge lp:~sergei.glushchenko/percona-server/5.6-ps-bug1194097 into lp:percona-server/5.6

[Sergei Glushchenko]

This patch fixes multiple issues.
- assertion failure (pfs->m_idle || (state == PSI_SOCKET_STATE_IDLE)) in pfs.cc
PFS updates of current state of socket in threadpool has been improved.
* as comment in threadpool_common.cc states, there is a possibility for
more than one iteration of loop in threadpool_process_request to be done,
which is handled now. Cases when loop wasn't executed at all are also
handled.
* change in mysqld.cc is aimed to prevent race condition (concurrent changes
in THD and it's PFS structures) which caused SIGSEGV's on server shutdown.
* adding vio_cancel for socket and other VIO implementations allows to
handle kill idle transaction correctly. Similar change present in 5.5
branch Percona Server rev. 442.1.4.

http://jenkins.percona.com/view/PS%205.6/job/percona-server-5.6-param/196/

Build is run with parameters using threadpool and maximum pool size of 50
threads. Why default value of 500 threads cannot be used in our Jenkins
farm will be explained below.
Results are pretty good. Except for several sporadic failures of slow log
related tests there are two constantly failing tests which expect maximum
pool size isn't set explicitly:
- pool_of_threads
- thread_pool_max_threads_basic
Tests ssl_crl_crlpath and ssl_crl are also failing. I relate these failures
to the fact that I built with -DWITH_SSL=system.
We can see that slave (http://jenkins.percona.com/view/PS%205.6/job/percona-server-5.6-param/196/BUILD_TYPE=debug,Host=ubuntu-lucid-64bit/)
ran out resources and was unable to create new threads and processes.
I can explain this by the fact that this is a VPS instance which means it
not fully virtualized and resources like processes and threads are shared
between all virtual environments. When 8 mysql tests run in parallel, each
testing replication (2 mysqld processes), each allocated pool of 50 threads,
we'll have about 800 only threadpool worker threads, we should also take
into account IO threads and many others. This also likely a reason of weird
failures seen by Alexey B.

[Sergei Glushchenko (sergei.glushchenko)]

I added test case for bug #1201681, which is also fixed by this branch

[Vlad Lesin (vlad-lesin)]

1) Unnecessary changes in 303;
2) vio_ssl_cancel() is declared but not defined;

There are two places where vio_shutdown() is replaced by vio_cancel(). vio_shutdown() invokes mysql_socket_close() but vio_cancel() does not. Are there any other places where socket is closed for those two cases to avoid descriptors leak?

review: Needs Fixing (g2)

[Vlad Lesin (vlad-lesin)]

The above was partial review.

[Sergei Glushchenko (sergei.glushchenko)]

Hi Vlad,

Thank you for the review,

I agree with 1) and 2). Will puth the empty line back and remove vio_ssl_cancel() declaration.

Connection will eventually be closed via vio_shutdown called from close_connection from threadpool_remove_connection in case of threadpool or from do_handle_one_connection in case of thread per connection. The reason for this change is that when connection is killed we have to signal to signal to the epoll/kevent listener that it should handle this connection and shut it down. By closing socket we will not emit any event for epoll and connection will stuck in epoll queue.

Sergei

[Sergei Glushchenko (sergei.glushchenko)]

repushed with fixes of 1 and 2 of Vlad's comments

[Alexey Kopytov (akopytov)]

Sergei,

Looks good in general, just a couple of minor comments:

  - is “--innodb-read-only” in the test case a copy paste from another
    test?
  - it doesn’t look good that there’s now some copy-pasted code between
    vio_shutdown*() and vio_cancel*(). Would be nice to avoid
    duplication by making vio_shutdown*() call vio_cancel*() to do most
    of the work, and then close/invalidate the socket.

review: Needs Fixing

[Sergei Glushchenko (sergei.glushchenko)]

Hi Alexey,

Yes, it was copied by mistake, fixed.
Code duplication was removed as well.

http://jenkins.percona.com/view/PS%205.6/job/-server-5.6-param/325/

[Laurynas Biveinis (laurynas-biveinis)]

Diff lines 61--75 can be replaced by a single --source include/restart_mysqld.inc

Otherwise looks good.

review: Needs Fixing

[Sergei Glushchenko (sergei.glushchenko)]

Laurynas,

--source include/restart_mysqld.inc

will not work this testcase. This testcase is to check that server with threadpool enabled and two connections opened restarted normally. If bug is present, testcase will fail with timeout.

restart_mysqld.inc will force server restart with timeout 10 seconds, this mean server will restart anyway and we not test the bug.

[Laurynas Biveinis (laurynas-biveinis)]

review: Approve

=== modified file 'Percona-Server/include/violite.h'
--- Percona-Server/include/violite.h	2013-06-25 13:13:06 +0000
+++ Percona-Server/include/violite.h	2013-09-29 16:12:18 +0000
@@ -73,6 +73,7 @@
 
 void    vio_delete(Vio* vio);
 int vio_shutdown(Vio* vio, int how);
+int vio_cancel(Vio* vio, int how);
 my_bool vio_reset(Vio* vio, enum enum_vio_type type,
                   my_socket sd, void *ssl, uint flags);
 size_t  vio_read(Vio *vio, uchar *	buf, size_t size);
@@ -190,6 +191,7 @@
 #define vio_should_retry(vio)                   (vio)->should_retry(vio)
 #define vio_was_timeout(vio)                    (vio)->was_timeout(vio)
 #define vio_shutdown(vio,how)                   ((vio)->vioshutdown)(vio,how)
+#define vio_cancel(vio,how)                     ((vio)->viocancel)(vio,how)
 #define vio_peer_addr(vio, buf, prt, buflen)    (vio)->peer_addr(vio, buf, prt, buflen)
 #define vio_io_wait(vio, event, timeout)        (vio)->io_wait(vio, event, timeout)
 #define vio_is_connected(vio)                   (vio)->is_connected(vio)
@@ -266,6 +268,11 @@
      descriptors, handles can remain valid after a shutdown.
   */
   int     (*vioshutdown)(Vio*, int);
+  /*
+     Partial shutdown. All the actions performed which shutdown performs,
+     but descriptor remains open and valid.
+  */
+  int     (*viocancel)(Vio*, int);
   my_bool (*is_connected)(Vio*);
   my_bool (*has_data) (Vio*);
   int (*io_wait)(Vio*, enum enum_vio_io_event, int);

=== added file 'Percona-Server/mysql-test/r/percona_bug1201681.result'
--- Percona-Server/mysql-test/r/percona_bug1201681.result	1970-01-01 00:00:00 +0000
+++ Percona-Server/mysql-test/r/percona_bug1201681.result	2013-09-29 16:12:18 +0000
@@ -0,0 +1,6 @@
+# Request clean shutdown
+# Wait for disconect
+# Restart server.
+SELECT 1;
+1
+1

=== added file 'Percona-Server/mysql-test/t/percona_bug1201681-master.opt'
--- Percona-Server/mysql-test/t/percona_bug1201681-master.opt	1970-01-01 00:00:00 +0000
+++ Percona-Server/mysql-test/t/percona_bug1201681-master.opt	2013-09-29 16:12:18 +0000
@@ -0,0 +1,1 @@
+--thread-handling=pool-of-threads

=== added file 'Percona-Server/mysql-test/t/percona_bug1201681.test'
--- Percona-Server/mysql-test/t/percona_bug1201681.test	1970-01-01 00:00:00 +0000
+++ Percona-Server/mysql-test/t/percona_bug1201681.test	2013-09-29 16:12:18 +0000
@@ -0,0 +1,27 @@
+# Bug 1201681: Server with threadpool hangs during shutdown when there are
+#              open connections
+
+-- source include/have_pool_of_threads.inc
+
+--connect (con1,localhost,root,,)
+
+# Write file to make mysql-test-run.pl wait for the server to stop
+--exec echo "wait" > $MYSQLTEST_VARDIR/tmp/mysqld.1.expect
+
+--echo # Request clean shutdown
+--send_shutdown
+
+--echo # Wait for disconect
+--source include/wait_until_disconnected.inc
+
+--echo # Restart server.
+--exec echo "restart" > $MYSQLTEST_VARDIR/tmp/mysqld.1.expect
+
+-- enable_reconnect
+-- source include/wait_until_connected_again.inc
+-- disable_reconnect
+
+--connection con1
+SELECT 1;
+
+--disconnect con1

=== modified file 'Percona-Server/sql/mysqld.cc'
--- Percona-Server/sql/mysqld.cc	2013-09-09 15:24:45 +0000
+++ Percona-Server/sql/mysqld.cc	2013-09-29 16:12:18 +0000
@@ -1434,8 +1434,8 @@
       continue;
 
     tmp->killed= THD::KILL_CONNECTION;
+    mysql_mutex_lock(&tmp->LOCK_thd_data);
     MYSQL_CALLBACK(thread_scheduler, post_kill_notification, (tmp));
-    mysql_mutex_lock(&tmp->LOCK_thd_data);
     if (tmp->mysys_var)
     {
       tmp->mysys_var->abort=1;

=== modified file 'Percona-Server/sql/sql_class.cc'
--- Percona-Server/sql/sql_class.cc	2013-09-21 18:26:20 +0000
+++ Percona-Server/sql/sql_class.cc	2013-09-29 16:12:18 +0000
@@ -1902,33 +1902,8 @@
 #ifdef SIGNAL_WITH_VIO_SHUTDOWN
     if (this != current_thd)
     {
-      /*
-        Before sending a signal, let's close the socket of the thread
-        that is being killed ("this", which is not the current thread).
-        This is to make sure it does not block if the signal is lost.
-        This needs to be done only on platforms where signals are not
-        a reliable interruption mechanism.
-
-        Note that the downside of this mechanism is that we could close
-        the connection while "this" target thread is in the middle of
-        sending a result to the application, thus violating the client-
-        server protocol.
-
-        On the other hand, without closing the socket we have a race
-        condition. If "this" target thread passes the check of
-        thd->killed, and then the current thread runs through
-        THD::awake(), sets the 'killed' flag and completes the
-        signaling, and then the target thread runs into read(), it will
-        block on the socket. As a result of the discussions around
-        Bug#37780, it has been decided that we accept the race
-        condition. A second KILL awakes the target from read().
-
-        If we are killing ourselves, we know that we are not blocked.
-        We also know that we will check thd->killed before we go for
-        reading the next statement.
-      */
-
-      shutdown_active_vio();
+      if (active_vio)
+        vio_cancel(active_vio, SHUT_RDWR);
     }
 #endif
 

=== modified file 'Percona-Server/sql/threadpool_common.cc'
--- Percona-Server/sql/threadpool_common.cc	2013-05-27 03:01:29 +0000
+++ Percona-Server/sql/threadpool_common.cc	2013-09-29 16:12:18 +0000
@@ -226,6 +226,7 @@
         thd->net.reading_or_writing= 1;
         thd->skip_wait_timeout= true;
         MYSQL_SOCKET_SET_STATE(thd->net.vio->mysql_socket, PSI_SOCKET_STATE_IDLE);
+        thd->m_server_idle= true;
         threadpool_init_net_server_extension(thd);
       }
     }
@@ -326,13 +327,18 @@
       thd->net.reading_or_writing= 1;
       goto end;
     }
+    if (!thd->m_server_idle) {
+      MYSQL_SOCKET_SET_STATE(thd->net.vio->mysql_socket, PSI_SOCKET_STATE_IDLE);
+      MYSQL_START_IDLE_WAIT(thd->m_idle_psi, &thd->m_idle_state);
+      thd->m_server_idle= true;
+    }
   }
 
 end:
-  if (!thd->m_server_idle) {
+  if (!retval && !thd->m_server_idle) {
     MYSQL_SOCKET_SET_STATE(thd->net.vio->mysql_socket, PSI_SOCKET_STATE_IDLE);
     MYSQL_START_IDLE_WAIT(thd->m_idle_psi, &thd->m_idle_state);
-    thd->m_server_idle = true;
+    thd->m_server_idle= true;
   }
 
   worker_context.restore();

=== modified file 'Percona-Server/sql/threadpool_unix.cc'
--- Percona-Server/sql/threadpool_unix.cc	2013-05-27 03:01:29 +0000
+++ Percona-Server/sql/threadpool_unix.cc	2013-09-29 16:12:18 +0000
@@ -1295,7 +1295,7 @@
     DBUG_VOID_RETURN;
   
   if (thd->net.vio)
-    vio_shutdown(thd->net.vio, SHUT_RD);
+    vio_cancel(thd->net.vio, SHUT_RD);
   DBUG_VOID_RETURN;
 }
 

=== modified file 'Percona-Server/vio/vio.c'
--- Percona-Server/vio/vio.c	2013-05-12 06:24:46 +0000
+++ Percona-Server/vio/vio.c	2013-09-29 16:12:18 +0000
@@ -92,6 +92,7 @@
     vio->should_retry	=vio_should_retry;
     vio->was_timeout    =vio_was_timeout;
     vio->vioshutdown	=vio_shutdown_pipe;
+    vio->viocancel      =vio_cancel_pipe;
     vio->peer_addr	=vio_peer_addr;
     vio->io_wait        =no_io_wait;
     vio->is_connected   =vio_is_connected_pipe;
@@ -111,6 +112,7 @@
     vio->should_retry	=vio_should_retry;
     vio->was_timeout    =vio_was_timeout;
     vio->vioshutdown	=vio_shutdown_shared_memory;
+    vio->viocancel      =vio_cancel_shared_memory;
     vio->peer_addr	=vio_peer_addr;
     vio->io_wait        =no_io_wait;
     vio->is_connected   =vio_is_connected_shared_memory;
@@ -130,6 +132,7 @@
     vio->should_retry	=vio_should_retry;
     vio->was_timeout    =vio_was_timeout;
     vio->vioshutdown	=vio_ssl_shutdown;
+    vio->viocancel      =vio_cancel;
     vio->peer_addr	=vio_peer_addr;
     vio->io_wait        =vio_io_wait;
     vio->is_connected   =vio_is_connected;
@@ -147,6 +150,7 @@
   vio->should_retry     =vio_should_retry;
   vio->was_timeout      =vio_was_timeout;
   vio->vioshutdown      =vio_shutdown;
+  vio->viocancel        =vio_cancel;
   vio->peer_addr        =vio_peer_addr;
   vio->io_wait          =vio_io_wait;
   vio->is_connected     =vio_is_connected;

=== modified file 'Percona-Server/vio/vio_priv.h'
--- Percona-Server/vio/vio_priv.h	2013-05-12 06:24:46 +0000
+++ Percona-Server/vio/vio_priv.h	2013-09-29 16:12:18 +0000
@@ -30,6 +30,7 @@
 size_t vio_write_pipe(Vio *vio, const uchar * buf, size_t size);
 my_bool vio_is_connected_pipe(Vio *vio);
 int vio_shutdown_pipe(Vio * vio, int how);
+int vio_cancel_pipe(Vio * vio, int how);
 #endif
 
 #ifdef HAVE_SMEM
@@ -37,6 +38,7 @@
 size_t vio_write_shared_memory(Vio *vio, const uchar * buf, size_t size);
 my_bool vio_is_connected_shared_memory(Vio *vio);
 int vio_shutdown_shared_memory(Vio * vio, int how);
+int vio_cancel_shared_memory(Vio * vio, int how);
 void vio_delete_shared_memory(Vio *vio);
 #endif
 

=== modified file 'Percona-Server/vio/viopipe.c'
--- Percona-Server/vio/viopipe.c	2013-05-12 06:24:46 +0000
+++ Percona-Server/vio/viopipe.c	2013-09-29 16:12:18 +0000
@@ -143,5 +143,19 @@
   DBUG_RETURN(ret);
 }
 
+
+int vio_cancel_pipe(Vio *vio, int how)
+{
+  DBUG_ENTER("vio_shutdown_pipe");
+
+  CancelIo(vio->hPipe);
+  CloseHandle(vio->overlapped.hEvent);
+  DisconnectNamedPipe(vio->hPipe);
+
+  vio->inactive= TRUE;
+
+  DBUG_RETURN(0);
+}
+
 #endif
 

=== modified file 'Percona-Server/vio/vioshm.c'
--- Percona-Server/vio/vioshm.c	2013-05-12 06:24:46 +0000
+++ Percona-Server/vio/vioshm.c	2013-09-29 16:12:18 +0000
@@ -222,5 +222,21 @@
   DBUG_RETURN(0);
 }
 
+int vio_cancel_shared_memory(Vio * vio, int how)
+{
+  DBUG_ENTER("vio_cancel_shred_memory");
+  if (vio->inactive == FALSE)
+  {
+    /*
+      Set event_conn_closed for notification of both client and server that
+      connection is closed
+    */
+    SetEvent(vio->event_conn_closed);
+  }
+
+  vio->inactive= TRUE;
+  DBUG_RETURN(0);
+}
+
 #endif /* #if defined(_WIN32) && defined(HAVE_SMEM) */
 

=== modified file 'Percona-Server/vio/viosocket.c'
--- Percona-Server/vio/viosocket.c	2013-06-25 13:13:06 +0000
+++ Percona-Server/vio/viosocket.c	2013-09-29 16:12:18 +0000
@@ -449,7 +449,30 @@
   int r=0;
   DBUG_ENTER("vio_shutdown");
 
- if (vio->inactive == FALSE)
+  r= vio_cancel(vio, how);
+
+  if (mysql_socket_close(vio->mysql_socket))
+    r= -1;
+
+  if (r)
+  {
+    DBUG_PRINT("vio_error", ("close() failed, error: %d",socket_errno));
+    /* FIXME: error handling (not critical for MySQL) */
+  }
+
+  vio->inactive= TRUE;
+  vio->mysql_socket= MYSQL_INVALID_SOCKET;
+
+  DBUG_RETURN(r);
+}
+
+
+int vio_cancel(Vio * vio, int how)
+{
+  int r= 0;
+  DBUG_ENTER("vio_cancel");
+
+  if (vio->inactive == FALSE)
   {
     DBUG_ASSERT(vio->type ==  VIO_TYPE_TCPIP ||
       vio->type == VIO_TYPE_SOCKET ||
@@ -463,16 +486,8 @@
     Windows). */
     (void) cancel_io((HANDLE)vio->mysql_socket, vio->thread_id);
 #endif
-    if (mysql_socket_close(vio->mysql_socket))
-      r= -1;
-  }
-  if (r)
-  {
-    DBUG_PRINT("vio_error", ("close() failed, error: %d",socket_errno));
-    /* FIXME: error handling (not critical for MySQL) */
-  }
-  vio->inactive= TRUE;
-  vio->mysql_socket= MYSQL_INVALID_SOCKET;
+  }
+
   DBUG_RETURN(r);
 }