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

Hi Vlad,

General comments:

     - I think this MP should wait until 5.6.11-60.3 with the fix for bug
       #1172591 is released. Then the test suite should use 5.6.11-60.30
       for testing instead of the custom build

     - there should be a separate fix for bug #1177182 for both 2.0 and
2.1, and again, this branch should be rebased once the fix is merged to
trunks.

     - xtrabackup_arch_search_files() should use msg() instead of
       fprintf(stderr,...)

Other comments/questions:

On Fri, 10 May 2013 09:42:06 +0400, Vlad Lesin wrote:
>> - 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.
>

If a tablespace exists on recovery, it is created into fil_system before
the recovery starts. When replaying MLOG_FILE_CREATE,
fil_op_log_parse_or_replay first checks if the tablespace exists in
fil_system and does nothing. So I still don't understand that change in
fil_create_new_single_table_tablespace().

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

Not fully done, it is "aligned" (single "l"), not "alligned".

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

OK, I see the problem now, reported as bug #1179193. This means we have
to fix that bug separately in both 2.0 and 2.1, and then rebase this
branch on 2.1 trunk with that fix included.

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

Yes, I plan to remove patches to InnoDB from XtraBackup source tree as
soon as possible. I.e. the XtraBackup source tree will be a 5.6-based
tree with innodb56.patch merged. Otherwise we will eventually drown in
patch maintenance. But this also means the issue of implementing changes
in XtraBackup code to simplify patch maintenance disappears. I.e. we are
free to implement changes where they actually belong.

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

OK.

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

I tried to repeat the test, but your branch doesn't build for me now, it
fails as follows:

[ 70%] Building CXX object
libmysqld/CMakeFiles/sql_embedded.dir/__/sql/field_conv.cc.o
/Users/kaa/src/launchpad/percona-xtrabackup/2.1-apply-archived-logs-innodb5.6/mysql-5.6/storage/innobase/log/log0recv.cc:3142:3:
error: no matching function for call to
       'log_checkpoint_get_nth_group_info'
                 log_checkpoint_get_nth_group_info(buf, group->id,
                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/Users/kaa/src/launchpad/percona-xtrabackup/2.1-apply-archived-logs-innodb5.6/mysql-5.6/storage/innobase/include/log0log.h:286:1:
note: candidate function not viable: no known
       conversion from 'lsn_t *' (aka 'unsigned long long *') to 'ulint
*' (aka 'unsigned long *') for 3rd argument;
log_checkpoint_get_nth_group_info(
^
1[ 70%] error generated.