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

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

It does. 
 if (cond && !cond->val_int())
	 continue;

cond->val_int() traverses the condition tree and calculates condition value using fields values stored earlier. 

But we discussed it would be useful to recognize
"START_LSN >some_value_1 and  END_LSN < some_value_2"
expression to prevent scanning the rest of bitmap file(s)
after reaching the upper limit. 

In progress.

> - 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.

Done.

> - 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.
 Done.

> - 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 condition can be any, not only start_lsn >... and end_lsn <... .
And the size of the table is limited with that condition. As I understood
the main purpose of condition pushdown for this table is to limit memory size
for storing temporary table. How can we test that size? Beside that the condition
is checked twice. The first is during table generation and the second is during
query execution. So it may be situation when we get right query execution result
but pushdown doesn't work and the table were completely stored in memory before
output. So how it can be tested?

I'm afraid I don't understand your idea. Do you offer to make several queries with the
following "where" expressions?
1) start_lsn > 0 and end_lsn < ($max_lsn/2)
2) start_lsn > 0 and end_lsn < $max_lsn
3) ($max_lsn/2) >= 0 and end_lsn < ($max_lsn)

If yes, what expected results should be?

> - 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?

I think my mistake is in wrong naming of the variable type. It isn't a system
variable, it's plugin variable. Jenkins build didn't show any problems with variable tests. 
And I saw another innodb_* variables without tests in sys_var test suite. 

> - 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:
Done.

> - 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.
Done.

> - (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)
Done.

> - Maybe log_online_bitmap_iterator_init should set the rest of the
>   fields it does not touch now to some safe values?
Done.
 
> - 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.
Done.
 
> - 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.

Still under discussion.

> - 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.
Done.

> - 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.
Done.

> - Line 440: s/space/page
Done.