Bug#15933: max_used_connections is wrong after FLUSH STATUS if connections are cached

After FLUSH STATUS max_used_connections was reset to 0, and haven't
been updated while cached threads were reused, until the moment a new
thread was created.

The first suggested fix from original bug report was implemented:

  a) On flushing the status, set max_used_connections to
  threads_connected, not to 0.

  b) Check if it is necessary to increment max_used_connections when
  taking a thread from the cache as well as when creating new threads

mysql-test/r/status.result

@@ -23,3 +23,23 @@ select 1;
 show status like 'last_query_cost';
 Variable_name	Value
 Last_query_cost	0.000000
+FLUSH STATUS;
+SHOW STATUS LIKE 'max_used_connections';
+Variable_name	Value
+Max_used_connections	3
+SET @save_thread_cache_size=@@thread_cache_size;
+SET GLOBAL thread_cache_size=3;
+SHOW STATUS LIKE 'max_used_connections';
+Variable_name	Value
+Max_used_connections	5
+FLUSH STATUS;
+SHOW STATUS LIKE 'max_used_connections';
+Variable_name	Value
+Max_used_connections	4
+SHOW STATUS LIKE 'max_used_connections';
+Variable_name	Value
+Max_used_connections	5
+SHOW STATUS LIKE 'max_used_connections';
+Variable_name	Value
+Max_used_connections	6
+SET GLOBAL thread_cache_size=@save_thread_cache_size;

mysql-test/t/status.test

@@ -39,8 +39,71 @@ drop table t1;
 # End of 4.1 tests
 
 #
-# lost_query_cost
+# last_query_cost
 #
 
 select 1;
 show status like 'last_query_cost';
+
+#
+# Test for Bug #15933 max_used_connections is wrong after FLUSH STATUS
+# if connections are cached
+#
+#
+# The first suggested fix from the bug report was chosen
+# (see http://bugs.mysql.com/bug.php?id=15933):
+#
+#   a) On flushing the status, set max_used_connections to
+#   threads_connected, not to 0.
+#
+#   b) Check if it is necessary to increment max_used_connections when
+#   taking a thread from the cache as well as when creating new threads
+#
+
+# Previous test uses con1, con2.  If we disconnect them, there will be
+# a race on when exactly this will happen, so we better leave them as
+# is.
+connection default;
+
+# Reset max_used_connections from previous tests.
+FLUSH STATUS;
+SHOW STATUS LIKE 'max_used_connections';
+
+# Save original setting.
+SET @save_thread_cache_size=@@thread_cache_size;
+SET GLOBAL thread_cache_size=3;
+
+connect (con3,localhost,root,,);
+connect (con4,localhost,root,,);
+
+connection con3;
+disconnect con4;
+
+# Check that max_used_connections still reflects maximum value.
+SHOW STATUS LIKE 'max_used_connections';
+
+# Check that after flush max_used_connections equals to current number
+# of connections.
+FLUSH STATUS;
+SHOW STATUS LIKE 'max_used_connections';
+
+# Check that max_used_connections is updated when cached thread is
+# reused...
+connect (con4,localhost,root,,);
+SHOW STATUS LIKE 'max_used_connections';
+
+# ...and when new thread is created.
+connect (con5,localhost,root,,);
+SHOW STATUS LIKE 'max_used_connections';
+
+# Restore original setting.
+connection default;
+SET GLOBAL thread_cache_size=@save_thread_cache_size;
+
+disconnect con5;
+disconnect con4;
+disconnect con3;
+disconnect con2;
+disconnect con1;
+
+# End of 5.0 tests

sql/mysql_priv.h

@@ -1097,6 +1097,7 @@ File open_binlog(IO_CACHE *log, const char *log_file_name,
 
 /* mysqld.cc */
 extern void MYSQLerror(const char*);
+void refresh_status(THD *thd);
 
 /* item_func.cc */
 extern bool check_reserved_words(LEX_STRING *name);

sql/mysqld.cc

@@ -1639,17 +1639,6 @@ void end_thread(THD *thd, bool put_in_cache)
 }
 
 
