MySQL Lists are EOL. Please join:

List:Commits« Previous MessageNext Message »
From:konstantin Date:June 27 2006 11:17pm
Subject:bk commit into 5.0 tree (konstantin:1.2207) BUG#19022
View as plain text  
Below is the list of changes that have just been committed into a local
5.0 repository of kostja. When kostja does a push these changes will
be propagated to the main repository and, within 24 hours after the
push, to the public repository.
For information on how to access the public repository
see http://dev.mysql.com/doc/mysql/en/installing-source-tree.html

ChangeSet
  1.2207 06/06/28 03:17:23 konstantin@stripped +5 -0
  A fix for Bug#19022 "Memory bug when switching db during trigger execution". 
  No test case as the bug is in an existing test case (rpl_trigger.test
  when it is run under valgrind).

  sql/sql_db.cc
    1.130 06/06/28 03:17:19 konstantin@stripped +40 -63
    Always make a deep copy of the argument in mysql_change_db, even 
    if running in a replication slave. This is necessary because 
    sp_use_new_db (stored procedures) assumes that mysql_change_db always makes a 
    deep copy of the argument, and thus passes a pointer to stack into it.
    This assumption was true for all cases except the replication slave thread.

  sql/sql_class.h
    1.292 06/06/28 03:17:19 konstantin@stripped +13 -11
    Fix set_db to always free the old db, even if the argument is NULL.
    Add a comment.

  sql/slave.h
    1.95 06/06/28 03:17:19 konstantin@stripped +0 -1
    Remove an unneeded declaration.

  sql/slave.cc
    1.273 06/06/28 03:17:19 konstantin@stripped +0 -18
    Move rewrite_db to log_event.cc

  sql/log_event.cc
    1.208 06/06/28 03:17:19 konstantin@stripped +26 -7
    Move rewrite_db to log_event.cc, the only place where it is used.

# This is a BitKeeper patch.  What follows are the unified diffs for the
# set of deltas contained in the patch.  The rest of the patch, the part
# that BitKeeper cares about, is below these diffs.
# User:	konstantin
# Host:	bodhi.netgear
# Root:	/opt/local/work/mysql-5.0-19022

--- 1.207/sql/log_event.cc	2006-06-27 14:56:13 +04:00
+++ 1.208/sql/log_event.cc	2006-06-28 03:17:19 +04:00
@@ -1635,14 +1635,33 @@
 */
 
 #if defined(HAVE_REPLICATION) && !defined(MYSQL_CLIENT)
+
+static const char *rewrite_db(const char *db)
+{
+  if (replicate_rewrite_db.is_empty() || db == NULL)
+    return db;
+  I_List_iterator<i_string_pair> it(replicate_rewrite_db);
+  i_string_pair* tmp;
+
+  while ((tmp=it++))
+  {
+    if (strcmp(tmp->key, db) == 0)
+      return tmp->val;
+  }
+  return db;
+}
+
+
 int Query_log_event::exec_event(struct st_relay_log_info* rli)
 {
   return exec_event(rli, query, q_len);
 }
 
 
