Fix for BUG#12228: SP cache code:

* Cleanup SP Cache code, now SP Cache only deletes sp_head objects in 
  sp_cache_flush_obsolete() invalidates all pointers to routines in the cache.
* Use new SP Cache use contract in the code.

There is no test case because it doesn't seem to be possible to cause thread races to end
the same way they end in heavy-load test. This patch removes the crash in heavy test.

mysql-test/r/type_bit.result

@@ -34,7 +34,7 @@ select 0 + b'1111111111111111';
 select 0 + b'1000000000000001';
 0 + b'1000000000000001'
 32769
-drop table if exists t1;
+drop table if exists t1,t2;
 create table t1 (a bit(65));
 ERROR 42000: Column length too big for column 'a' (max = 64); use BLOB or TEXT instead
 create table t1 (a bit(0));

mysql-test/r/view.result

@@ -1880,6 +1880,8 @@ test.v5	check	error	View 'test.v5' references invalid table(s) or column(s) or f
 test.v6	check	status	OK
 drop view v1, v2, v3, v4, v5, v6;
 drop table t2;
+drop function if exists f1;
+drop function if exists f2;
 CREATE TABLE t1 (col1 time);
 CREATE TABLE t2 (col1 time);
 CREATE TABLE t3 (col1 time);

mysql-test/t/type_bit.test

@@ -16,7 +16,7 @@ select 0 + b'1111111111111111';
 select 0 + b'1000000000000001';
 
 --disable_warnings
-drop table if exists t1;
+drop table if exists t1,t2;
 --enable_warnings
 
 --error 1074

mysql-test/t/view.test

@@ -1711,6 +1711,10 @@ CHECK TABLE v1, v2, v3, v4, v5, v6;
 drop view v1, v2, v3, v4, v5, v6;
 drop table t2;
 
+--disable_warnings
+drop function if exists f1;
+drop function if exists f2;
+--enable_warnings
 CREATE TABLE t1 (col1 time);
 CREATE TABLE t2 (col1 time);
 CREATE TABLE t3 (col1 time);

sql/sp.cc

@@ -986,13 +986,11 @@ int
 sp_drop_procedure(THD *thd, sp_name *name)
 {
   int ret;
-  bool found;
   DBUG_ENTER("sp_drop_procedure");
   DBUG_PRINT("enter", ("name: %*s", name->m_name.length, name->m_name.str));
 
-  found= sp_cache_remove(&thd->sp_proc_cache, name);
   ret= db_drop_routine(thd, TYPE_ENUM_PROCEDURE, name);
-  if (!found && !ret)
+  if (!ret)
     sp_cache_invalidate();
   DBUG_RETURN(ret);
 }
@@ -1002,13 +1000,11 @@ int
 sp_update_procedure(THD *thd, sp_name *name, st_sp_chistics *chistics)
 {
   int ret;
-  bool found;
   DBUG_ENTER("sp_update_procedure");
   DBUG_PRINT("enter", ("name: %*s", name->m_name.length, name->m_name.str));
 
-  found= sp_cache_remove(&thd->sp_proc_cache, name);
   ret= db_update_routine(thd, TYPE_ENUM_PROCEDURE, name, chistics);
-  if (!found && !ret)
+  if (!ret)
     sp_cache_invalidate();
   DBUG_RETURN(ret);
 }
@@ -1099,13 +1095,11 @@ int
 sp_drop_function(THD *thd, sp_name *name)
 {
   int ret;
-  bool found;
   DBUG_ENTER("sp_drop_function");
   DBUG_PRINT("enter", ("name: %*s", name->m_name.length, name->m_name.str));
 
-  found= sp_cache_remove(&thd->sp_func_cache, name);
   ret= db_drop_routine(thd, TYPE_ENUM_FUNCTION, name);
-  if (!found && !ret)
+  if (!ret)
     sp_cache_invalidate();
   DBUG_RETURN(ret);
 }
