Hi Laurynas,

As discussed on IRC, I’m only posting comments specific to the port
itself. I’ll handle everything else in the followup work on AHI.

On Wed, Aug 28 2013 13:59:04 +0400, Laurynas Biveinis wrote:

[...]

> -	btr_search_latch_temp = (rw_lock_t*) mem_alloc(sizeof(rw_lock_t));
> +	/* btr_search_index_num should be <= 32. (bits of
> +	trx->has_search_latch) */
> +	ut_ad(btr_search_index_num <= 32);

This assertion is wrong. We allow 64 AHI partitions in 64-bit
architectures in 5.5, and its documented. It’s just that the code
comment is wrong.

>  
> -	rw_lock_create(btr_search_latch_key, &btr_search_latch,
> -		       SYNC_SEARCH_SYS);
> +	btr_search_latch_temp_arr = (rw_lock_t**)
> +		mem_alloc(sizeof(rw_lock_t *) * btr_search_index_num);

What’s so temporal about btr_search_latch_temp_arr[]? It might make
sense in the single-lock code (though the reasoning for it is dubious),
but it correctly does not have “temp” in 5.5 code, as it’s a regular array.


[...]

>  	index = block->index;
> -
> -	if (UNIV_LIKELY(!index)) {
> -
> -		rw_lock_s_unlock(&btr_search_latch);
> -
> +	if (UNIV_UNLIKELY(!index)) {
> +		rw_lock_s_unlock(btr_search_latch);
>  		return;
>  	}
>  
> +	ut_a(btr_search_latch == btr_search_get_latch(index->id));
> +

Shouldn’t this be a debug assertion? btr_search_latch is basically a
constant attribute of each index. So it’s more of a code invariant.


[...]

> -btr_search_validate(void)
> -/*=====================*/
> +btr_search_validate_one_table(
> +/*==========================*/
> +	ulint	t)
>  {
>  	ha_node_t*	node;
>  	ulint		n_page_dumps	= 0;
> @@ -1874,24 +1944,20 @@
>  
>  	rec_offs_init(offsets_);
>  
> -	rw_lock_x_lock(&btr_search_latch);
> -	buf_pool_mutex_enter_all();
> -
> -	cell_count = hash_get_n_cells(btr_search_sys->hash_index);
> +	cell_count = hash_get_n_cells(btr_search_sys->hash_index[t]);
>  

Why buf_pool_mutex_enter_all() / buf_pool_mutex_exit_all() calls are
being removed from btr_search_validate[_one_table]()?


[...]

> +ha_assert_btr_x_locked(
> +/*===================*/
> +	const hash_table_t* table)	/*!<in: hash table to check */
> +{
> +	ulint i;
> +
> +	ut_ad(table->adaptive);
> +
> +	for (i = 0; i < btr_search_index_num; i++)
> +		if (btr_search_sys->hash_index[i] == table)
> +			break;

Braces.

[...]


> +/********************************************************************//**
> +Latches all adaptive hash index latches in shared mode.  */
> +UNIV_INLINE
> +void
> +btr_search_s_lock_all(void);
> +/*========================*/
> +
> +/********************************************************************//**
> +Unlatches all adaptive hash index latches in shared mode.  */
> +UNIV_INLINE
> +void
> +btr_search_s_unlock_all(void);
> +/*==========================*/
> +

These ones are declared and defined, but not used.


[...]


> === modified file 'Percona-Server/storage/innobase/include/mtr0log.ic'
> --- Percona-Server/storage/innobase/include/mtr0log.ic	2013-08-06 15:16:34 +0000
> +++ Percona-Server/storage/innobase/include/mtr0log.ic	2013-08-28 09:57:53 +0000
> @@ -28,6 +28,7 @@
>  #include "buf0buf.h"
>  #include "buf0dblwr.h"
>  #include "fsp0types.h"
> +#include "btr0types.h"
>  #include "trx0sys.h"

Why new #includes in mtr0log.ic, que0que.h and read0read.h, trx0roll.h,
trx0trx.cc, ut0ut.cc and os0file.cc?


[...]


>  trx_search_latch_release_if_reserved(
> @@ -172,7 +172,13 @@
>  	trx_t*	   trx) /*!< in: transaction */
>  {
>  	if (trx->has_search_latch) {

This “if” is not really needed. Though the entire function will go away
along with trx_t::has_search_latch in the followup work.

> -		rw_lock_s_unlock(&btr_search_latch);
> +		ulint i;
> +
> +		for (i = 0; i < btr_search_index_num; i++) {
> +			if (trx->has_search_latch & (1UL << i)) {
> +				rw_lock_s_unlock(btr_search_latch_temp_arr[i]);
> +			}
> +		}
>  
>  		trx->has_search_latch = FALSE;

Should assign 0 instead of FALSE technically, but again, this code will
be removed later anyway.


[...]


> +trx_search_latch_lock(
> +/*==================*/
> +	trx_t*	trx,		/*!< in: latching transcation */
> +	ulint	index_id)	/*!< in: index to latch AHI for */
> +{
> +	ulint	latch_mask = 1UL << (index_id % btr_search_index_num);
> +
> +	if (!(trx->has_search_latch & latch_mask)) {
> +
> +		if (trx->has_search_latch < latch_mask) {
> +
> +			rw_lock_s_lock(btr_search_get_latch(index_id));
> +			trx->has_search_latch |= latch_mask;
> +		} else {
> +
> +			ibool	taken_latches = trx->has_search_latch;

ibool? but again, no practical importance and will be removed later anyway.