> Diff line 281 query could be written as SELECT MAX(end_lsn) AS > end_lsn FROM I_S....; Done. > The same comments re. server restart from > https://code.launchpad.net/~vlad-lesin/percona-server/i_s-innodb-log- > tracking-status/+merge/117871 > apply here too. Done. > srv_changed_pages_limit is declared in srv0srv.h, there is no > need for separate extern declaration in i_s.cc. Done. > I'd name the max_lsn_struct to lsn_range_struct because that's > what it is - or would get rid of it altogether, passing *min_lsn, > *max_lsn to the get_max_lsn_from_and_condition does not seem like > a big deal. Likewise I'd rename get_max_lsn_from_and_condition() > to limit_lsn_range_from_condition() Done. > The range start and end values are not updated correctly: they > are both compared with their old values and updated if they are > less. So if we have if start_lsn > 100 && start_lsn > 50, 50 will > be chosen instead of 100. The way to do this is to start with > start_lsn of zero and compare if it's greater than the old > value, and end_lsn of +infinity, compared if it's smaller than the old > value. I think the current logic is right for "AND" operation and "less(or equal)" comparisons. The original task was to avoid full bitmap files scanning if we reach some limit value from condition. That's why only upper limits from "less(or equal)" comparisons are found. It finds the minimum value because if one of the "AND" operands is "false" the result of the whole operation is "false" too. For example for our expression "start_lsn < 100 && start_lsn < 50" there is no need to scan bitmaps further if start_lsn reaches 50 because the result of "AND" operation will be "false". For "start_lsn > 100 && start_lsn > 50 && start_lsn < 150 && 200 >= start_lsn" only last two comparisons will be parsed and "150" value will be a result because there is no sense to parse "greater(or equal)" comparisons as we can't calculate an offset of start bitmap block in a file. So any operand of "AND" is ignored if it is not a "(start|end)_lsn (<|<=) some_numeric_value" or "some_numeric_value (>=|>) (start|end)_lsn". And any conditions except "AND" and comparison itself are currently ignored. > I am not sure if the condition range limiting is applied > correctly in the main fill loop: if > (LOG_BITMAP_ITERATOR_START_LSN(i) > max_lsn.start || ...) break; > In case we have requested an interval that has a checkpoint in > the middle, this check will only output the rows up to that > checkpoint and then ...START_LSN(i) > max_lsn.start will become > true. I think currently (before we have multiple bitmap files) > the only limitation we can do here is > LOG_BTIMAP_ITERATOR_START_LSN > max_lsn.end I'm not sure I understood you right. The function just stop scanning bitmap files if condition becomes "false" for certain condition pattern. The condition for "AND" operation becomes "false" if at least one operand is "false". The condition: if (LOG_BITMAP_ITERATOR_START_LSN(i) > lsn_range.start || LOG_BITMAP_ITERATOR_END_LSN(i) > lsn_range.end) break; means that at least one operand in "AND" operation is "false" and there is no need to continue reading data. > It is also possible to check if the condition range limitation > resulted in empty range of start_lsn > end_lsn and not to > anything then. I think it's too complex for this task. At my point of view the most used query will be something like that: SELECT * FROM I_S.INNODB_CHAGED_PAGES WHERE start_lsn > numeric_value_1 AND start_lsn < numeric_value_2; If I'm not mistaken that's why we decided to parse only "AND" and comparison operations where start_lsn or end_lsn are limited from above. I don't think somebody will want to make query where start_lsn > end_lsn. And I don't think it's a reason to complicate the code for quite rare use case. How do you think? > Line 481: "expression". > Line 486: not sure what " if fill max_lsn with the minimum > values." means. I think the current description is not so clear as I would like. That's why I rewrote it. > Line 543: s/cause/case Done.