Bug #50461 many uses of btr_search_latch with innodb_adaptive_hash_index is OFF
Submitted: 20 Jan 2010 0:57 Modified: 12 Jan 2011 22:41
Reporter: Mark Callaghan Email Updates:
Status: Closed Impact on me:
Category:MySQL Server: InnoDB storage engine Severity:S5 (Performance)
Version:5.0.84, 5.1.44, 5.5.99-m3 OS:Any
Assigned to: Jimmy Yang CPU Architecture:Any
Tags: btr_search_latch, innodb_adaptive_hash_index

[20 Jan 2010 0:57] Mark Callaghan
Many uses of btr_search_latch remain when innodb_adaptive_hash_index is OFF and srv_use_adaptive_hash_indexes is FALSE. The most important one to remove is in row_search_for_mysql in the 'phase 2' block of code.

Others that use btr_search_latch when InnoDB regression tests are run include:
* btr_search_info_get_ref_count
* btr_search_drop_page_hash_index
* btr_search_move_or_delete_hash_entries

How to repeat:
Read the code
Run regression tests with asserts in the functions that get rw-locks to confirm that btr_search_latch is used

Suggested fix:
Return early when srv_use_adaptive_hash_indexes == FALSE. Do this before trying to lock btr_search_latch.
[28 Jan 2010 21:19] Mark Leith
Verified as described. 

The following would fix those mentioned:

=== modified file 'storage/innobase/btr/btr0sea.c'
--- storage/innobase/btr/btr0sea.c      revid:build@mysql.com-20100115170348-qyaxw83mnms3gx8y
+++ storage/innobase/btr/btr0sea.c      2010-01-28 20:49:08 +0000
@@ -196,6 +196,9 @@
        ulint ret;
+       if (!srv_use_adaptive_hash_indexes)
+               return;
@@ -943,6 +946,10 @@
Returns the value of ref_count. The value is protected by
        ut_ad(!rw_lock_own(&btr_search_latch, RW_LOCK_SHARED));
        ut_ad(!rw_lock_own(&btr_search_latch, RW_LOCK_EX));
 #endif /* UNIV_SYNC_DEBUG */
+        if (!srv_use_adaptive_hash_indexes)
+                return;
@@ -1337,6 +1344,9 @@
        ulint           n_bytes;
        ibool           left_side;
+        if (!srv_use_adaptive_hash_indexes)
+                return;
        block = buf_block_align(page);
        new_block = buf_block_align(new_page);
        ut_a(page_is_comp(page) == page_is_comp(new_page));

