Code review comment for lp:~sergei.glushchenko/percona-xtrabackup/xb-tools

Laurynas,

>
> Please create and link a blueprint for this work.
>

https://blueprints.launchpad.net/percona-xtrabackup/+spec/log-and-page-printers-for-xtrabackup
Please look at the BP above and tells whether it good or must be rewritten.

> Is it possible to merge the tools InnoDB patch with some existing
> patch? I guess for that some more effort would be required: to
> uncomment but conditionally skip the commented
> innobase_rec_to_mysql(), trx_i_s_cache_init() etc. calls. I don't
> know if this additional effort is justifiable or not.
>

Yep, there is nothing hard in merging utils patch with innodb51.patch.
innobase_rec_to_mysql(), trx_i_s_cache_init() etc. calls should be commented
for both xtrabackup and utils so there is no problem.
Patches been merged.

> Instead of open_or_create_log_file_ex() perhaps better to have
> just open_log_file() that always sets the proposed log file from
> the actual file size? In general I don't like the _ex naming
> convention, because as time passes it is never apparent what is
> meant by it.
>
Yep. I tend to agree with you.

> What is the purpose of rec_get_offsets_old()? Its implementation
> does not have a header comment, and the one next to its
> declaration does not tell why "new" function is not good enough.
>

In case when we deal with redundant page format we don't need to have
dict_index_t* for this page. However rec_get_offsets_new always require
this, so I made separate function rec_get_offsets_old. Another way is
to replace
ut_ad(index);
with something like
ut_ad(!page_is_comp(page_align(rec))||index);

> Why there are separate rules for the utility functions in XB? I
> think we should just link them in the relevant xb configurations
> by default. If they are needed then the Makefile diff at lines
> 591--599 contains $(CFLAGS), $(CXXFLAGS), $(CCFLAGS). I think
> only the first is correct, and then the rules can be merged.
>
> Makefile "utils" target should be marked as phony and IMHO it
> should be included in the default build.
>
> Can the INNODBOBJS definition for utils be reused from some other
> target instead of copy-paste?
>

I've removed target 'utils'. Now utils are build with 'innodb51' target.

> Line 677: please define MLOG_LSN conditionally with #ifndef
> MLOG_LSN
>

done

> The #if 0 blocks in fill_parse_buffer should go away. Likewise in
> ib_log_sys_g et_next_rec().
>

done

> The ib_log_rec_*, ib_log_sys_* structs and functions need
> comments. This was my work, please ping me on IRC and we will
> coordinate.
>

I'll try to write comments by myself first (it's useful as I will get more familiar with code) and
will send you email my version. Then you will correct them and reply back to me. Will this work?