Code review comment for lp:~akopytov/percona-server/ahi-fixes-5.5

Revision history for this message
Laurynas Biveinis (laurynas-biveinis) wrote :

    - 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.
    - 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?
    - 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.
    - 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?
    - Diff line 1273: "These latches"
    - 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).
    - 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.
    - 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.

review: Needs Fixing

« Back to merge proposal