=== modified file 'storage/innobase/row/row0sel.c'
--- storage/innobase/row/row0sel.c      revid:build@mysql.com-20100115170348-qyaxw83mnms3gx8y
+++ storage/innobase/row/row0sel.c      2010-01-28 20:33:52 +0000
@@ -3420,6 +3420,7 @@
        may be long and there may be externally stored fields */
        if (UNIV_UNLIKELY(direction == 0)
+         && srv_use_adaptive_hash_indexes
            && unique_search
            && index->type & DICT_CLUSTERED
            && !prebuilt->templ_contains_blob

It's locked in a couple of other functions in btr0sea.c as well, these are protected when called in btr_cur_search_to_nth_level however:


I question the usage in row_sel as well, but would like input from the InnoDB devs on that.
[28 Jan 2010 21:20] Mark Leith
Forgot to mention, above was against the current 5.1 tree, with the builtin InnoDB.
[21 Jun 2010 12:53] Marko Mäkelä
Now that innodb_use_adaptive_hash_index is settable at runtime in the InnoDB Plugin 1.0 and MySQL 5.5+, it is not safe to read the variable btr_search_enabled without holding one of btr_search_latch or btr_search_enabled_mutex.

I hope that it would be cheap enough to do the following:

enabled = btr_search_enabled;

if (!enabled) {
[21 Jun 2010 13:36] Marko Mäkelä
Another observation: If block->is_hashed == FALSE while you are holding block->lock (and block->buf_fix_count > 0), it is not necessary to protect access to block->frame with btr_search_latch. I am not sure if there are (m)any places where this optimization could be applied.
[28 Jun 2010 14:53] MySQL Verification Team
See bug: http://bugs.mysql.com/bug.php?id=54759.
[28 Jun 2010 16:05] Mikhail Izioumtchenko
reassigning to Jimmy. Now that innodb_adaptive_hash_index is dynamic
it's not safe to look at it without holding a shared (I hope) lock 
on btr_search_latch so some uses may be legitimate.
[28 Jun 2010 23:41] Mikhail Izioumtchenko
I'd say generally it should be safe to dirty read btr_search_enabled
and abandon search or page hash creation if it is 0. Page hash drop, too,
unless called from the disabler routine. 
Maybe it is already so, I haven't reviewed the code carefully.
As for uses of btr_search_latch not immediately related to a.h.i.,
they should be eliminated as this bug suggests, but we should be very careful 
here in testing to avoid increasing pressure on some mutex instead.
[21 Sep 2010 10:24] Jimmy Yang
For PHASE 2 operation in row_search_for_mysql(), the current behavior is that it will obtain btr_search_latch, and continue the row_sel_try_search_shortcut_for_mysql() call:


In btr_cur_search_to_nth_level(), btr_search_enabled would be checked before calling btr_search_guess_on_hash(). And if the AHI is turned off, it will switch to binary search down the tree.

So to avoid holding btr_search_latch unnecessarily if AHI is turned off, we will do a check in row_search_for_mysql(), and release the lock if AHI is off:

                       if (!trx->has_search_latch) {
 +                               if (btr_search_enabled) {
 +                                       rw_lock_s_unlock(&btr_search_latch);
 +                               } else {
                                        trx->has_search_latch = TRUE;
 +                               }
[22 Sep 2010 2:49] Jimmy Yang
Since we can turn on/off AHI dynamically, we need to pay attention to the cases such as bug #54311, where some AHI activity should continue during the process of turning off the AHI, and not simply check btr_search_enabled and return.
[22 Sep 2010 5:36] Jimmy Yang
1) For btr_search_move_or_delete_hash_entries(), it does 


     if (block->is_hashed) {...}


So it is a reasonable check similar with btr_search_enabled while holding btr_search_latch. As mentioned in above case #54311, even if btr_search_enabled is set to 0, we need to continue this function for split until hash index completely dropped. So we cannot simply return if btr_search_enabled is 0 here. 

2) For btr_search_drop_page_hash_index(), it holds btr_search_latch to check block->is_hashed. In this case, we might be able to use a dirty check with 
"btr_search_fully_disabled" to avoid the rwlock acquisition.

+        /* Do a dirty check on btr_search_fully_disabled and
+        block->is_hashed, if it is TRUE, just return. This is 
+        to avoid acquiring shared lock for btr_search_latch for 
+        performance reason. If either condition not satisfied 
+        just continue to acquire latch before check again */
+        if (btr_search_fully_disabled && (!block->is_hashed)) {
+                return;
+        }

        if (UNIV_LIKELY(!block->is_hashed)) {



3) For btr_search_info_get_ref_count(), it is called during index drop, which is a relatively infrequent operation. So a check with btr_search_latch is preferred.
[13 Nov 2010 16:06] Bugs System
Pushed into mysql-trunk 5.6.99-m5 (revid:alexander.nozdrin@oracle.com-20101113155825-czmva9kg4n31anmu) (version source revid:alexander.nozdrin@oracle.com-20101113152450-2zzcm50e7i4j35v7) (merge vers: 5.6.1-m4) (pib:21)
[13 Nov 2010 16:34] Bugs System
Pushed into mysql-next-mr (revid:alexander.nozdrin@oracle.com-20101113160336-atmtmfb3mzm4pz4i) (version source revid:vasil.dimov@oracle.com-20100629074804-359l9m9gniauxr94) (pib:21)
[5 Sep 2012 16:08] Guillaume Giroux
Any chances this could get backported to 5.1 and/or 5.5 ?
[5 Mar 2014 7:38] Bibekananda Pahi
We are using 5.5.28 and getting the issue "holds adaptive hash latch" even if innodb_adaptive_hash_index is OFF. 

Had this issue been fixed? If yes on which version.