Code review comment for lp:~vlad-lesin/percona-xtrabackup/2.1-apply-archived-logs-innodb5.6

On 05/01/2013 02:40 PM, Alexey Kopytov wrote:
> - I don't think the XtraBackup part should pull the entire server
> implementation of log archiving. It looks like only a minor part
> of it should be applied to InnoDB 5.6 code base to be able to
> parse and apply archived logs? As in, do we really need
> fil_space_contains_node(), log_sys->archive_buf initialization,
> changes to log_checkpoint_set_nth_group_info(), etc.?

Done.

> - can you clarify the following change? I.e. under what
> circumstances exactly can a tablespace exist when we are trying
> to replay MLOG_FILE_CREATE from the archived logs?

Yes. It can be for example in the case when this file did not exist at
the beginning of backup process and was created somewhere in the middle
of backup process. So log file contains "create" command as backup
process started tracking it before the file creation, but data directory
contains this file as it was copied from original directory. I copied
this explanation to code comment.

> +@@ -3325,9 +3341,13 @@
> + path = fil_make_ibd_name(tablename, false);
> + }
> +
> ++ /* The files can already exist in the case of archived logs applying */
> + file = os_file_create(
> + innodb_file_data_key, path,
> +- OS_FILE_CREATE | OS_FILE_ON_ERROR_NO_EXIT,
> ++ (srv_archive_recovery ?
> ++ OS_FILE_OVERWRITE :
> ++ OS_FILE_CREATE) |
> ++ OS_FILE_ON_ERROR_NO_EXIT,
> + OS_FILE_NORMAL,
> + OS_DATA_FILE,
> + &ret);
>
> - s/alligned to the up of log file block/aligned up to the log block size/

Done.

> - please clarify why the following change is needed. As I
> understand, we don't start the FTS optimize thread in
> XtraBackup. That code is also missing block braces and is using
> spaces instead of tabs for indentation:
>
> +@@ -3032,7 +3032,8 @@
> +
> + ib_logf(IB_LOG_LEVEL_INFO, "FTS optimize thread exiting.");
> +
> +- os_event_set(exit_event);
> ++ if (exit_event)
> ++ os_event_set(exit_event);
> +
> + /* We count the number of threads in os_thread_exit(). A created
> + thread should always use that to exit and not use return() to exit. */
> +

I fixed using spaces instead tabs for indentation and missing braces.

As I understuud FTS optimize thread is started in xtrabackup because I
have the following backtrace in the case of missing exit_event condition:
56 ../nptl/sysdeps/unix/sysv/linux/raise.c: No such file or directory.
(gdb) bt
#0 0x00007ffff633f037 in __GI_raise (sig=sig@entry=6) at
../nptl/sysdeps/unix/sysv/linux/raise.c:56
#1 0x00007ffff6342698 in __GI_abort () at abort.c:90
#2 0x000000000063ac80 in os_event_set (event=0x0)
     at
/home/vlesin/src/work/percona-xtrabackup-2.1-arch-log/mysql-5.6/storage/innobase/os/os0sync.cc:437
#3 0x00000000006063ec in fts_optimize_thread (arg=0x1e604e8)
     at
/home/vlesin/src/work/percona-xtrabackup-2.1-arch-log/mysql-5.6/storage/innobase/fts/fts0opt.cc:3036
#4 0x00007ffff79bcf8e in start_thread (arg=0x7fffd57fa700) at
pthread_create.c:311
#5 0x00007ffff6401e1d in clone () at
../sysdeps/unix/sysv/linux/x86_64/clone.S:113

Here is the backtrace of FTS initialization:
(gdb) bt
#0 fts_optimize_init () at
/home/vlesin/src/work/percona-xtrabackup-2.1-arch-log/mysql-5.6/storage/innobase/fts/fts0opt.cc:3053
#1 0x00000000004a8364 in innobase_start_or_create_for_mysql ()
     at
/home/vlesin/src/work/percona-xtrabackup-2.1-arch-log/mysql-5.6/storage/innobase/srv/srv0start.cc:2767
#2 0x00000000004501fc in innodb_init () at xtrabackup.cc:1416
#3 0x0000000000458400 in xtrabackup_prepare_func () at xtrabackup.cc:5392
#4 0x0000000000459951 in main (argc=0, argv=0x15eb1f8) at
xtrabackup.cc:5952

And no one patch contains disabling of this initialization.

> - please use LSN_PF instead of "%llu"

I think this is unnecessary changes because I added it for
debug purpose and forgot to remove.

> - in recv_recovery_from_archive_start() we ignore the first_log_no
> argument and use xtrabackup_arch_first_file_lsn instead. wouldn't
> it be better to pass xtrabackup_arch_first_file_lsn as the
> first_log_no argument to recv_recovery_from_archive_start() and
> get rid of min/max_arch_log_no in
> innobase_start_or_create_for_mysql()?

I think no because at my point of view changes in xtrabackup.cc are
easily supported than changes in innodb engine because the last are in
the patch file which grows and the further changes depends on the
previous ones and it hard to track that changes because we don't see
the entire picture in the patch file. So I think all the changes of
innodb should be minimal. Do you have another opinion?

> - in xb_data_files_init() the patch replaces min/max_flushed_lsn
> declarations with min/max_arch_log_no. Which are not really
> used. but then the call still calls open_or_create_data_files()
> with the (now non-existing) flushed_lsn variables?

No, it does not replace min/max_flushed_lsn. They became global because
they are used in xtrabackup_prepare_func() to find
xtrabackup_arch_first_file_lsn. But there are two new variables
min/max_arch_log_no when UNIV_LOG_ARCHIVE is defined. They are not used,
but open_or_create_data_files() expects them as arguments if
UNIV_LOG_ARCHIVE is defined.

> - s/xtrabackup: Note: opening archived log directory %s was
> failed/xtrabackup: error: cannot open archived log directory %s/
>
> - replace the strstr() call in xtrabackup_arch_search_files() with
> strncmp()
>

Done.

> - xtrabackup_arch_search_files() is declared to return bool, but
> returns xtrabackup_arch_first_file_lsn?

Yes, the condition is "xtrabackup_arch_first_file_lsn != 0", but I
decided to use the shorter record. But you are right, it is more
understandable to use explicit cast.

> - we tend to use the InnoDB code style in xtrabackup.cc. In
> particular, the opening brace must be on the same line as the
> 'if' statement. And braces should be used even for
> single-statement 'if' blocks;
>
> - s/trunsactions/transactions/
>
> - s/undid/rolled back/

Done.

> - the check for Percona Server 5.6 doesn't work (try starting it
> with an upstream 5.6 server). The reason is that the syntax for
> string equality comparison is "val1 = val2", not "val1=val2"

Done.

> - the test case doesn't work for me (fails with "table 'test.t1'
> doesn't exist" when run against PS 5.6 with log archiving enabled)
>

I need your assistance here because I can't reproduce this on my
environment and, as I understood, on Jenkins environment too(see
http://jenkins.percona.com/view/XtraBackup/job/percona-xtrabackup-2.1-param/298/#showFailuresLink).

--
Vlad Lesin, Software Engineer, Percona Inc.
<email address hidden>
skype: vlad_lesin
JID: <email address hidden>
ICQ: 204036003
phone: +79051122311
Tula, Russia (GMT +4)