Merge lp://qastaging/~sergei.glushchenko/percona-xtrabackup/xb-tools into lp://qastaging/percona-xtrabackup/2.1

Sergei,

I didn't review the actual code, just want to comment on a number of basic things that caught my eye:

   - large blocks of empty lines, i.e. after ib_page_page_print_list()
     declaration (and what does that "SOME IBUF STUFF" comment mean?),
     and after ib_page_ibuf_rec_get_size()
   - lots of lines breaking the 80 chars limit
   - I see absolutely no reasons to use C++ and Boost in percona-redo.cc
     and percona-pprint.cc. Option parsing is available in my_getopt.c,
     which is already used in xtrabackup.c and xbstream.c. And
     pprint/reado are very small utilities.
   - large block of "#if 0"ed code in percona-redo.cc. If it's a
     debug-only code, create appropriately named #defines and some way
     to compile a debug binary. If that code is not supposed to be used
     by anyone, remove it.
   - some commented out code in percona-pprint.c. Same comments as in
     the previous item.
   - what are those "BEGIN LICENSE" / "END LICENSE" for?
   - can innodb_init_param() be moved to innodb_int.[ch] so we don't
     create separate files for it, which are also confusingly similar to
     innodb_int*?

In addition to the above, I think api0internal.h has to be merged with
innodb_int.h, since the latter has the same purpose.

And api0log.c and api0print.c should have better name, I don't see a
point in following the InnoDB file naming conventions. Especially
because no one except Heikki knows the reason to name files like
xxx0yyy.c :)

Alexey -

I guess we want to minimize differences between the HailDB and XB versions of the utilities, and IMHO api0internal.h makes more sense in the former where there is no innodb_int.h. I agree we could name files better for XB, but for the HailDB modifications it makes sense to stick with foo0bar naming convention. But this is not a very strong opinion.

Laurynas,

On 07/05/2012 09:26 AM, Laurynas Biveinis wrote:
> I guess we want to minimize differences between the HailDB and XB versions of the utilities, and IMHO api0internal.h makes more sense in the former where there is no innodb_int.h. I agree we could name files better for XB, but for the HailDB modifications it makes sense to stick with foo0bar naming convention. But this is not a very strong opinion.
>

I remember we were discussing this, but I'm not sure about our current
plans about the HailDB version of the utilities. Do we have anything
specific set?

Following been done:

Formatted to meet 80 chars limitation.
C++ parts rewritten in C.
Some code cleanup, removed commented and #if 0'ed parts.
innodb_init_param moved to innodb_int.c
api0* files were renamed to api_*
api0internal.h was merged with innodb_int.h

I consider that it is ready for the next iteration.

This is a partial review only, I will continue later.

    Please create and link a blueprint for this work.

    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.

    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.

    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.

    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?

    Line 677: please define MLOG_LSN conditionally with #ifndef
    MLOG_LSN

    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.

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

    The untested log record types still need testing. That's a non
    trivial task, need coordination with QA.

Alexey Kopytov <email address hidden> writes:
> I remember we were discussing this, but I'm not sure about our current
> plans about the HailDB version of the utilities. Do we have anything
> specific set?

No, nothing solid. Although api0internal.h is more innodb-like naming.

--
Stewart Smith

Couple more comments:

The page printer utility starts & commits MTRs, for example i ib_page_load_table_for_id(). How does that work since we are not allowed to write to the log in the utility?

How do you plan automating testing this, both as standalone utility and GDB entry points in the XB binary?

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?

> Couple more comments:
>
> The page printer utility starts & commits MTRs, for example i
> ib_page_load_table_for_id(). How does that work since we are not allowed to
> write to the log in the utility?
>

As long as page printer doesn't make changes to buffer pool pages, mtr_commit doesn't write to log.

> How do you plan automating testing this, both as standalone utility and GDB
> entry points in the XB binary?

We should think about it:)

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

Thanks, I commented on its whiteboard.

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

Yes.

> > Couple more comments:
> >
> > The page printer utility starts & commits MTRs, for example i
> > ib_page_load_table_for_id(). How does that work since we are not allowed to
> > write to the log in the utility?
> >
>
> As long as page printer doesn't make changes to buffer pool pages, mtr_commit
> doesn't write to log.

No way to avoid creating those MTRs and passing NULL MTRs around? Or asserting that MTR did not write anything?

> > How do you plan automating testing this, both as standalone utility and GDB
> > entry points in the XB binary?
>
> We should think about it:)

I see two ways, with distinct advantages and disadvantages:
1) Provide entry to those functions through some new XB command-line options. Then test those options.
2) Script gdb to call these functions.

The latter is closer to the actual use case but the former might be enough perhaps?

