MySQL Lists are EOL. Please join:

List:Internals« Previous MessageNext Message »
From:dlenev Date:October 17 2005 6:37pm
Subject:bk commit into 5.0 tree (dlenev:1.2030) BUG#12739
View as plain text  
Below is the list of changes that have just been committed into a local
5.0 repository of dlenev. When dlenev 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.2030 05/10/17 22:37:24 dlenev@stripped +4 -0
  Fix for bug #12739 "Deadlock in multithreaded environment during creating/
  droping trigger on InnoDB table".
  
  Deadlock occured in cases when we were trying to create two triggers for
  the same InnoDB table concurrently and both threads were able to reach
  close_cached_table() simultaneously. Bugfix implements new approach to
  table locking and table cache invalidation during creation/dropping
  of trigger.
  
  No testcase is supplied since bug was repeatable only under high concurrency.

  sql/sql_trigger.cc
    1.30 05/10/17 22:37:08 dlenev@stripped +39 -37
    mysql_create_or_drop_trigger():
      Now instead of opening and locking table, creating trigger, and then trying
      to invalidate all instances of this table in table cache, we obtain name
      lock on table first (thus ensuring that no other thread has this table
      open), open it, create trigger and then close table therefore releasing lock.
      New approach is more in line with other places where change .frm files
      (i.e. change table meta-data).
      With this change we also get rid of deadlock which occured in cases when we
      were trying to create two triggers for the same InnoDB table concurrently
      and both threads were able to reach close_cached_table() simultaneously.
      (Alternative was to forbid to InnoDB downgrade locks for CREATE/DROP
       TRIGGER statements in one way or another but I think that proposed
       solution is better long term).

  sql/sql_table.cc
    1.278 05/10/17 22:37:08 dlenev@stripped +13 -6
    prepare_for_restore()/prepare_for_repair():
      We should not prefer connection's current database over database which was
      specified in TABLE_LIST::db member (even if database is not explicitly
      specified for table in original query TABLE_LIST::db will be set properly
      at parsing stage). Fixed behavior in unlikely case when we are unable
      to open table which we are restoring/reparing at the end of preparation
      stage.

  sql/sql_base.cc
    1.310 05/10/17 22:37:07 dlenev@stripped +42 -18
    reopen_name_locked_table():
      Changed function signature to make it more robust against erroneous usage.
      Obtaining LOCK_open lock is now resposibility of caller.
      When searching for the table to open we should not prefer connection's current
      database over database which was explicitly specified in TABLE_LIST::db member
      (even if database is not explicitly specified for table in original query
      TABLE_LIST::db will be set properly at parsing stage).
      Fixed behavior of function in cases when error occurs during opening of table.

  sql/mysql_priv.h
    1.360 05/10/17 22:37:07 dlenev@stripped +1 -1
    reopen_name_locked_table():
      Changed function signature to make it more robust against erroneous usage.
      

# 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:	dlenev
# Host:	brandersnatch.site
# Root:	/home/dlenev/src/mysql-5.0-bg12739

--- 1.359/sql/mysql_priv.h	2005-10-06 02:40:54 +04:00
+++ 1.360/sql/mysql_priv.h	2005-10-17 22:37:07 +04:00
@@ -757,7 +757,7 @@
 TABLE *open_ltable(THD *thd, TABLE_LIST *table_list, thr_lock_type update);
 TABLE *open_table(THD *thd, TABLE_LIST *table_list, MEM_ROOT* mem,
 		  bool *refresh, uint flags);
-TABLE *reopen_name_locked_table(THD* thd, TABLE_LIST* table);
+bool reopen_name_locked_table(THD* thd, TABLE_LIST* table);
 TABLE *find_locked_table(THD *thd, const char *db,const char *table_name);
 bool reopen_table(TABLE *table,bool locked);
 bool reopen_tables(THD *thd,bool get_locks,bool in_refresh);

--- 1.309/sql/sql_base.cc	2005-10-04 18:35:55 +04:00
+++ 1.310/sql/sql_base.cc	2005-10-17 22:37:07 +04:00
@@ -976,32 +976,57 @@
 }
 
 
