Code review comment for lp:~sergei.glushchenko/percona-xtrabackup/xb20-bug932623

Sergei,

Thanks for clarifications. My comments on the new version of the fix:

   - I wonder if it would be less invasive to define our own simple
     wrappers around "HASH_SEARCH(hash, fil_system->spaces, id, space,
     space->id == id);" so we don't have to patch InnoDB or provide our
     own implementations for built-in InnoDB, and mess with
     mutexes. I.e. can we have xb_space_get_by_*()?

  - the code in xtrabackup's version of io_handler_thread() doesn't use
    the 'i' variable, why not just copy the code from InnoDB, i.e.:

 while (srv_shutdown_state != SRV_SHUTDOWN_EXIT_THREADS) {
  fil_aio_wait(segment);
 }

  - fil_free_all_spaces() seems to be defined for built-in InnoDB, but
    is not used anywhere?

  - in fact, ut_free_all_mem() will not free anything when the system
    malloc() is used instead of the InnoDB own malloc manager, i.e. when
    srv_use_sys_malloc == TRUE, which is the default. I have some
    thoughts on how to fix that, but I suggest we'll do it later as a
    part of bigger refactoring.

  - POSIX guarantees the result of snprintf() to be always
    zero-terminated, so there's no need for things like:

+ snprintf(dest_dir, FN_REFLEN, "%s/%s",
+ xtrabackup_target_dir, dbname);
+ dest_dir[FN_REFLEN - 1] = 0;

    if you care about Windows, you should be using either my_snprintf()
    or ut_snprintf(), which do the right thing on Windows. But
    currently, xtrabackup.c just uses snprintf() in other places anyway,
    so what's the point to do some windows-specific stuff in this
    patch. We can do s/snprintf/ut_snprintf/ later, if and when we
    have to support Windows.

  - this code just uses sprintf() instead snprintf() for some reasons:

+ sprintf(tmpname, "./%s/tmp#%lu", dbname, fil_space->id);

  - I think the output in xb_delta_open_matching_space() is too
    verbose. Let's only print messages when something unexpected
    happens, i.e. remove these messages:

+ msg("xtrabackup: Looking for tablespace %s with id %lu by name\n",
+ dest_space_name, space_id);

and

+ msg("xtrabackup: Looking for tablespace %s with id %lu by id\n",
+ dest_space_name, space_id);

and

+ msg("xtrabackup: Found matching tablespace %s\n",
+ fil_space->name);

and

+ msg("xtrabackup: Created tablespace %s with id %lu.\n",
+ dest_space_name, space_id);

  - what's the point to rename a tablespace with the same name but a
    different ID to tmp#id? the only case when it happens is when a
    tablespace was recreated between the full backup and an incremental
    one. In which case, if we rename the dropped table, we will end up
    with ghost 'tmp...' table in the full backup.

  - this if() block can be merged:

+ if (xtrabackup_incremental) {
+ err = xb_data_files_init();
+ if (err != DB_SUCCESS) {
+ msg("xtrabackup: error: xb_data_files_init() failed with"
+ "error code %lu\n", err);
+ goto error;
+ }
+
+ if(!xtrabackup_apply_deltas(TRUE)) {
+ xb_data_files_close();
+ goto error;
+ }
+ }
+
+ if (xtrabackup_incremental) {
+ xb_data_files_close();
+ }

  - can we also a add a cross-db rename test to bug932623.sh?