@@ -1115,13 +1109,11 @@ int
 sp_update_function(THD *thd, sp_name *name, st_sp_chistics *chistics)
 {
   int ret;
-  bool found;
   DBUG_ENTER("sp_update_procedure");
   DBUG_PRINT("enter", ("name: %*s", name->m_name.length, name->m_name.str));
 
-  found= sp_cache_remove(&thd->sp_func_cache, name);
   ret= db_update_routine(thd, TYPE_ENUM_FUNCTION, name, chistics);
-  if (!found && !ret)
+  if (!ret)
     sp_cache_invalidate();
   DBUG_RETURN(ret);
 }

sql/sp_cache.cc

@@ -24,17 +24,78 @@
 static pthread_mutex_t Cversion_lock;
 static ulong Cversion = 0;
 
-void
-sp_cache_init()
+
+/*
+  Cache of stored routines. 
+*/
+
+class sp_cache
+{
+public:
+  ulong version;
+
+  sp_cache();
+  ~sp_cache();
+
+  inline void insert(sp_head *sp)
+  {
+    /* TODO: why don't we check return value? */
+    my_hash_insert(&m_hashtable, (const byte *)sp);
+  }
+
+  inline sp_head *lookup(char *name, uint namelen)
+  {
+    return (sp_head *)hash_search(&m_hashtable, (const byte *)name, namelen);
+  }
+
+#ifdef NOT_USED
+  inline bool remove(char *name, uint namelen)
+  {
+    sp_head *sp= lookup(name, namelen);
+    if (sp)
+    {
+      hash_delete(&m_hashtable, (byte *)sp);
+      return TRUE;
+    }
+    return FALSE;
+  }
+#endif 
+
+  inline void remove_all()
+  {
+    cleanup();
+    init();
+  }
+
+private:
+  void init();
+  void cleanup();
+
+  /* All routines in this cache */
+  HASH m_hashtable;
+}; // class sp_cache
+
+
+/* Initialize the SP caching once at startup */
+
+void sp_cache_init()
 {
   pthread_mutex_init(&Cversion_lock, MY_MUTEX_INIT_FAST);
 }
 
-void
-sp_cache_clear(sp_cache **cp)
+
+/*
+  Clear the cache *cp and set *cp to NULL.
+  SYNOPSIS
+    sp_cache_clear()
+      cp  Pointer to cache to clear
+  NOTE
+    This function doesn't invalidate other caches.
+*/
+
+void sp_cache_clear(sp_cache **cp)
 {
   sp_cache *c= *cp;
-
   if (c)
   {
     delete c;
@@ -42,85 +103,118 @@ sp_cache_clear(sp_cache **cp)
   }
 }
 