-TABLE *reopen_name_locked_table(THD* thd, TABLE_LIST* table_list)
+/*
+  Open table which is already name-locked by this thread.
+
+  SYNOPSIS
+    reopen_name_locked_table()
+      thd         Thread handle
+      table_list  TABLE_LIST object for table to be open, TABLE_LIST::table
+                  member should point to TABLE object which was used for
+                  name-locking.
+
+  NOTE
+    This function assumes that its caller already acquired LOCK_open mutex.
+
+  RETURN VALUE
+    FALSE - Success
+    TRUE  - Error
+*/
+
+bool reopen_name_locked_table(THD* thd, TABLE_LIST* table_list)
 {
-  DBUG_ENTER("reopen_name_locked_table");
-  if (thd->killed)
-    DBUG_RETURN(0);
-  TABLE *table;
+  TABLE *table= table_list->table;
   TABLE_SHARE *share;
-  if (!(table = table_list->table))
-    DBUG_RETURN(0);
+  char *db= table_list->db;
+  char *table_name= table_list->table_name;
+  char key[MAX_DBKEY_LENGTH];
+  uint key_length;
+  TABLE orig_table;
+  DBUG_ENTER("reopen_name_locked_table");
+
+  safe_mutex_assert_owner(&LOCK_open);
+
+  if (thd->killed || !table)
+    DBUG_RETURN(TRUE);
 
-  char* db = thd->db ? thd->db : table_list->db;
-  char* table_name = table_list->table_name;
-  char	key[MAX_DBKEY_LENGTH];
-  uint	key_length;
+  orig_table= *table;
   key_length=(uint) (strmov(strmov(key,db)+1,table_name)-key)+1;
 
-  pthread_mutex_lock(&LOCK_open);
   if (open_unireg_entry(thd, table, db, table_name, table_name, 0,
                         thd->mem_root) ||
       !(table->s->table_cache_key= memdup_root(&table->mem_root, (char*) key,
                                                key_length)))
   {
-    delete table->triggers;
-    closefrm(table);
-    pthread_mutex_unlock(&LOCK_open);
-    DBUG_RETURN(0);
+    intern_close_table(table);
+    /*
+      If there was an error during opening of table (for example if it
+      does not exist) '*table' object can be wiped out. To be able
+      properly release name-lock in this case we should restore this
+      object to its original state.
+    */
+    *table= orig_table;
+    DBUG_RETURN(TRUE);
   }
 
   share= table->s;
@@ -1011,7 +1036,6 @@
   share->flush_version=0;
   table->in_use = thd;
   check_unused();
-  pthread_mutex_unlock(&LOCK_open);
   table->next = thd->open_tables;
   thd->open_tables = table;
   table->tablenr=thd->current_tablenr++;
@@ -1021,7 +1045,7 @@
   table->status=STATUS_NO_RECORD;
   table->keys_in_use_for_query= share->keys_in_use;
   table->used_keys= share->keys_for_keyread;
-  DBUG_RETURN(table);
+  DBUG_RETURN(FALSE);
 }
 
 

--- 1.277/sql/sql_table.cc	2005-10-05 21:36:48 +04:00
+++ 1.278/sql/sql_table.cc	2005-10-17 22:37:08 +04:00
@@ -1950,8 +1950,8 @@
   {
     char* backup_dir= thd->lex->backup_dir;
     char src_path[FN_REFLEN], dst_path[FN_REFLEN];
-    char* table_name = table->table_name;
-    char* db = thd->db ? thd->db : table->db;
+    char* table_name= table->table_name;
+    char* db= table->db;
 
     if (fn_format_relative_to_data_home(src_path, table_name, backup_dir,
 					reg_ext))
@@ -1987,12 +1987,15 @@
     Now we should be able to open the partially restored table
     to finish the restore in the handler later on
   */
-  if (!(table->table = reopen_name_locked_table(thd, table)))
+  pthread_mutex_lock(&LOCK_open);
+  if (reopen_name_locked_table(thd, table))
   {
-    pthread_mutex_lock(&LOCK_open);
     unlock_table_name(thd, table);
     pthread_mutex_unlock(&LOCK_open);
+    DBUG_RETURN(send_check_errmsg(thd, table, "restore",
+                                  "Failed to open partially restored table"));
   }
+  pthread_mutex_unlock(&LOCK_open);
   DBUG_RETURN(0);
 }
 
@@ -2089,12 +2092,16 @@
     Now we should be able to open the partially repaired table
     to finish the repair in the handler later on.
   */
-  if (!(table_list->table = reopen_name_locked_table(thd, table_list)))
+  pthread_mutex_lock(&LOCK_open);
+  if (reopen_name_locked_table(thd, table_list))
   {
-    pthread_mutex_lock(&LOCK_open);
     unlock_table_name(thd, table_list);
     pthread_mutex_unlock(&LOCK_open);
+    error= send_check_errmsg(thd, table_list, "repair",
+                             "Failed to open partially repaired table");
+    goto end;
   }
+  pthread_mutex_unlock(&LOCK_open);
 
 end:
   if (table == &tmp_table)