1) Building:
AUTO_DOWNLOAD=yes utils/build.sh innodb51
...
Building XtraBackup
Makefile:190: target `innodb_int.o' given more than once in the same rule.
...
api_page.c: In function ‘ib_page_rec_print_old’:
api_page.c:2715:24: warning: variable ‘ifield’ set but not used [-Wunused-but-set-variable]
api_page.c: In function ‘ib_page_page_print_list’:
api_page.c:2880:9: warning: variable ‘n_recs’ set but not used [-Wunused-but-set-variable]
api_log.c: In function ‘ib_log_sys_parse_rec_body’:
api_log.c:816:8: warning: variable ‘ret’ set but not used [-Wunused-but-set-variable]
innodb_int.c: In function ‘innodb_init_param_for_util’:
innodb_int.c:65:16: warning: assignment discards ‘const’ qualifier from pointer target type [enabled by default]
innodb_int.c:82:2: warning: passing argument 1 of ‘srv_parse_log_group_home_dirs’ discards ‘const’ qualifier from pointer target type [enabled by default]
/home/vlesin/src/work/xb-tools-sergey_gl/mysql-5.1/storage/innodb_plugin/include/srv0start.h:54:1: note: expected ‘char *’ but argument is of type ‘const char *’
api_log.c: In function ‘ib_log_sys_print’:
api_log.c:1449:1: warning: control reaches end of non-void function [-Wreturn-type]
...
cc: error: percona-pprint.o: No such file or directory

2) Code style:
a) 16, 35, 93 - innodb code style requires comment which underlines function name the same length as underlined function name;
b) 100, 251 - unaligned comment;
c) 150, 151, 243- function description is absent;
d) 14-15, 33-34, 60-61, 67-68, 2427, 2444, 2468 and other functions in api_page.c containing page pointer as an argument, - function argument description is absent;
e) I don't think _old and _new function names postfixes is a good idea. What if there is one more new format? As I see there are two variants of code for calculating record offsets. One is for "compact" and another is for "external" formats. We could just use _cmp_fmt and _ext_fmt postfixes for that functions instead of _old and _new.
f) I think api_page.c should correspond to innodb code style as api_log.c does to have some uniformity in api_* files. If so there should be explaining comments for function declarations and definitions. And the same remark as 2a.

3) Possible errors:
a) 330-338 - if "create_new_db" is false then "ret" is not initialized but it is compared with "FALSE" in 338. This comparison is unneeded because "ret" is initialized only if "create_new_db" is true. Otherwise the first argument of "or" operator is true and there is not need to check the second one.
339-340 - the "if" block will never work because if we are at this line the "create_new_db" is false. I think this block should be under "if (create_new_db)" condition.
It would be great if you comment your changes in open_or_create_log_file() function.
b) 2567-2569 it would be good if we checked "create_new_db" for false.

Vlad,

> a) 330-338 - if "create_new_db" is false then "ret" is not initialized but it is compared with "FALSE" in 338. This comparison is unneeded because "ret" is initialized only if "create_new_db" is true. Otherwise the first argument of "or" operator is true and there is not need to check the second one.

Just a little comment. I put the code below to not move eyes up and down while writing this.
 if (create_new_db) {

  files[i]
   = os_file_create(name, OS_FILE_CREATE, OS_FILE_NORMAL,
      OS_LOG_FILE, &ret);
 }
 if (!create_new_db || ret == FALSE) {
  if (create_new_db
      && os_file_get_last_error(FALSE) != OS_FILE_ALREADY_EXISTS
...
      ) {
...
   return(DB_ERROR);
  }

  files[i] = os_file_create(name, OS_FILE_OPEN, OS_FILE_AIO,
       OS_LOG_FILE, &ret);

I can't see any mistake in this lines.
Indeed, consider create_new_db is TRUE and we are trying to create new file. We have two options here. Creation succeeded and ret is set to TRUE, or creation failed and ret is set to FALSE.
Next we check whether we created file or not by using this condition (!create_new_db || ret == FALSE). Obviously if create_new_db is TRUE, the result depends only on the ret value. And we should report error (last error was not OS_FILE_ALREADY_EXISTS) or try to open existing file.

Now lets consider create_new_db is FALSE. In this case we go straight to opening file with OS_FILE_OPEN. Because all conditions in this case not depend on ret value, which is not set as you noticed before.

Vlad,
> e) I don't think _old and _new function names postfixes is a good idea. What if there is one more new format? As I see there are two variants of code for calculating record offsets. One is for "compact" and another is for "external" formats. We could just use _cmp_fmt and _ext_fmt postfixes for that functions instead of _old and _new.

Yes. There are two major physical row formats in InnoDB http://dev.mysql.com/doc/refman/5.0/en/innodb-physical-record.html. Redundant page keeps much more information inside the record itself, which allow us to print fields even if we have no information about the index which occupied this page. While compact page needs less space to store records. I don't like _old and _new suffixes as much as you do. But it is common practice to use these suffixes. Just look at rem0rec.h and you will see it. So I think we should use this convention too.

I did some testing of entry points in xtrabackup and here are my findings:

In 'backup' mode:
Page printer fails. In order for it to work following initialization should be done first:
 buf_pool_init();
 dict_ind_init();
 dict_boot(FALSE);
 dict_check_tablespaces_and_store_max_id(FALSE);
and IBUF should initialized in order to print IBUF pages
Log printer also fails.
 recv_sys_create()
is needed.

In 'prepare' mode:
For page printer to work:
 dict_ind_init();
 dict_boot(FALSE);
 dict_check_tablespaces_and_store_max_id(FALSE);
Log printer work just fine

In 'stats' mode:
Both page and log printers work OK.

I don't think we should initialize all these systems every time when running xtrabackup. However it would be nice to make log and page printers more useable inside xtrabackup. Maybe we should make some initialization function for these tools which used could call from gdb before using log or page printer.

I also found that by making create_new_db flag meaningful breaks second '--prepare' run (in open_or_create_log_file). XtraBackup doesn't create new log files because of when datafiles been found, create_new_db=FALSE is passed. So I reverted a change with create_new_db.

I think all required subsystems must be initialized on startup then, unless something precludes this.

Couple more bugs been found in initialization of log printer.
1. When compiled with debug, several failures occurred on ut_a(&(log_sys->mutex))
2. mem_init() call needed for debug builds
3. ib_log_sys_start opens only one log file from group.

Also it would be nice to have commandline argument --log-file-size

Comments for ib_page_* and ib_log_* fucntions, more InnoDB code conventions fixes.
Two testcases added for standalone tools.
Fixed number of initialization and locking errors.

Hey, guys!
Many of your comments were taken into account, many other things were spotted and fixed. Could you please review this once more.