MySQL Bugs: #50461: many uses of btr_search_latch with innodb_adaptive_hash

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:	None
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

Description:
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.

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;
+
        ut_ad(info);
 
 #ifdef UNIV_SYNC_DEBUG
@@ -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;
+
 retry:
        rw_lock_s_lock(&btr_search_latch);
 
@@ -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:

btr_search_guess_on_hash
btr_search_build_page_hash_index

I question the usage in row_sel as well, but would like input from the InnoDB devs on that.

Forgot to mention, above was against the current 5.1 tree, with the builtin InnoDB.

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:

mutex_enter(&btr_search_enabled_mutex);
enabled = btr_search_enabled;
mutex_exit(&btr_search_enabled_mutex);

if (!enabled) {
	return;
}

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.

See bug: http://bugs.mysql.com/bug.php?id=54759.

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.

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.

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:

btr_cur_search_to_nth_level
btr_pcur_open_with_no_init_func
row_sel_try_search_shortcut_for_mysql
row_search_for_mysql

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) {
                                rw_lock_s_lock(&btr_search_latch);
 +                               if (btr_search_enabled) {
 +                                       rw_lock_s_unlock(&btr_search_latch);
 +                               } else {
                                        trx->has_search_latch = TRUE;
 +                               }
                        }

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.

1) For btr_search_move_or_delete_hash_entries(), it does 

btr_search_move_or_delete_hash_entries()
{
     rw_lock_s_lock(&btr_search_latch);

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

     rw_lock_s_unlock(&btr_search_latch);
}

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.

btr_search_drop_page_hash_index() 
{
+        /* 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;
+        }

       rw_lock_s_lock(&btr_search_latch);
   
        if (UNIV_LIKELY(!block->is_hashed)) {

                rw_lock_s_unlock(&btr_search_latch);

                return;
        }
}

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.

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)

Pushed into mysql-next-mr (revid:alexander.nozdrin@oracle.com-20101113160336-atmtmfb3mzm4pz4i) (version source revid:vasil.dimov@oracle.com-20100629074804-359l9m9gniauxr94) (pib:21)

Any chances this could get backported to 5.1 and/or 5.5 ?

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.