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

Alexey,

> - 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_*()?

I've wrote xb_space_by_* wrappers. The only modification of innodb51_builtin.patch left is mem_close function.

>
> - 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);
> }
>

fixed

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

Yes. I forgot to put call in proper place, fixed.

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

OK. Hopefully memory leaks will not be critical.

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

Great! I removed implicit zero-terminating statement.

> - this code just uses sprintf() instead snprintf() for some reasons:
>
> + sprintf(tmpname, "./%s/tmp#%lu", dbname,
> fil_space->id);
>

Hmm... I expected that generated tmpname cannot be longer than FN_REFLEN. I've changed to snprintf for more safety.

> - 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);
>

Removed.

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

As have been discussed on IRC, we have cases when matching name and not matching ID doesn't mean DROP TABLE, but just RENAME TABLE. So we decided to keep this fix.
I've checked that actual filename is xtrabackup_tmp#ID.ibd now.

> - 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();
> + }
>

fixed.

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

added.