Code review comment for lp:~vlad-lesin/percona-server/i_s-innodb-changed-pages

Some feature design and code questions. Maybe not all of them need to
be resolved right now.

- We need to know somehow if the current run is complete, i.e. if we
  query some LSN interval, the last block we read should have the last
  block bit set, if we silently output partial data, the user will not
  know if he should re-query the last part of LSN interval or not. I
  am not sure what would be the best way. Another column for last
  block bit flag? That would be quite clumsy for users to use.

- Likewise I am not sure what should we do about corrupted blocks in
  the middle of requested LSN intervals (and maybe missing LSN
  interval data too). Another boolean column "this block is continuous
  from the previous block"?

- In any case I guess we should diagnose bad block checksums we
  encounter to the error log at least. That's easy, but it raises the
  issue of what would happen if the I_S query would attempt to read the
  last bitmap block while it's being written. Currently such block
  would be ignored correctly, but in this case we would get false
  errors. I think we should introduce a log online mutex for the last
  written block protection. Acquire it in the log writer around the
  block write + sync. Acquire it (and immediatelly release) in the
  log iterator right before the read when (offset == file size - 1
  block).

General comments:

- Please submit an MP against lp:percona-server/5.1, as the current
  target branch has been merged.

- I guess the current code did not implement the index condition
  pushdown yet?

- There is no decision on the I_S table row count limit system
  variable in the blueprint, but the code has it, please update the
  blueprint.

- Please add tests for LSNs too. Unfortunately, their values are not
  that predictable, so the basic test would be to check that start lsn
  < end lsn.

- Please add tests for the index condition pushdown, i.e. where
  start_lsn > ... and end_lsn < .... The lsn values for this test can
  be taken from the 1st query on I_S, and get the e.g. MIN, MAX, and
  median values of the tracked interval.

- The new system var requires a new sys_var/var_name-basic.test. In
  the current branch, isn't sys_vars/all_vars test failing due to that
  test being missing?

- Code formatting issues (I know, these are very pedantic): the
  /*=====*/ under function names should match the function name
  length. The function arg comments should start with /*!< in: or /*!<
  out: or /*!< in/out:

- Indentation issue around diff lines 308--312: both args and code
  should be indented by 1 tab. Likewise in many other places in
  log0online.c etc.. Also there should be 1 tab between var type and
  name in the local var declarations.

- (srv_changed_pages_limit ? output_rows_num < srv_changed_pages_limit
  : TRUE) can be simplified to (!srv_changed_pages_limit ||
  output_rows_num < srv_changed_pages_limit)

- Maybe log_online_bitmap_iterator_init should set the rest of the
  fields it does not touch now to some safe values?

- No need for the comment in diff lines 618--620, 646--648, it should
  be enough to mention it once in log0online.c next to the first file
  name printing place.

- In log_online_bitmap_iterator_next in the case of bitmap file
  checksum error it makes sense to report it as a warning to the error
  log.

- The naming convention for structs in InnoDB is that
  st_log_bitmap_iterator should be called log_bitmap_iterator_struct
  and st_log_bitmap_iterator should be log_bitmap_iterator_t.

- The comments for the struct st_log_bitmap_iterator should not
  mention "bitmap output", but it's input there. Likewise with "out"
  in the field names, i.e. out_offset should be read_offset or similar.

- Line 440: s/space/page

- What is the purpose of LOG_BITMAP_ITERATOR_* macros? Except for
  LOG_BITMAP_ITERATOR_PAGE_NUM