Code review comment for lp:~laurynas-biveinis/percona-server/ahi-partitions-5.6-5.6

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.