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

> Vlad -
>
> You are right, I misunderstood the purpose of max start and max end lsn
> struct, I thought that it is meant to limit the lower bound of the read
> interval too, although this information cannot be usefully applied yet. In
> this case I am not sure we need to have the max start lsn at all and we can
> have a single end_lsn limit variable for simpler checks, where start_lsn is
> handled as follows: suppose the condition is start_lsn < 100, this means we
> have to read all blocks with start_lsn < 100. Which is equivalent to reading
> all the blocks with end_lsn <= 99, or just end_lsn < 100.
> And we know that
> have read all such blocks when we read a block where start_lsn >= 100.
> Condition LOG_BITMAP_ITERATOR_START_LSN(i) > lsn_range.end would handle this.
> Does this look correct to you?

Yes, that's a good idea, thanks. I changed limit_lsn_range_from_condition() so it finds maximum lsn value at all, doesn't matter if it's start or end lsn.

> A check (whatever its final form is) in lines 627--629 should simply become a
> part of the while condition.
Done.

> Should the "regular" ICP condition check (lines 659--660) be moved before
> table->field[i]->store calls?
No, it shouldn't. Because "cond->val_int()" use stored field values to calculate condition value. For example if condition is "page_id = 100" val_int() will use the table->field[1] value to calculate if condition for current iteration is true or false.

> A few too long lines in the header comment of
> limit_lsn_range_from_condition(): lines 476, 477, 484 and 491.
>
> Misaligned comment starts in lines 737--747.
Done.

> The log0online.h function declarations do not follow InnoDB style for their
> arguments. The log0online.c definitions follow them and have expanded
> comments, so probably should be re-copy-pasted to the header file.
Done.

> The header
> comments should be "Do something" instead of "Does something."
If so there will not be uniformity because the other functions in "log0online.h" has "Does something" comments.

> The
> log_bitmap_iterator_t *i arg in all functions should be commented as "in/out"
> instead of "out" because it is required that i is allocated beforehand.
Done.