--- 1.29/sql/sql_trigger.cc	2005-09-15 23:29:01 +04:00
+++ 1.30/sql/sql_trigger.cc	2005-10-17 22:37:08 +04:00
@@ -103,8 +103,7 @@
 bool mysql_create_or_drop_trigger(THD *thd, TABLE_LIST *tables, bool create)
 {
   TABLE *table;
-  bool result= 0;
-
+  bool result= TRUE;
   DBUG_ENTER("mysql_create_or_drop_trigger");
 
   /*
@@ -119,9 +118,6 @@
   /* We should have only one table in table list. */
   DBUG_ASSERT(tables->next_global == 0);
 
-  if (!(table= open_ltable(thd, tables, tables->lock_type)))
-    DBUG_RETURN(TRUE);
-
   /*
     TODO: We should check if user has TRIGGER privilege for table here.
     Now we just require SUPER privilege for creating/dropping because
@@ -131,28 +127,24 @@
     DBUG_RETURN(TRUE);
 
   /*
-    We do not allow creation of triggers on temporary tables. We also don't
-    allow creation of triggers on views but fulfilment of this restriction
-    is guaranteed by open_ltable(). It is better to have this check here
-    than do it in Table_triggers_list::create_trigger() and mess with table
-    cache.
+    There is no DETERMINISTIC clause for triggers, so can't check it.
+    But a trigger can in theory be used to do nasty things (if it supported
+    DROP for example) so we do the check for privileges. For now there is
+    already a stronger test right above; but when this stronger test will
+    be removed, the test below will hold.
   */
-  if (table->s->tmp_table != NO_TMP_TABLE)
+  if (!trust_routine_creators && mysql_bin_log.is_open() &&
+      !(thd->security_ctx->master_access & SUPER_ACL))
   {
-    my_error(ER_TRG_ON_VIEW_OR_TEMP_TABLE, MYF(0), tables->alias);
+    my_error(ER_BINLOG_CREATE_ROUTINE_NEED_SUPER, MYF(0));
     DBUG_RETURN(TRUE);
   }
 
-  if (!table->triggers)
+  /* We do not allow creation of triggers on temporary tables. */
+  if (create && find_temporary_table(thd, tables->db, tables->table_name))
   {
-    if (!create)
-    {
-      my_message(ER_TRG_DOES_NOT_EXIST, ER(ER_TRG_DOES_NOT_EXIST), MYF(0));
-      DBUG_RETURN(TRUE);
-    }
-
-    if (!(table->triggers= new (&table->mem_root) Table_triggers_list(table)))
-      DBUG_RETURN(TRUE);
+    my_error(ER_TRG_ON_VIEW_OR_TEMP_TABLE, MYF(0), tables->alias);
+    DBUG_RETURN(TRUE);
   }
 
   /*
@@ -161,31 +153,41 @@
     again until we are done. (Acquiring LOCK_open is not enough because
     global read lock is held without helding LOCK_open).
   */
-  if (wait_if_global_read_lock(thd, 0, 0))
+  if (wait_if_global_read_lock(thd, 0, 1))
     DBUG_RETURN(TRUE);
 
-  /*
-    There is no DETERMINISTIC clause for triggers, so can't check it.
-    But a trigger can in theory be used to do nasty things (if it supported
-    DROP for example) so we do the check for privileges. For now there is
-    already a stronger test above (see start of the function); but when this
-    stronger test will be removed, the test below will hold.
-  */
-  if (!trust_routine_creators && mysql_bin_log.is_open() &&
-      !(thd->security_ctx->master_access & SUPER_ACL))
+  VOID(pthread_mutex_lock(&LOCK_open));
+
+  if (lock_table_names(thd, tables))
+    goto end;
+
+  /* We also don't allow creation of triggers on views. */
+  tables->required_type= FRMTYPE_TABLE;
+
+  if (reopen_name_locked_table(thd, tables))
   {
-    my_message(ER_BINLOG_CREATE_ROUTINE_NEED_SUPER,
-	       ER(ER_BINLOG_CREATE_ROUTINE_NEED_SUPER), MYF(0));
-    DBUG_RETURN(TRUE);
+    unlock_table_name(thd, tables);
+    goto end;
+  }
+  table= tables->table;
+
+  if (!table->triggers)
+  {
+    if (!create)
+    {
+      my_error(ER_TRG_DOES_NOT_EXIST, MYF(0));
+      goto end;
+    }
+
+    if (!(table->triggers= new (&table->mem_root) Table_triggers_list(table)))
+      goto end;
   }
 
-  VOID(pthread_mutex_lock(&LOCK_open));
   result= (create ?
            table->triggers->create_trigger(thd, tables):
            table->triggers->drop_trigger(thd, tables));
 
-  /* It is sensible to invalidate table in any case */
-  close_cached_table(thd, table);
+end:
   VOID(pthread_mutex_unlock(&LOCK_open));
   start_waiting_global_read_lock(thd);
 
Thread
bk commit into 5.0 tree (dlenev:1.2030) BUG#12739dlenev17 Oct