-int Query_log_event::exec_event(struct st_relay_log_info* rli, const char *query_arg, uint32 q_len_arg)
+int Query_log_event::exec_event(struct st_relay_log_info* rli,
+                                const char *query_arg, uint32 q_len_arg)
 {
+  const char *new_db;
   int expected_error,actual_error= 0;
   /*
     Colleagues: please never free(thd->catalog) in MySQL. This would lead to
@@ -1651,8 +1670,8 @@
     Thank you.
   */
   thd->catalog= catalog_len ? (char *) catalog : (char *)"";
-  thd->db_length= db_len;
-  thd->db= (char*) rewrite_db(db, &thd->db_length);
+  new_db= rewrite_db(db);
+  thd->set_db(new_db, strlen(new_db));
   thd->variables.auto_increment_increment= auto_increment_increment;
   thd->variables.auto_increment_offset=    auto_increment_offset;
 
@@ -1869,7 +1888,7 @@
     TABLE uses the db.table syntax.
   */
   thd->catalog= 0;
-  thd->reset_db(NULL, 0);               // prevent db from being freed
+  thd->set_db(NULL, 0);
   thd->query= 0;			// just to be sure
   thd->query_length= 0;
   VOID(pthread_mutex_unlock(&LOCK_thread_count));
@@ -2817,8 +2836,8 @@
 int Load_log_event::exec_event(NET* net, struct st_relay_log_info* rli, 
 			       bool use_rli_only_for_errors)
 {
-  thd->db_length= db_len;
-  thd->db= (char*) rewrite_db(db, &thd->db_length);
+  const char *new_db= rewrite_db(db);
+  thd->set_db(new_db, strlen(new_db));
   DBUG_ASSERT(thd->query == 0);
   thd->query_length= 0;                         // Should not be needed
   thd->query_error= 0;
@@ -3018,7 +3037,7 @@
   const char *remember_db= thd->db;
   VOID(pthread_mutex_lock(&LOCK_thread_count));
   thd->catalog= 0;
-  thd->reset_db(NULL, 0);
+  thd->set_db(NULL, 0);
   thd->query= 0;
   thd->query_length= 0;
   VOID(pthread_mutex_unlock(&LOCK_thread_count));

--- 1.272/sql/slave.cc	2006-06-27 17:34:08 +04:00
+++ 1.273/sql/slave.cc	2006-06-28 03:17:19 +04:00
@@ -1177,24 +1177,6 @@
 }
 
 
-const char *rewrite_db(const char* db, uint *new_len)
-{
-  if (replicate_rewrite_db.is_empty() || !db)
-    return db;
-  I_List_iterator<i_string_pair> it(replicate_rewrite_db);
-  i_string_pair* tmp;
-
-  while ((tmp=it++))
-  {
-    if (!strcmp(tmp->key, db))
-    {
-      *new_len= (uint32)strlen(tmp->val);
-      return tmp->val;
-    }
-  }
-  return db;
-}
-
 /*
   From other comments and tests in code, it looks like
   sometimes Query_log_event and Load_log_event can have db == 0

--- 1.291/sql/sql_class.h	2006-06-27 14:56:13 +04:00
+++ 1.292/sql/sql_class.h	2006-06-28 03:17:19 +04:00
@@ -1574,21 +1574,23 @@
 
   /*
     Initialize the current database from a NULL-terminated string with length
+    If we run out of memory, we free the current database and return TRUE.
+    This way the user will notice the error as there will be no current
+    database selected (in addition to the error message set by malloc).
   */
-  void set_db(const char *new_db, uint new_db_len)
+  bool set_db(const char *new_db, uint new_db_len)
   {
-    if (new_db)
+    /* Do not reallocate memory if current chunk is big enough. */
+    if (db && new_db && db_length >= new_db_len)
+      memcpy(db, new_db, new_db_len+1);
+    else
     {
-      /* Do not reallocate memory if current chunk is big enough. */
-      if (db && db_length >= new_db_len)
-        memcpy(db, new_db, new_db_len+1);
-      else
-      {
-        safeFree(db);
-        db= my_strdup_with_length(new_db, new_db_len, MYF(MY_WME));
-      }
-      db_length= db ? new_db_len: 0;
+      safeFree(db);
+      db= new_db ? my_strdup_with_length(new_db, new_db_len, MYF(MY_WME)) :
+                   NULL;
     }
+    db_length= db ? new_db_len : 0;
+    return new_db && !db;
   }
   void reset_db(char *new_db, uint new_db_len)
   {

--- 1.129/sql/sql_db.cc	2006-06-27 00:50:50 +04:00
+++ 1.130/sql/sql_db.cc	2006-06-28 03:17:19 +04:00
@@ -781,32 +781,13 @@
 exit:
   (void)sp_drop_db_routines(thd, db); /* QQ Ignore errors for now  */
   /*
-    If this database was the client's selected database, we silently change the
-    client's selected database to nothing (to have an empty SELECT DATABASE()
-    in the future). For this we free() thd->db and set it to 0. But we don't do
-    free() for the slave thread. Indeed, doing a x_free() on it leads to nasty
-    problems (i.e. long painful debugging) because in this thread, thd->db is
-    the same as data_buf and db of the Query_log_event which is dropping the
-    database. So if you free() thd->db, you're freeing data_buf. You set
-    thd->db to 0 but not data_buf (thd->db and data_buf are two distinct
-    pointers which point to the same place). Then in ~Query_log_event(), we
-    have 'if (data_buf) free(data_buf)' data_buf is !=0 so this makes a
-    DOUBLE free().
-    Side effects of this double free() are, randomly (depends on the machine),
-    when the slave is replicating a DROP DATABASE:
-    - garbage characters in the error message:
-    "Error 'Can't drop database 'test2'; database doesn't exist' on query
-    - segfault
-    - hang in "free(vio)" (yes!) in the I/O or SQL slave threads (so slave
-    server hangs at shutdown etc).
+    If this database was the client's selected database, we silently
+    change the client's selected database to nothing (to have an empty
+    SELECT DATABASE() in the future). For this we free() thd->db and set
+    it to 0.
   */
   if (thd->db && !strcmp(thd->db, db))
-  {
-    if (!(thd->slave_thread)) /* a slave thread will free it itself */
-      x_free(thd->db);
-    thd->reset_db(NULL, 0);
-  }
+    thd->set_db(NULL, 0);
   VOID(pthread_mutex_unlock(&LOCK_mysql_create_db));
   start_waiting_global_read_lock(thd);
 exit2:
@@ -1103,12 +1084,12 @@
 
   SYNOPSIS
     mysql_change_db()
-    thd			Thread handler
-    name		Databasename
+    thd			Thread handle
+    name		Database name, can not be NULL
     no_access_check	True don't do access check. In this case name may be ""
 
   DESCRIPTION
-    Becasue the database name may have been given directly from the
+    Because the database name may have been given directly from the
     communication packet (in case of 'connect' or 'COM_INIT_DB')
     we have to do end space removal in this function.
 
@@ -1128,9 +1109,12 @@
 
 bool mysql_change_db(THD *thd, const char *name, bool no_access_check)
 {
-  int length, db_length;
-  char *dbname= thd->slave_thread ? (char *) name :
-                                    my_strdup((char *) name, MYF(MY_WME));
+  int path_length, db_length;
+  /*
+    We need to make a copy because check_db_name requires
+    a non-constant argument. TODO: fix check_db_name.
+  */
+  char *db_name= my_strdup(name, MYF(MY_WME));
   char	path[FN_REFLEN];
   HA_CREATE_INFO create;
   bool system_db= 0;
@@ -1144,30 +1128,27 @@
 
   LINT_INIT(db_length);
 
-  /* dbname can only be NULL if malloc failed */
-  if (!dbname || !(db_length= strlen(dbname)))
+  if (db_name == NULL || (db_length= strlen(db_name)) == 0)
   {
-    if (no_access_check && dbname)
+    if (no_access_check && db_name)
     {
       /* Called from SP when orignal database was not set */
       system_db= 1;
       goto end;
     }
-    if (!(thd->slave_thread))
-      x_free(dbname);				/* purecov: inspected */
+    x_free(db_name);				/* purecov: inspected */
     my_message(ER_NO_DB_ERROR, ER(ER_NO_DB_ERROR),
                MYF(0));                         /* purecov: inspected */
     DBUG_RETURN(1);				/* purecov: inspected */
   }
-  if (check_db_name(dbname))
+  if (check_db_name(db_name))
   {
-    my_error(ER_WRONG_DB_NAME, MYF(0), dbname);
-    if (!(thd->slave_thread))
-      my_free(dbname, MYF(0));
+    my_error(ER_WRONG_DB_NAME, MYF(0), db_name);
+    my_free(db_name, MYF(0));
     DBUG_RETURN(1);
   }
-  DBUG_PRINT("info",("Use database: %s", dbname));
-  if (!my_strcasecmp(system_charset_info, dbname, information_schema_name.str))
+  DBUG_PRINT("info",("Use database: %s", db_name));
+  if (!my_strcasecmp(system_charset_info, db_name, information_schema_name.str))
   {
     system_db= 1;
 #ifndef NO_EMBEDDED_ACCESS_CHECKS
@@ -1182,45 +1163,41 @@
     if (test_all_bits(sctx->master_access, DB_ACLS))
       db_access=DB_ACLS;
     else
-      db_access= (acl_get(sctx->host, sctx->ip, sctx->priv_user, dbname, 0) |
+      db_access= (acl_get(sctx->host, sctx->ip, sctx->priv_user, db_name, 0) |
                   sctx->master_access);
     if (!(db_access & DB_ACLS) && (!grant_option ||
-                                   check_grant_db(thd,dbname)))
+                                   check_grant_db(thd,db_name)))
     {
       my_error(ER_DBACCESS_DENIED_ERROR, MYF(0),
                sctx->priv_user,
                sctx->priv_host,
-               dbname);
+               db_name);
       mysql_log.write(thd, COM_INIT_DB, ER(ER_DBACCESS_DENIED_ERROR),
-                      sctx->priv_user, sctx->priv_host, dbname);
-      if (!(thd->slave_thread))
-        my_free(dbname,MYF(0));
+                      sctx->priv_user, sctx->priv_host, db_name);
+      my_free(db_name,MYF(0));
       DBUG_RETURN(1);
     }
   }
 #endif
