Hi Laurynas,

On Wed, 04 Sep 2013 14:50:31 -0000, Laurynas Biveinis wrote:
> Review: Needs Fixing
>
>      - In percona_bug1218330.test, consider replacing MTR variables
>        with SQL variables.  Then the need to hide the exact values by
>        disable_query_log is removed from the adaptive_hash_mem_2 point
>        onwards, resulting in an easier-to-understand test result file.
>      - btr_search_info_get_ref_count header comment in btr0sea.c has
>        "the ."  Also, this header comment should be synced with
>        btr0sea.h after editing.

I considered SQL variables when implementing the test, but preferred MTR 
variables, because the .test file is easier to read this way. And the 
.result file is for MTR to read.

>      - It seems that now every index structure in memory create must
>        be accompanied by a btr_search_index_init() call.  This is OK,
>        but I'm wondering if there is any way to merge-proof this
>        further.  For example, poison index->search_latch and
>        search_table pointers in dict_mem_fill_index_struct() if
>        UNIV_DEBUG?

Yes, btr_search_index_init() must be called. The main reason is that 
dict_mem_index_create() does not set the index id, it is supposed to be 
assigned by the caller and btr_search_index_init() needs an ID.

Now the callers of dict_mem_index_create() do assign an ID in most, but 
not all cases. For instance, when a "dummy" index is created, it doesn't 
get an ID.

One way to fix that would be to modify dict_mem_index_create() to accept 
an ID argument, and make dummy index pass some bogus value (0) as an ID. 
But that's almost as fragile as the current approach. Consider a 
potential index copy routine that copies an existing index (i.e. 
bypassing dict_mem_index_create()) and assigning an ID separately.

I don't see any value in poisoning index->search_latch, as index 
structure is created with mem_heap_zalloc(), so if an uninitialized 
value is used, we'll know.

>      - The ha_innodb.cc conflict needs either you fixing it, if
>        possible on GCA, otherwise me fixing it on merge.  Note that
>        the snippet which was supposed to go to
>        innodb_release_stat_resources() went to
>        innodb_srv_conc_force_exit_innodb() instead.

Yes, saw the conflict. The reason is that 5.5 GCA tree is behind the 
refactoring which made it into the 5.6 tree.

Basically, do what 5.6 code does when merging. It's rather trivial to 
resolve.

>      - Why do innobase_release_stat_resources(),
>        innobase_commit_ordered(), and innobase_commit() appear to have
>        new calls to trx_search_latch_release_if_reserved() added
>        anyway, if they are? Or is it a bzr mismerge and only comments
>        next to the calls have been added?

It's just that the GCA branch has those calls, but they were removed 
later. Null-merge those conflicts.

>      - Diff line 1273: "These latches"

Oh noes :) But since that is just a "typo in a comment" kind of issue, 
and the recommit/merge procedure is quite complicated in this specific 
case, I'd keep it as is.

>      - It appears that sync_thread_levels_nonempty_trx() is dead code
>        now, and all its calls may be replaced by
>        sync_thread_levels_nonempty_gen(FALSE).

It's not "dead code", it's more a debug validation check that we _think_ 
is now unnecessary because on the current code paths we believe 
trx->has_search_latch is always FALSE. We may be wrong. Or code paths 
may change in the future. The more debug checks, the better.

>      - trx->search_latch_timeout now becomes a never-changing constant
>        BTR_SEA_TIMEOUT.  It can be removed and
>        I_S.INNODB_TRX.TRX_ADAPTIVE_HASH_TIMEOUT field can return
>        BTR_SEA_TIMEOUT directly, always.

Yep, the original iteration of the patch removed both 
trx->has_search_latch and trx->search_latch_timeout. I eventually 
decided to keep them. Less upstream code to touch, no user-visible 
differences.

>      - Not fully sure but it appears that there are some
>        locking/unlocking assymetries wrt UNIV_SEARCH_DEBUG, preceding
>        this MP.  Not if actionable, probably no need to waste time on
>        this.  One suspicious example is the target code of
>        release_search_latch goto label.
>

No, both before- and after-MP code are OK. The before-MP code might be 
calling rw_lock_s_unlock() just after the switch as the after-MP code 
does. But it checks trx->has_search_latch, so it can do it sometime later.

The after-MP code is guaranteed to not have the latch before the 
enclosing "if" statement, so it is safe to remove one condition and 
release the latch earlier unconditionally.