Code review comment for lp:~vlad-lesin/percona-server/5.6-logical-readahead

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

Review of commit 576. I think it will be easier if it will have its own MP (probably the other two commits too).

    Code

    - s/ibool/bool/g (in all three commits if applies)
    - I'd add a defensive asserts at os_aio_free that for all arrays,
      count[x] == 0. This would catch any unsubmitted buffered read
      request, and any request buffered on the non-read array.
    - s/ut_malloc+memset(0)/calloc in os_aio_array_create
    - Why does buf_read_recv_pages call
      os_aio_linux_dispatch_read_array_submit? It does not appear to
      submit any buffered requests.
    - fil_extend_space_to_desired_size calling os_aio with
      should_buffer == TRUE is a (benign) typo?
    - The abstraction level for buffered request submitting seems to
      be off. I'd rename os_aio_linux_dispatch_read_array_submit to
      os_aio_submit_buffered_requests and push #ifdef LINUX_NATIVE_AIO
      down to it.
    - buf_read_page_low header comment @return tag: edit to "1 if
      read request is issued or buffered" to clarify that the function
      returns the same for both buffered and immediatelly issued read
      requests.
    - s/read/ready in the buf_read_page_low should_buffer
      comment. (https://github.com/facebook/mysql-5.6/commit/b1b4c7977136d57170f8bf500aaedba740b1c333)
    - Make sure the patch does not break the build with performance
      schema configured out
    - os_aio_linux_dispatch_read_array_submit would need a /*===*/
      comment, os_aio_func should_buffer arg declaration is misaligned
      </pedantic>

    Testcase

    - --disable_warnings/DROP TABLE IF EXISTS/--enable_warnings idiom
      is obsolete and should be removed. (in all three commits if
      applies)
    - innodb_merge_read.test needs --source
      include/have_innodb_16k.inc, as the number of readahead requests
      is likely to differ for other page sizes.
    - innodb_merge_read-master.opt is redundant, as
      --innodb-use-native-aio=1 is the default. The source
      include/have_native_aio.inc check in the testcase itself is
      enough.
    - newline at the end of have_native_aio.inc
    - I'd extend the innodb_merge_read testcase to check that linear
      read ahead read buffering works for compressed tablespaces too
      (there is code if (zip_size) then read(... should_buffer) else
      read(... should_buffer)). That would cause move of the testcase
      to the innodb_zip suite as well.
    - (wishlist) Consider submitting
      https://github.com/webscalesql/webscalesql-5.6/commit/32e49b4d66eaa392d9f06198596db4b16e8b8d04
      for Percona Server so that we exercise AIO with MTR --mem.

review: Needs Fixing

« Back to merge proposal