-/* Start a cached thread. LOCK_thread_count is locked on entry */
-
-static void start_cached_thread(THD *thd)
-{
-  thread_cache.append(thd);
-  wake_thread++;
-  thread_count++;
-  pthread_cond_signal(&COND_thread_cache);
-}
-
-
 void flush_thread_cache()
 {
   (void) pthread_mutex_lock(&LOCK_thread_count);
@@ -3780,6 +3769,25 @@ static bool read_init_file(char *file_name)
 
 
 #ifndef EMBEDDED_LIBRARY
+/*
+  Create new thread to handle incoming connection.
+
+  SYNOPSIS
+    create_new_thread()
+      thd in/out    Thread handle of future thread.
+
+  DESCRIPTION
+    This function will create new thread to handle the incoming
+    connection.  If there are idle cached threads one will be used.
+    'thd' will be pushed into 'threads'.
+
+    In single-threaded mode (#define ONE_THREAD) connection will be
+    handled inside this function.
+
+  RETURN VALUE
+    none
+*/
+
 static void create_new_thread(THD *thd)
 {
   DBUG_ENTER("create_new_thread");
@@ -3803,11 +3811,12 @@ static void create_new_thread(THD *thd)
   thd->real_id=pthread_self();			// Keep purify happy
 
   /* Start a new thread to handle connection */
+  thread_count++;
+
 #ifdef ONE_THREAD
   if (test_flags & TEST_NO_THREADS)		// For debugging under Linux
   {
     thread_cache_size=0;			// Safety
-    thread_count++;
     threads.append(thd);
     thd->real_id=pthread_self();
     (void) pthread_mutex_unlock(&LOCK_thread_count);
@@ -3816,18 +3825,20 @@ static void create_new_thread(THD *thd)
   else
 #endif
   {
+    if (thread_count-delayed_insert_threads > max_used_connections)
+      max_used_connections=thread_count-delayed_insert_threads;
+
     if (cached_thread_count > wake_thread)
     {
-      start_cached_thread(thd);
+      thread_cache.append(thd);
+      wake_thread++;
+      pthread_cond_signal(&COND_thread_cache);
     }
     else
     {
       int error;
-      thread_count++;
       thread_created++;
       threads.append(thd);
-      if (thread_count-delayed_insert_threads > max_used_connections)
-        max_used_connections=thread_count-delayed_insert_threads;
       DBUG_PRINT("info",(("creating thread %d"), thd->thread_id));
       thd->connect_time = time(NULL);
       if ((error=pthread_create(&thd->real_id,&connection_attrib,
@@ -7393,6 +7404,36 @@ static void create_pid_file()
 }
 
 
+/* Clear most status variables */
+void refresh_status(THD *thd)
+{
+  pthread_mutex_lock(&LOCK_status);
+
+  /* We must update the global status before cleaning up the thread */
+  add_to_status(&global_status_var, &thd->status_var);
+  bzero((char*) &thd->status_var, sizeof(thd->status_var));
+
+  for (struct show_var_st *ptr=status_vars; ptr->name; ptr++)
+  {
+    if (ptr->type == SHOW_LONG)
+      *(ulong*) ptr->value= 0;
+  }
+  /* Reset the counters of all key caches (default and named). */
+  process_key_caches(reset_key_cache_counters);
+  pthread_mutex_unlock(&LOCK_status);
+
+  /*
+    Set max_used_connections to the number of currently open
+    connections.  Lock LOCK_thread_count out of LOCK_status to avoid
+    deadlocks.  Status reset becomes not atomic, but status data is
+    not exact anyway.
+  */
+  pthread_mutex_lock(&LOCK_thread_count);
+  max_used_connections= thread_count-delayed_insert_threads;
+  pthread_mutex_unlock(&LOCK_thread_count);
+}
+
+
 /*****************************************************************************
   Instantiate have_xyx for missing storage engines
 *****************************************************************************/

sql/sql_parse.cc

@@ -74,7 +74,6 @@ static void decrease_user_connections(USER_CONN *uc);
 static bool check_db_used(THD *thd,TABLE_LIST *tables);
 static bool check_multi_update_lock(THD *thd);
 static void remove_escape(char *name);
-static void refresh_status(THD *thd);
 static bool append_file_to_dir(THD *thd, const char **filename_ptr,
 			       const char *table_name);
 
@@ -6709,27 +6708,6 @@ void kill_one_thread(THD *thd, ulong id, bool only_kill_query)
 }
 
 
-/* Clear most status variables */
-
-static void refresh_status(THD *thd)
-{
-  pthread_mutex_lock(&LOCK_status);
-
-  /* We must update the global status before cleaning up the thread */
-  add_to_status(&global_status_var, &thd->status_var);
-  bzero((char*) &thd->status_var, sizeof(thd->status_var));
-
-  for (struct show_var_st *ptr=status_vars; ptr->name; ptr++)
-  {
-    if (ptr->type == SHOW_LONG)
-      *(ulong*) ptr->value= 0;
-  }
-  /* Reset the counters of all key caches (default and named). */
-  process_key_caches(reset_key_cache_counters);
-  pthread_mutex_unlock(&LOCK_status);
-}
-
-
 	/* If pointer is not a null pointer, append filename to it */
 
 static bool append_file_to_dir(THD *thd, const char **filename_ptr,