-void
-sp_cache_insert(sp_cache **cp, sp_head *sp)
+
+/*
+  Insert a routine into the cache.
+
+  SYNOPSIS
+    sp_cache_insert()
+      cp  The cache to put routine into
+      sp  Routine to insert.
+*/
+
+void sp_cache_insert(sp_cache **cp, sp_head *sp)
 {
   sp_cache *c= *cp;
 
   if (! c)
-    c= new sp_cache();
-  if (c)
   {
     ulong v;
-
+    c= new sp_cache();
     pthread_mutex_lock(&Cversion_lock); // LOCK
     v= Cversion;
     pthread_mutex_unlock(&Cversion_lock); // UNLOCK
-
-    if (c->version < v)
-    {
-      if (*cp)
-	c->remove_all();
     c->version= v;
   }
+  if (c)
+  {
+    DBUG_PRINT("info",("sp_cache: inserting: %*s", sp->m_qname.length, sp->m_qname.str));
     c->insert(sp);
     if (*cp == NULL)
       *cp= c;
   }
 }
 
-sp_head *
-sp_cache_lookup(sp_cache **cp, sp_name *name)
+
+/* 
+  Look up a routine in the cache.
+  SYNOPSIS
+    sp_cache_lookup()
+      cp    Cache to look into
+      name  Name of rutine to find
+      
+  NOTE
+    An obsolete (but not more obsolete then since last
+    sp_cache_flush_obsolete call) routine may be returned.
+
+  RETURN 
+    The routine or
+    NULL if the routine not found.
+*/
+
+sp_head *sp_cache_lookup(sp_cache **cp, sp_name *name)
 {
-  ulong v;
   sp_cache *c= *cp;
-
-  if (! c)
+  if (!c)
     return NULL;
+  return c->lookup(name->m_qname.str, name->m_qname.length);
+}
+
 
+/*
+  Invalidate all routines in all caches.
+
+  SYNOPSIS
+    sp_cache_invalidate()
+      
+  NOTE
+    This is called when a VIEW definition is modifed. We can't destroy sp_head
+    objects here as one may modify VIEW definitions from prelocking-free SPs.
+*/
+
+void sp_cache_invalidate()
+{
+  DBUG_PRINT("info",("sp_cache: invalidating"));
   pthread_mutex_lock(&Cversion_lock); // LOCK
-  v= Cversion;
+  Cversion++;
   pthread_mutex_unlock(&Cversion_lock); // UNLOCK
-
-  if (c->version < v)
-  {
-    c->remove_all();
-    c->version= v;
-    return NULL;
-  }
-  return c->lookup(name->m_qname.str, name->m_qname.length);
 }
 
-bool
-sp_cache_remove(sp_cache **cp, sp_name *name)
+
+/*
+  Remove out-of-date SPs from the cache. 
+  
+  SYNOPSIS
+    sp_cache_flush_obsolete()
+      cp  Cache to flush
+
+  NOTE
+    This invalidates pointers to sp_head objects this thread uses.
+    In practice that means 'dont call this function when inside SP'.
+*/
+
+void sp_cache_flush_obsolete(sp_cache **cp)
 {
   sp_cache *c= *cp;
-  bool found= FALSE;
-
   if (c)
   {
     ulong v;
-
     pthread_mutex_lock(&Cversion_lock); // LOCK
-    v= Cversion++;
+    v= Cversion;
     pthread_mutex_unlock(&Cversion_lock); // UNLOCK
-
     if (c->version < v)
+    {
+      DBUG_PRINT("info",("sp_cache: deleting all functions"));
+      /* We need to delete all elements. */
       c->remove_all();
-    else
-      found= c->remove(name->m_qname.str, name->m_qname.length);
-    c->version= v+1;
+      c->version= v;
+    }
   }
-  return found;
 }
 
-void
-sp_cache_invalidate()
-{
-  pthread_mutex_lock(&Cversion_lock); // LOCK
-  Cversion++;
-  pthread_mutex_unlock(&Cversion_lock); // UNLOCK
-}
+/*************************************************************************
+  Internal functions 
+ *************************************************************************/
 
 static byte *
 hash_get_key_for_sp_head(const byte *ptr, uint *plen,
@@ -136,7 +230,6 @@ static void
 hash_free_sp_head(void *p)
 {
   sp_head *sp= (sp_head *)p;
-
   delete sp;
 }
 

sql/sp_cache.h

@@ -32,87 +32,32 @@
 class sp_head;
 class sp_cache;
 
-/* Initialize the SP caching once at startup */
-void sp_cache_init();
-
-/* Clear the cache *cp and set *cp to NULL */
-void sp_cache_clear(sp_cache **cp);
-
-/* Insert an SP into cache. If 'cp' points to NULL, it's set to a new cache */
-void sp_cache_insert(sp_cache **cp, sp_head *sp);
-
-/* Lookup an SP in cache */
-sp_head *sp_cache_lookup(sp_cache **cp, sp_name *name);
-
-/* 
-  Remove an SP from cache, and also bump the Cversion number so all other 
-  caches are invalidated. 
-  Returns true if something was removed.
-*/
-bool sp_cache_remove(sp_cache **cp, sp_name *name);
-
-/* Invalidate all existing SP caches by bumping Cversion number. */
-void sp_cache_invalidate();
-
-
 /*
- *
- * The cache class. Don't use this directly, use the C API above
- *
- */
+  Cache usage scenarios:
+  1. Application-wide init:
+    sp_cache_init();
 
-class sp_cache
-{
-public:
+  2. SP execution in thread:
+  2.1 While holding sp_head* pointers:
   
-  ulong version;
+    // look up a routine in the cache (no checks if it is up to date or not)
+    sp_cache_lookup(); 
     
-  sp_cache();
+    sp_cache_insert();
+    sp_cache_invalidate();
   
-  ~sp_cache();
+  2.2 When not holding any sp_head* pointers (at query end):
+    sp_cache_flush_obsolete();
   
-  void
-  init();
-
-  void
-  cleanup();
-
-  inline void
-  insert(sp_head *sp)
-  {
-    my_hash_insert(&m_hashtable, (const byte *)sp);
-  }
-
-  inline sp_head *
-  lookup(char *name, uint namelen)
-  {
-    return (sp_head *)hash_search(&m_hashtable, (const byte *)name, namelen);
-  }
-
-  inline bool
-  remove(char *name, uint namelen)
-  {
-    sp_head *sp= lookup(name, namelen);
-
-    if (sp)
-    {
-      hash_delete(&m_hashtable, (byte *)sp);
-      return TRUE;
-    }
-    return FALSE;
-  }
-
-  inline void
-  remove_all()
-  {
-    cleanup();
-    init();
-  }
-
-private:
-
-  HASH m_hashtable;
+  3. Before thread exit:
+    sp_cache_clear();
+*/
 
-}; // class sp_cache
+void sp_cache_init();
+void sp_cache_clear(sp_cache **cp);
+void sp_cache_insert(sp_cache **cp, sp_head *sp);
+sp_head *sp_cache_lookup(sp_cache **cp, sp_name *name);
+void sp_cache_invalidate();
+void sp_cache_flush_obsolete(sp_cache **cp);
 
 #endif /* _SP_CACHE_H_ */

sql/sql_parse.cc

@@ -5347,7 +5347,8 @@ void mysql_init_multi_delete(LEX *lex)
 void mysql_parse(THD *thd, char *inBuf, uint length)
 {
   DBUG_ENTER("mysql_parse");
-
+  sp_cache_flush_obsolete(&thd->sp_proc_cache);
+  sp_cache_flush_obsolete(&thd->sp_func_cache);
   mysql_init_query(thd, (uchar*) inBuf, length);
   if (query_cache_send_result_to_client(thd, inBuf, length) <= 0)
   {

sql/sql_prepare.cc

@@ -73,6 +73,7 @@ Long data handling:
 #include <m_ctype.h>  // for isspace()
 #include "sp_head.h"
 #include "sp.h"
+#include "sp_cache.h"
 #ifdef EMBEDDED_LIBRARY
 /* include MYSQL_BIND headers */
 #include <mysql.h>
@@ -1729,6 +1730,8 @@ bool mysql_stmt_prepare(THD *thd, char *packet, uint packet_length,
   DBUG_ENTER("mysql_stmt_prepare");
 
   DBUG_PRINT("prep_query", ("%s", packet));
+  sp_cache_flush_obsolete(&thd->sp_proc_cache);
+  sp_cache_flush_obsolete(&thd->sp_func_cache);
 
   /*
     If this is an SQLCOM_PREPARE, we also increase Com_prepare_sql.
@@ -1978,6 +1981,8 @@ void mysql_stmt_execute(THD *thd, char *packet, uint packet_length)
   Prepared_statement *stmt;
   DBUG_ENTER("mysql_stmt_execute");
 
+  sp_cache_flush_obsolete(&thd->sp_proc_cache);
+  sp_cache_flush_obsolete(&thd->sp_func_cache);
   packet+= 9;                               /* stmt_id + 5 bytes of flags */
 
   statistic_increment(thd->status_var.com_stmt_execute, &LOCK_status);