List:Commits« Previous MessageNext Message »
From:Dmitry Lenev Date:September 2 2009 9:41am
Subject:bzr commit into mysql-5.4 branch (dlenev:2804) Bug#30977
View as plain text  
#At file:///home/dlenev/src/bzr/mysql-next-bg-pre1/ based on revid:alik@stripped

 2804 Dmitry Lenev	2009-09-02
      Pre-requisite patch for fixing bug #30977 "Concurrent statement
      using stored function and DROP FUNCTION breaks SBR".
      
      Changed open_tables() implementation to process stored routines only
      after tables which are directly used by statement were processed.
      
      Thanks to this change we can now start taking shared metadata locks 
      on stored routines used in statement without breaking one of metadata
      locking invariants for statements like "CREATE TABLE ... SELECT f1()".
      
      The problem was that for such statements we take exclusive metadata 
      lock on table being created and this invariant states that such locks 
      should not be taken if connection holds any other locks. So processing
      "f1()", which would have included taking shared metadata lock on this 
      routine, before processing table list element for table being created
      would have led to breaking this invariant.
     @ sql/sql_base.cc
        Refactored open_tables() implementation to process stored routines 
        only after tables which are directly used by statement were processed.
        
        To achieve this moved handling of routines in open_tables() (done 
        by open_routines() calls) to one place. This involved moving calls 
        to open_routines() out of loop which iterates over tables to a new 
        separate loop. And in its turn this allowed to split handling of
        tables and views to an auxiliary function, which made code in 
        open_tables() simplier and more easy to understand.
     @ storage/myisammrg/ha_myisammrg.cc
        Ensure that when we are adding or excluding elements for child 
        tables to/from the statement's table list LEX::query_tables_last 
        is updated to correct value so more elements can be added to the 
        table list.

    modified:
      sql/sql_base.cc
      storage/myisammrg/ha_myisammrg.cc
