Bug#17181 (mysqlslap test server crash):

  Moving assignments to table_map_id for thread-safe handling of
  table shares.


sql/ha_ndbcluster_binlog.cc:
  Assign_new_table_id() now takes table share instead of table.
sql/mysql_priv.h:
  New protptype for assign_new_table_id().
sql/sql_base.cc:
  Assign_new_table_id() now takes a table share instead of a table.
  Moving call to assign_new_table_id() into get_table_share().
sql/table.cc:
  Setting default values of table_map_id and table_map_version inside
  alloc_table_share() and init_tmp_table_share().
  Removing the settings from open_table_from_share().

4 files changed, 65 insertions, 46 deletions

diff --git a/sql/ha_ndbcluster_binlog.cc b/sql/ha_ndbcluster_binlog.cc
index 06f946b013c..2c2624358af 100644
--- a/sql/ha_ndbcluster_binlog.cc
+++ b/sql/ha_ndbcluster_binlog.cc
@@ -310,7 +310,7 @@ void ndbcluster_binlog_init_share(NDB_SHARE *share, TABLE *_table)
       table= 0;
       break;
     }
-    assign_new_table_id(table);
+    assign_new_table_id(table_share);
     if (!table->record[1] || table->record[1] == table->record[0])
     {
       table->record[1]= alloc_root(&table->mem_root,
diff --git a/sql/mysql_priv.h b/sql/mysql_priv.h
index fc777c47818..d4f9ddfa167 100644
--- a/sql/mysql_priv.h
+++ b/sql/mysql_priv.h
@@ -657,7 +657,7 @@ bool table_cache_init(void);
 void table_cache_free(void);
 bool table_def_init(void);
 void table_def_free(void);
-void assign_new_table_id(TABLE *table);
+void assign_new_table_id(TABLE_SHARE *share);
 uint cached_open_tables(void);
 uint cached_table_definitions(void);
 void kill_mysql(void);
diff --git a/sql/sql_base.cc b/sql/sql_base.cc
index 2d6576bff8b..f48df61e9a1 100644
--- a/sql/sql_base.cc
+++ b/sql/sql_base.cc
@@ -313,6 +313,22 @@ TABLE_SHARE *get_table_share(THD *thd, TABLE_LIST *table_list, char *key,
     conflicts
   */
   (void) pthread_mutex_lock(&share->mutex);
+
+  /*
+    We assign a new table id under the protection of the LOCK_open and
+    the share's own mutex.  We do this insted of creating a new mutex
+    and using it for the sole purpose of serializing accesses to a
+    static variable, we assign the table id here.  We assign it to the
+    share before inserting it into the table_def_cache to be really
+    sure that it cannot be read from the cache without having a table
+    id assigned.
+
+    CAVEAT. This means that the table cannot be used for
+    binlogging/replication purposes, unless get_table_share() has been
+    called directly or indirectly.
+   */
+  assign_new_table_id(share);
+
   if (my_hash_insert(&table_def_cache, (byte*) share))
   {
 #ifdef NOT_YET
@@ -2383,43 +2399,50 @@ void abort_locked_tables(THD *thd,const char *db, const char *table_name)
 
 
 /*
-  Function to assign a new table map id to a table.
+  Function to assign a new table map id to a table share.
 
   PARAMETERS
 
-    table - Pointer to table structure
+    share - Pointer to table share structure
 
   PRE-CONDITION(S)
 
-    table is non-NULL
+    share is non-NULL
     The LOCK_open mutex is locked
+    The share->mutex is locked
 
   POST-CONDITION(S)
 
-    table->s->table_map_id is given a value that with a high certainty
-    is not used by any other table.
+    share->table_map_id is given a value that with a high certainty is
+    not used by any other table (the only case where a table id can be
+    reused is on wrap-around, which means more than 4 billion table
+    shares open at the same time).
 
-    table->s->table_map_id is not ULONG_MAX.
+    share->table_map_id is not ULONG_MAX.
  */
-void assign_new_table_id(TABLE *table)
+void assign_new_table_id(TABLE_SHARE *share)
 {
   static ulong last_table_id= ULONG_MAX;
 
-  DBUG_ENTER("assign_new_table_id(TABLE*)");
+  DBUG_ENTER("assign_new_table_id");
 
   /* Preconditions */
-  DBUG_ASSERT(table != NULL);
+  DBUG_ASSERT(share != NULL);
   safe_mutex_assert_owner(&LOCK_open);
+  safe_mutex_assert_owner(&share->mutex);
 
   ulong tid= ++last_table_id;                   /* get next id */
-  /* There is one reserved number that cannot be used. */
+  /*
+    There is one reserved number that cannot be used.  Remember to
+    change this when 6-byte global table id's are introduced.
+  */
   if (unlikely(tid == ULONG_MAX))
     tid= ++last_table_id;
-  table->s->table_map_id= tid;
+  share->table_map_id= tid;
   DBUG_PRINT("info", ("table_id=%lu", tid));
 
   /* Post conditions */
-  DBUG_ASSERT(table->s->table_map_id != ULONG_MAX);
+  DBUG_ASSERT(share->table_map_id != ULONG_MAX);
 
   DBUG_VOID_RETURN;
 }
@@ -2573,20 +2596,6 @@ retry:
      break;
    }
 
-  /*
-    We assign a new table id under the protection of the LOCK_open
-    mutex.  We assign a new table id here instead of inside openfrm()
-    since that function can be used without acquiring any lock (e.g.,
-    inside ha_create_table()).  Insted of creatint a new mutex and
-    using it for the sole purpose of serializing accesses to a static
-    variable, we assign the table id here.
-
-    CAVEAT. This means that the table cannot be used for
-    binlogging/replication purposes, unless open_table() has been called
-    directly or indirectly.
-   */
-  assign_new_table_id(entry);
-
   if (Table_triggers_list::check_n_load(thd, share->db.str,
                                         share->table_name.str, entry, 0))
   {
diff --git a/sql/table.cc b/sql/table.cc
index 7c266243d29..3e766fe6c0f 100644
--- a/sql/table.cc
+++ b/sql/table.cc
@@ -130,6 +130,24 @@ TABLE_SHARE *alloc_table_share(TABLE_LIST *table_list, char *key,
     share->version=       refresh_version;
     share->flush_version= flush_version;
 
+#ifdef HAVE_ROW_BASED_REPLICATION
+    /*
+      This constant is used to mark that no table map version has been
+      assigned.  No arithmetic is done on the value: it will be
+      overwritten with a value taken from MYSQL_BIN_LOG.
+    */
+    share->table_map_version= ~(ulonglong)0;
+
+    /*
+      Since alloc_table_share() can be called without any locking (for
+      example, ha_create_table... functions), we do not assign a table
+      map id here.  Instead we assign a value that is not used
+      elsewhere, and then assign a table map id inside open_table()
+      under the protection of the LOCK_open mutex.
+    */
+    share->table_map_id= ULONG_MAX;
+#endif
+
     memcpy((char*) &share->mem_root, (char*) &mem_root, sizeof(mem_root));
     pthread_mutex_init(&share->mutex, MY_MUTEX_INIT_FAST);
     pthread_cond_init(&share->cond, NULL);
@@ -180,6 +198,15 @@ void init_tmp_table_share(TABLE_SHARE *share, const char *key,
   share->path.length= share->normalized_path.length= strlen(path);
   share->frm_version= 		 FRM_VER_TRUE_VARCHAR;
 
+#ifdef HAVE_ROW_BASED_REPLICATION
+  /*
+    Temporary tables are not replicated, but we set up these fields
+    anyway to be able to catch errors.
+   */
+  share->table_map_version= ~(ulonglong)0;
+  share->table_map_id= ULONG_MAX;
+#endif
+
   DBUG_VOID_RETURN;
 }
 
@@ -371,6 +398,7 @@ err_not_open:
     share->error= error;
     open_table_error(share, error, (share->open_errno= my_errno), 0);
   }
+
   DBUG_RETURN(error);
 }
 
@@ -1503,24 +1531,6 @@ int open_table_from_share(THD *thd, TABLE_SHARE *share, const char *alias,
 
   *root_ptr= old_root;
   thd->status_var.opened_tables++;
-#ifdef HAVE_REPLICATION
-
-  /*
-     This constant is used to mark that no table map version has been
-     assigned.  No arithmetic is done on the value: it will be
-     overwritten with a value taken from MYSQL_BIN_LOG.
-  */
-  share->table_map_version= ~(ulonglong)0;
-
-  /*
-    Since openfrm() can be called without any locking (for example,
-    ha_create_table... functions), we do not assign a table map id
-    here.  Instead we assign a value that is not used elsewhere, and
-    then assign a table map id inside open_table() under the
-    protection of the LOCK_open mutex.
-   */
-  share->table_map_id= ULONG_MAX;
-#endif
 
   DBUG_RETURN (0);