-  (void) sprintf(path,"%s/%s",mysql_data_home,dbname);
-  length=unpack_dirname(path,path);		// Convert if not unix
-  if (length && path[length-1] == FN_LIBCHAR)
-    path[length-1]=0;				// remove ending '\'
+  (void) sprintf(path,"%s/%s", mysql_data_home, db_name);
+  path_length= unpack_dirname(path, path);      // Convert if not UNIX
+  if (path_length && path[path_length-1] == FN_LIBCHAR)
+    path[path_length-1]= '\0';                  // remove ending '\'
   if (my_access(path,F_OK))
   {
-    my_error(ER_BAD_DB_ERROR, MYF(0), dbname);
-    if (!(thd->slave_thread))
-      my_free(dbname,MYF(0));
+    my_error(ER_BAD_DB_ERROR, MYF(0), db_name);
+    my_free(db_name, MYF(0));
     DBUG_RETURN(1);
   }
 end:
-  if (!(thd->slave_thread))
-    x_free(thd->db);
-  if (dbname && dbname[0] == 0)
-  {
-    if (!(thd->slave_thread))
-      my_free(dbname, MYF(0));
-    thd->reset_db(NULL, 0);
+  x_free(thd->db);
+  DBUG_ASSERT(db_name && db_name[0]);
+  if (db_name && db_name[0] == '\0')
+  {
+    my_free(db_name, MYF(0));
+    db_name= 0;
   }
-  else
-    thd->reset_db(dbname, db_length);          // THD::~THD will free this
+  thd->reset_db(db_name, db_length);            // THD::~THD will free this
 #ifndef NO_EMBEDDED_ACCESS_CHECKS
   if (!no_access_check)
     sctx->db_access= db_access;

--- 1.94/sql/slave.h	2006-06-27 17:34:08 +04:00
+++ 1.95/sql/slave.h	2006-06-28 03:17:19 +04:00
@@ -550,7 +550,6 @@
 int add_wild_table_rule(DYNAMIC_ARRAY* a, const char* table_spec);
 void init_table_rule_hash(HASH* h, bool* h_inited);
 void init_table_rule_array(DYNAMIC_ARRAY* a, bool* a_inited);
-const char *rewrite_db(const char* db, uint *new_db_len);
 const char *print_slave_db_safe(const char *db);
 int check_expected_error(THD* thd, RELAY_LOG_INFO* rli, int error_code);
 void skip_load_data_infile(NET* net);
Thread
bk commit into 5.0 tree (konstantin:1.2207) BUG#19022konstantin28 Jun