=== modified file 'sql/sql_base.cc'
--- a/sql/sql_base.cc	2009-08-27 06:22:17 +0000
+++ b/sql/sql_base.cc	2009-09-02 09:41:15 +0000
@@ -3718,9 +3718,10 @@ thr_lock_type read_lock_type_for_table(T
 
   @param[in]  thd                  Thread context.
   @param[in]  prelocking_ctx       Prelocking context.
-  @param[in]  start                First element in the list representing
-                                   subset of the prelocking set to be
-                                   processed.
+  @param[in,out] sroutine_to_open  Pointer to the next member in the last
+                                   element of the list representing
+                                   prelocking set which was processed
+                                   by prelocking algorithm.
   @param[in]  prelocking_strategy  Strategy which specifies how the
                                    prelocking set should be extended when
                                    one of its elements is processed.
@@ -3734,13 +3735,19 @@ thr_lock_type read_lock_type_for_table(T
 
 static bool
 open_routines(THD *thd, Query_tables_list *prelocking_ctx,
-              Sroutine_hash_entry *start,
+              Sroutine_hash_entry ***sroutine_to_open,
               Prelocking_strategy *prelocking_strategy,
               bool *need_prelocking)
 {
   DBUG_ENTER("open_routines");
 
-  for (Sroutine_hash_entry *rt= start; rt; rt= rt->next)
+  /*
+    We update '*sroutine_to_open' to point to next element of the last
+    Sroutine_hash_entry element which was processed, so we can reuse it
+    to resume open_routines() using this pointer when we need it.
+  */
+  for (Sroutine_hash_entry *rt= **sroutine_to_open; rt;
+       *sroutine_to_open= &rt->next, rt= rt->next)
   {
     int type= rt->key.str[0];
 
@@ -3769,114 +3776,58 @@ open_routines(THD *thd, Query_tables_lis
       DBUG_ASSERT(0);
     }
   }
+
   DBUG_RETURN(FALSE);
 }
 
 
 /**
-  Open all tables in list
-
-  @param[in]     thd      Thread context.
-  @param[in,out] start    List of tables to be open (it can be adjusted for
-                          statement that uses tables only implicitly, e.g.
-                          for "SELECT f1()").
-  @param[out]    counter  Number of tables which were open.
-  @param[in]     flags    Bitmap of flags to modify how the tables will be
-                          open, see open_table() description for details.
-  @param[in]     prelocking_strategy  Strategy which specifies how prelocking
-                                      algorithm should work for this statement.
-
-  @note
-    Unless we are already in prelocked mode and prelocking strategy prescribes
-    so this function will also precache all SP/SFs explicitly or implicitly
-    (via views and triggers) used by the query and add tables needed for their
-    execution to table list. Statement that uses SFs, invokes triggers or
-    requires foreign key checks will be marked as requiring prelocking.
-    Prelocked mode will be enabled for such query during lock_tables() call.
-
-    If query for which we are opening tables is already marked as requiring
-    prelocking it won't do such precaching and will simply reuse table list
-    which is already built.
+  Perform steps of prelocking algorithm for tables and views. I.e. obtain
+  metadata locks on them, open them and, if prelocking strategy prescribes
+  so, extend the prelocking set with tables and routines used by them.
+
+  @param[in]     thd                  Thread context.
+  @param[in]     lex                  LEX structure for statement.
+  @param[in,out] table_to_open        Pointer to the next_global member in the
+                                      last table list element which was already
+                                      processed by prelocking algorithm.
+  @param[in,out] counter              Number of tables which are open.
+  @param[in]     flags                Bitmap of flags to modify how the tables
+                                      will be open, see open_table() description
+                                      for details.
+  @param[in]     prelocking_strategy  Strategy which specifies how the
+                                      prelocking set should be extended
+                                      when table or view is processed.
+  @param[in]     has_prelocking_list  Indicates that prelocking set/list for
+                                      this statement has already been built.
+  @param[in]     ot_ctx               Context used to recover from a failed
+                                      open_table() attempt.
+  @param[in]     new_frm_mem          Temporary MEM_ROOT to be used for
+                                      parsing .FRMs for views.
 
   @retval  FALSE  Success.
-  @retval  TRUE   Error, reported.
+  @retval  TRUE   Error, reported unless there is a chance to recover from it.
 */
 
-bool open_tables(THD *thd, TABLE_LIST **start, uint *counter, uint flags,
-                Prelocking_strategy *prelocking_strategy)
+static bool
+open_tables_impl(THD *thd, LEX *lex, TABLE_LIST ***table_to_open,
+                 uint *counter, uint flags,
+                 Prelocking_strategy *prelocking_strategy,
+                 bool has_prelocking_list,
+                 Open_table_context *ot_ctx,
+                 MEM_ROOT *new_frm_mem)
 {
-  TABLE_LIST *tables= NULL;
-  Open_table_context ot_ctx(thd);
+  TABLE_LIST *tables;
   bool error= FALSE;
-  MEM_ROOT new_frm_mem;
-  /* Also used for indicating that prelocking is need */
-  TABLE_LIST **query_tables_last_own;
   bool safe_to_ignore_table;
-  DBUG_ENTER("open_tables");
-
-  /*
-    temporary mem_root for new .frm parsing.
-    TODO: variables for size
-  */
-  init_sql_alloc(&new_frm_mem, 8024, 8024);
-
-  thd->current_tablenr= 0;
- restart:
-  *counter= 0;
-  query_tables_last_own= 0;
-  thd_proc_info(thd, "Opening tables");
-
-  /*
-    Close HANDLER tables which are marked for flush or against which there
-    are pending exclusive metadata locks. Note that we do this not to avoid
-    deadlocks (calls to mysql_ha_flush() in mdl_wait_for_locks() and
-    tdc_wait_for_old_version() are enough for this) but in order to have
-    a point during statement execution at which such HANDLERs are closed
-    even if they don't create problems for current thread (i.e. to avoid
-    having DDL blocked by HANDLERs opened for long time).
-  */
-  if (thd->handler_tables)
-    mysql_ha_flush(thd);
-
-  /*
-    If we are not already executing prelocked statement and don't have
-    statement for which table list for prelocking is already built, let
-    us cache routines and try to build such table list.
-  */
-
-  if (thd->locked_tables_mode <= LTM_LOCK_TABLES &&
-      !thd->lex->requires_prelocking() &&
-      thd->lex->uses_stored_routines())
-  {
-    bool need_prelocking= FALSE;
-    TABLE_LIST **save_query_tables_last= thd->lex->query_tables_last;
-
-    DBUG_ASSERT(thd->lex->query_tables == *start);
-
-    error= open_routines(thd, thd->lex,
-                         (Sroutine_hash_entry *)thd->lex->sroutines_list.first,
-                         prelocking_strategy, &need_prelocking);
-    if (error)
-    {
-      /*
-        Serious error during reading stored routines from mysql.proc table.
-        Something's wrong with the table or its contents, and an error has
-        been emitted; we must abort.
-      */
-      goto err;
-    }
-    else if (need_prelocking)
-    {
-      query_tables_last_own= save_query_tables_last;
-      *start= thd->lex->query_tables;
-    }
-  }
+  DBUG_ENTER("open_tables_impl");
 
   /*
     For every table in the list of tables to open, try to find or open
     a table.
   */
-  for (tables= *start; tables ;tables= tables->next_global)
+  for (tables= **table_to_open; tables;
+       *table_to_open= &tables->next_global, tables= tables->next_global)
   {
     safe_to_ignore_table= FALSE;
 
@@ -3916,7 +3867,7 @@ bool open_tables(THD *thd, TABLE_LIST **
       */
       if (tables->view)
         goto process_view_routines;
-      if (!mysql_schema_table(thd, thd->lex, tables) &&
+      if (!mysql_schema_table(thd, lex, tables) &&
           !check_and_update_table_version(thd, tables, tables->table->s))
       {
         continue;
@@ -3940,47 +3891,19 @@ bool open_tables(THD *thd, TABLE_LIST **
       */
       Prelock_error_handler prelock_handler;
       thd->push_internal_handler(& prelock_handler);
-      error= open_table(thd, tables, &new_frm_mem, &ot_ctx, flags);
+      error= open_table(thd, tables, new_frm_mem, ot_ctx, flags);
       thd->pop_internal_handler();
       safe_to_ignore_table= prelock_handler.safely_trapped_errors();
     }
     else
-      error= open_table(thd, tables, &new_frm_mem, &ot_ctx, flags);
+      error= open_table(thd, tables, new_frm_mem, ot_ctx, flags);
 
-    free_root(&new_frm_mem, MYF(MY_KEEP_PREALLOC));
+    free_root(new_frm_mem, MYF(MY_KEEP_PREALLOC));
 
     if (error)
     {
-      if (ot_ctx.can_recover_from_failed_open_table())
-      {
-        /*
-          We have met exclusive metadata lock or old version of table. Now we
-          have to close all tables which are not up to date/release metadata
-          locks. We also have to throw away set of prelocked tables (and thus
-          close tables from this set that were open by now) since it possible
-          that one of tables which determined its content was changed.
-
-          Instead of implementing complex/non-robust logic mentioned
-          above we simply close and then reopen all tables.
-
-          In order to prepare for recalculation of set of prelocked tables
-          we pretend that we have finished calculation which we were doing
-          currently.
-        */
-        if (query_tables_last_own)
-          thd->lex->mark_as_requiring_prelocking(query_tables_last_own);
-        close_tables_for_reopen(thd, start);
-        /*
-          Here we rely on the fact that 'tables' still points to the valid
-          TABLE_LIST element. Altough currently this assumption is valid
-          it may change in future.
-        */
-        if (ot_ctx.recover_from_failed_open_table_attempt(thd, tables))
-          goto err;
-
-        error= FALSE;
-	goto restart;
-      }
+      if (ot_ctx->can_recover_from_failed_open_table())
+        goto err;
 
       if (safe_to_ignore_table)
       {
@@ -4014,9 +3937,9 @@ bool open_tables(THD *thd, TABLE_LIST **
         has added its base tables after itself, adjust the boundary pointer
         accordingly.
       */
-      if (query_tables_last_own == &(tables->next_global) &&
+      if (lex->query_tables_own_last == &(tables->next_global) &&
           tables->view->query_tables)
-        query_tables_last_own= tables->view->query_tables_last;
+        lex->query_tables_own_last= tables->view->query_tables_last;
       /*
         Let us free memory used by 'sroutines' hash here since we never
         call destructor for this LEX.
@@ -4041,15 +3964,11 @@ bool open_tables(THD *thd, TABLE_LIST **
       to be changed.
     */
     if (thd->locked_tables_mode <= LTM_LOCK_TABLES &&
-        !thd->lex->requires_prelocking() &&
+        ! has_prelocking_list &&
         tables->lock_type >= TL_WRITE_ALLOW_WRITE)
     {
       bool need_prelocking= FALSE;
-      bool not_used;
-      TABLE_LIST **save_query_tables_last= thd->lex->query_tables_last;
-      Sroutine_hash_entry **sroutines_next= 
-        (Sroutine_hash_entry **)thd->lex->sroutines_list.next;
-
+      TABLE_LIST **save_query_tables_last= lex->query_tables_last;
       /*
         Extend statement's table list and the prelocking set with
         tables and routines according to the current prelocking
@@ -4059,25 +3978,12 @@ bool open_tables(THD *thd, TABLE_LIST **
         used by triggers which are going to be invoked for this element of
         table list and also add tables required for handling of foreign keys.
       */
-      error= prelocking_strategy->handle_table(thd, thd->lex, tables,
+      error= prelocking_strategy->handle_table(thd, lex, tables,
                                                &need_prelocking);
 
-      if (need_prelocking && ! query_tables_last_own)
-        query_tables_last_own= save_query_tables_last;
-
-      if (error)
-        goto err;
+      if (need_prelocking && ! lex->requires_prelocking())
+        lex->mark_as_requiring_prelocking(save_query_tables_last);
 
-      /*
-        Process elements of the prelocking set which were added
-        by the above invocation of Prelocking_strategy method.
-
-        For example, if new element is a routine, cache it and then, if
-        prelocking strategy prescribes so, add tables it uses to the table
-        list and routines it might invoke to the prelocking set.
-      */
-      error= open_routines(thd, thd->lex, *sroutines_next, prelocking_strategy,
-                           &not_used);
       if (error)
         goto err;
     }
@@ -4120,25 +4026,169 @@ process_view_routines:
     */
     if (tables->view &&
         thd->locked_tables_mode <= LTM_LOCK_TABLES &&
-        !thd->lex->requires_prelocking())
+        ! has_prelocking_list)
     {
       bool need_prelocking= FALSE;
-      bool not_used;
-      TABLE_LIST **save_query_tables_last= thd->lex->query_tables_last;
-      Sroutine_hash_entry **sroutines_next= 
-        (Sroutine_hash_entry **)thd->lex->sroutines_list.next;
+      TABLE_LIST **save_query_tables_last= lex->query_tables_last;
 
-      error= prelocking_strategy->handle_view(thd, thd->lex, tables,
+      error= prelocking_strategy->handle_view(thd, lex, tables,
                                               &need_prelocking);
 
-      if (need_prelocking && ! query_tables_last_own)
-        query_tables_last_own= save_query_tables_last;
+      if (need_prelocking && ! lex->requires_prelocking())
+        lex->mark_as_requiring_prelocking(save_query_tables_last);
 
       if (error)
         goto err;
+    }
+  }
+
+err:
+  DBUG_RETURN(error);
+}
+
+
+/**
+  Open all tables in list
+
+  @param[in]     thd      Thread context.
+  @param[in,out] start    List of tables to be open (it can be adjusted for
+                          statement that uses tables only implicitly, e.g.
+                          for "SELECT f1()").
+  @param[out]    counter  Number of tables which were open.
+  @param[in]     flags    Bitmap of flags to modify how the tables will be
+                          open, see open_table() description for details.
+  @param[in]     prelocking_strategy  Strategy which specifies how prelocking
+                                      algorithm should work for this statement.
+
+  @note
+    Unless we are already in prelocked mode and prelocking strategy prescribes
+    so this function will also precache all SP/SFs explicitly or implicitly
+    (via views and triggers) used by the query and add tables needed for their
+    execution to table list. Statement that uses SFs, invokes triggers or
+    requires foreign key checks will be marked as requiring prelocking.
+    Prelocked mode will be enabled for such query during lock_tables() call.
+
+    If query for which we are opening tables is already marked as requiring
+    prelocking it won't do such precaching and will simply reuse table list
+    which is already built.
 
-      error= open_routines(thd, thd->lex, *sroutines_next, prelocking_strategy,
-                           &not_used);
+  @retval  FALSE  Success.
+  @retval  TRUE   Error, reported.
+*/
+
+bool open_tables(THD *thd, TABLE_LIST **start, uint *counter, uint flags,
+                Prelocking_strategy *prelocking_strategy)
+{
+  /*
+    We use pointers to "next_global" member in the last processed TABLE_LIST
+    element and to the "next" member in the last processed Sroutine_hash_entry
+    element as iterators over, correspondingly, the table list and stored routines
+    list which stay valid and allow to continue iteration when new elements are
+    added to the tail of the lists.
+  */
+  TABLE_LIST **table_to_open;
+  Sroutine_hash_entry **sroutine_to_open;
+  Open_table_context ot_ctx(thd);
+  bool error= FALSE;
+  MEM_ROOT new_frm_mem;
+  bool has_prelocking_list= thd->lex->requires_prelocking();
+  DBUG_ENTER("open_tables");
+
+  /*
+    temporary mem_root for new .frm parsing.
+    TODO: variables for size
+  */
+  init_sql_alloc(&new_frm_mem, 8024, 8024);
+
+  thd->current_tablenr= 0;
+ restart:
+  table_to_open= start;
+  sroutine_to_open= (Sroutine_hash_entry**) &thd->lex->sroutines_list.first;
+  *counter= 0;
+  thd_proc_info(thd, "Opening tables");
+
+  /*
+    Close HANDLER tables which are marked for flush or against which there
+    are pending exclusive metadata locks. Note that we do this not to avoid
+    deadlocks (calls to mysql_ha_flush() in mdl_wait_for_locks() and
+    tdc_wait_for_old_version() are enough for this) but in order to have
+    a point during statement execution at which such HANDLERs are closed
+    even if they don't create problems for current thread (i.e. to avoid
+    having DDL blocked by HANDLERs opened for long time).
+  */
+  if (thd->handler_tables)
+    mysql_ha_flush(thd);
+
+
+  /*
+    Perform steps of prelocking algorithm until there are unprocessed
+    elements in prelocking list/set.
+  */
+
+  while (*table_to_open  ||
+         (thd->locked_tables_mode <= LTM_LOCK_TABLES &&
+          ! has_prelocking_list &&
+          *sroutine_to_open))
+  {
+    error= open_tables_impl(thd, thd->lex, &table_to_open, counter, flags,
+                            prelocking_strategy, has_prelocking_list, &ot_ctx,
+                            &new_frm_mem);
+
+    if (error)
+    {
+      if (ot_ctx.can_recover_from_failed_open_table())
+      {
+        /*
+          We have met exclusive metadata lock or old version of table. Now we
+          have to close all tables which are not up to date/release metadata
+          locks. We also have to throw away set of prelocked tables (and thus
+          close tables from this set that were open by now) since it possible
+          that one of tables which determined its content was changed.
+
+          Instead of implementing complex/non-robust logic mentioned
+          above we simply close and then reopen all tables.
+
+          We have to save pointer to table list element for table which we
+          have failed to open since closing tables can trigger removal of
+          elements from the table list (if MERGE tables are involved),
+        */
+        TABLE_LIST *failed_table= *table_to_open;
+        close_tables_for_reopen(thd, start);
+        /*
+          Here we rely on the fact that 'tables' still points to the valid
+          TABLE_LIST element. Altough currently this assumption is valid
+          it may change in future.
+        */
+        if (ot_ctx.recover_from_failed_open_table_attempt(thd, failed_table))
+          DBUG_RETURN(TRUE);
+
+        error= FALSE;
+        goto restart;
+      }
+      goto err;
+    }
+
+    /*
+      If we are not already in prelocked mode and extended table list is
+      not yet built for our statement we need to cache routines it uses
+      and build the prelocking list for it.
+    */
+    if (thd->locked_tables_mode <= LTM_LOCK_TABLES && ! has_prelocking_list)
+    {
+      bool need_prelocking= FALSE;
+      TABLE_LIST **save_query_tables_last= thd->lex->query_tables_last;
+      /*
+        Process elements of the prelocking set which are present there
+        since parsing stage or were added to it by invocations of
+        Prelocking_strategy methods in the above loop over tables.
+
+        For example, if element is a routine, cache it and then,
+        if prelocking strategy prescribes so, add tables it uses to the
+        table list and routines it might invoke to the prelocking set.
+      */
+      error= open_routines(thd, thd->lex, &sroutine_to_open,
+                           prelocking_strategy,
+                           &need_prelocking);
 
       if (error)
       {
@@ -4149,6 +4199,12 @@ process_view_routines:
         */
         goto err;
       }
+
+      if (need_prelocking && ! thd->lex->requires_prelocking())
+        thd->lex->mark_as_requiring_prelocking(save_query_tables_last);
+
+      if (need_prelocking && ! *start)
+        *start= thd->lex->query_tables;
     }
   }
 
@@ -4158,7 +4214,7 @@ process_view_routines:
     the children are detached. Attaching and detaching are always done,
     even under LOCK TABLES.
   */
-  for (tables= *start; tables; tables= tables->next_global)
+  for (TABLE_LIST *tables= *start; tables; tables= tables->next_global)
   {
     TABLE *tbl= tables->table;
 
@@ -4179,12 +4235,9 @@ err:
   thd_proc_info(thd, 0);
   free_root(&new_frm_mem, MYF(0));              // Free pre-alloced block
 
-  if (query_tables_last_own)
-    thd->lex->mark_as_requiring_prelocking(query_tables_last_own);
-
-  if (error && tables)
+  if (error && *table_to_open)
   {
-    tables->table= NULL;
+    (*table_to_open)->table= NULL;
   }
   DBUG_PRINT("open_tables", ("returning: %d", (int) error));
   DBUG_RETURN(error);

=== modified file 'storage/myisammrg/ha_myisammrg.cc'
--- a/storage/myisammrg/ha_myisammrg.cc	2009-08-13 22:17:20 +0000
+++ b/storage/myisammrg/ha_myisammrg.cc	2009-09-02 09:41:15 +0000
@@ -452,6 +452,13 @@ int ha_myisammrg::add_children_list(void
   *this->children_last_l= parent_l->next_global;
   parent_l->next_global= this->children_l;
   this->children_l->prev_global= &parent_l->next_global;
+  /*
+    We have to update LEX::query_tables_last if children are added to
+    the tail of the table list in order to be able correctly add more
+    elements to it (e.g. as part of prelocking process).
+  */
+  if (thd->lex->query_tables_last == &parent_l->next_global)
+    thd->lex->query_tables_last= this->children_last_l;
 
 end:
   DBUG_RETURN(0);
@@ -836,6 +843,8 @@ int ha_myisammrg::detach_children(void)
 
   if (this->children_l)
   {
+    THD *thd= table->in_use;
+
     /* Clear TABLE references. */
     for (child_l= this->children_l; ; child_l= child_l->next_global)
     {
@@ -873,6 +882,14 @@ int ha_myisammrg::detach_children(void)
     if (*this->children_last_l)
       (*this->children_last_l)->prev_global= this->children_l->prev_global;
 
+    /*
+      If table elements being removed are at the end of table list we
+      need to adjust LEX::query_tables_last member to point to the
+      new last element of the list.
+    */
+    if (thd->lex->query_tables_last == this->children_last_l)
+      thd->lex->query_tables_last= this->children_l->prev_global;
+
     /* Terminate child list. So it cannot be tried to remove again. */
     *this->children_last_l= NULL;
     this->children_l->prev_global= NULL;


Attachment: [text/bzr-bundle] bzr/dlenev@mysql.com-20090902094115-5x4kucvway09ry0e.bundle
Thread
bzr commit into mysql-5.4 branch (dlenev:2804) Bug#30977Dmitry Lenev2 Sep
  • Re: bzr commit into mysql-5.4 branch (dlenev:2804) Bug#30977Davi Arnaut12 Sep