Merge into 2.1 : 2.1-apply-archived-logs-innodb5.6 : Code : Percona XtraBackup moved to https://jira.percona.com/projects/PXB

Reviewer	Date Requested	Status
Alexey Kopytov (community)	2013-04-25	Approve on 2013-08-27
Laurynas Biveinis (community)		Needs Fixing on 2013-04-25
Review via email: mp+160804@code.staging.launchpad.net

Revision history for this message

Laurynas Biveinis (laurynas-biveinis) wrote on 2013-04-25:

#

A merge conflict, unless spurious, that needs rebasing on the current trunk?

review: Needs Fixing

Revision history for this message

Vlad Lesin (vlad-lesin) wrote on 2013-04-26:

#

> A merge conflict, unless spurious, that needs rebasing on the current trunk?
I have just rebased it on the current trunk.

Revision history for this message

Alexey Kopytov (akopytov) wrote on 2013-05-01:

#

Download full text (3.2 KiB)

Vlad,

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

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

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

    - 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. */
+

- please use LSN_PF instead of "%llu"

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

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

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

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

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

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

- the test case doesn't wo...

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.
vladislav.lesin@percona.com
skype: vlad_lesin
JID: vladlesin@gmail.com
ICQ: 204036003
phone: +79051122311
Tula, Russia (GMT +4)

Revision history for this message

Vlad Lesin (vlad-lesin) wrote on 2013-05-10:

#

On 05/06/2013 06:21 PM, Sergei Glushchenko wrote:
> Vlad,
>
> Why don't we use --log-arch-dir option (with same name as server one) instead of --archived-logs-dir?
>

Done.

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

Revision history for this message

Alexey Kopytov (akopytov) wrote on 2013-05-12:

#

Download full text (6.3 KiB)

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

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.

Revision history for this message

Vlad Lesin (vlad-lesin) wrote on 2013-05-13:

#

On 05/12/2013 04:48 PM, Alexey Kopytov wrote:
>>> - 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

As I understood our chat right the main cause of this change is to
remove 'fake' arguments from recv_recovery_from_archive_start().

Why just don't remove first_log_no from
recv_recovery_from_archive_start() arguments list? I don't see any
benefits in passing xtrabackup_arch_first_file_lsn and
xtrabackup_arch_last_file_lsn to recv_recovery_from_archive_start() as
arguments because in this case we just remove using global external
variables from recv_recovery_from_archive_start() to
innobase_start_or_create_for_mysql().

>>> get rid of min/max_arch_log_no in
>>> innobase_start_or_create_for_mysql()?

Right. In this case we could remove min/max_arch_log_no from
open_or_create_data_files() arguments list and from xb_data_files_init()
too.

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

Revision history for this message

Vlad Lesin (vlad-lesin) wrote on 2013-05-13:

#

On 05/12/2013 04:48 PM, Alexey Kopytov wrote:

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

I am preparing the more detailed answer with code fixes so currently I
don't want to commit unfinished changes. But it is possible just to
comment log_checkpoint_get_nth_group_info() invoking at that place
because the values it reads from buffer are not used for logs applying.

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

Revision history for this message

Alexey Kopytov (akopytov) wrote on 2013-05-14:

#

Hi Vlad,

On Mon, 13 May 2013 17:44:36 +0400, Vlad Lesin wrote:
> On 05/12/2013 04:48 PM, Alexey Kopytov wrote:
>>>> - 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
>
> As I understood our chat right the main cause of this change is to
> remove 'fake' arguments from recv_recovery_from_archive_start().
>
> Why just don't remove first_log_no from
> recv_recovery_from_archive_start() arguments list? I don't see any
> benefits in passing xtrabackup_arch_first_file_lsn and
> xtrabackup_arch_last_file_lsn to recv_recovery_from_archive_start() as
> arguments because in this case we just remove using global external
> variables from recv_recovery_from_archive_start() to
> innobase_start_or_create_for_mysql().
>

Either way is fine by me as long as we avoid fake arguments.

> >>> get rid of min/max_arch_log_no in
> >>> innobase_start_or_create_for_mysql()?
>
> Right. In this case we could remove min/max_arch_log_no from
> open_or_create_data_files() arguments list and from xb_data_files_init()
> too.
>

OK.

Revision history for this message

Vlad Lesin (vlad-lesin) wrote on 2013-06-13:

#

> > Why just don't remove first_log_no from
> > recv_recovery_from_archive_start() arguments list? I don't see any
> > benefits in passing xtrabackup_arch_first_file_lsn and
> > xtrabackup_arch_last_file_lsn to recv_recovery_from_archive_start() as
> > arguments because in this case we just remove using global external
> > variables from recv_recovery_from_archive_start() to
> > innobase_start_or_create_for_mysql().
> >
>
> Either way is fine by me as long as we avoid fake arguments.
Done.

> > Right. In this case we could remove min/max_arch_log_no from
> > open_or_create_data_files() arguments list and from xb_data_files_init()
> > too.
> >
>
> OK.
Done.

Revision history for this message

Vlad Lesin (vlad-lesin) wrote on 2013-06-13:

#

> 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
The fix is released.

> - 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.
The fix for 2.1 is merged and for 2.0 is approved for merging.

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

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

> 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.
I explored this problem more deeply and found out that the problem is "apply archived log" specific. See my comment to the bug report and
                if (srv_apply_log_only) {
                        goto skip_processes;
                }
strings in innobase_start_or_create_for_mysql().

> >> - 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.
Fixed. The invocation of log_checkpoint_get_nth_group_info() is commented in this place because there is no need to read archived_file_no and archived_offset. They are calculated another way.

> 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
The fix is released.

>      - 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.
The fix for 2.1 is merged and for 2.0 is approved for merging.

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

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

> 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.
I explored this problem more deeply and found out that the problem is "apply archived log" specific. See my comment to the bug report and
                if (srv_apply_log_only) {                                        
                        goto skip_processes;                                     
                } 
strings in innobase_start_or_create_for_mysql().

> >>      - 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.
Fixed. The invocation of log_checkpoint_get_nth_group_info() is commented in this place because there is no need to read archived_file_no and archived_offset. They are calculated another way.

Revision history for this message

Vlad Lesin (vlad-lesin) wrote on 2013-07-31:

#

Rebased on newest trunk (lp:percona-xtrabackup revision 650).

Revision history for this message

Alexey Kopytov (akopytov) wrote on 2013-07-31:

#

I don't think XtraDB-related changes in run.sh are necessary now. Check the is_xtradb()/require_xtradb() functions and how they are used in other tests.

Revision history for this message

Alexey Kopytov (akopytov) wrote on 2013-07-31:

#

Oh, and you will also need require_server_version_higher_than()

Revision history for this message

Vlad Lesin (vlad-lesin) wrote on 2013-07-31:

#

On 07/31/2013 04:54 PM, Alexey Kopytov wrote:
> I don't think XtraDB-related changes in run.sh are necessary now. Check the is_xtradb()/require_xtradb() functions and how they are used in other tests.
>
Yes, you are right, there is no need to change run.sh as server and
innodb flavours are detected right now based on "innodb_version" server
variable.

But require_server_version_higher_than() and
require_xtradb()/require_xtradb() does not suite me because
require_server_version_higher_than() checks only server version but I
need to check server flavour too as well as
require_xtradb()/require_xtradb() checks only the fact of xtradb using
but xtradb version must be checked too.

So I reverted run.sh and fixed xb_apply_archived_logs.sh to use
is_server_version_higher_than() for server version checking.

Revision history for this message

Alexey Kopytov (akopytov) wrote on 2013-08-01:

#

Hi Vlad,

On Wed, 31 Jul 2013 21:43:33 -0000, Vlad Lesin wrote:
> But require_server_version_higher_than() and
> require_xtradb()/require_xtradb() does not suite me because
> require_server_version_higher_than() checks only server version but I
> need to check server flavour too as well as
> require_xtradb()/require_xtradb() checks only the fact of xtradb using
> but xtradb version must be checked too.
>

You can do:

require_xtradb
require_server_version_higher_than 5.6.10

Or use is_* functions if you need something complex.

Revision history for this message

Vlad Lesin (vlad-lesin) wrote on 2013-08-01:

#

On 08/01/2013 08:39 AM, Alexey Kopytov wrote:
> Hi Vlad,
>
> On Wed, 31 Jul 2013 21:43:33 -0000, Vlad Lesin wrote:
>> But require_server_version_higher_than() and
>> require_xtradb()/require_xtradb() does not suite me because
>> require_server_version_higher_than() checks only server version but I
>> need to check server flavour too as well as
>> require_xtradb()/require_xtradb() checks only the fact of xtradb using
>> but xtradb version must be checked too.
>>
>
> You can do:
>
> require_xtradb
> require_server_version_higher_than 5.6.10
>
> Or use is_* functions if you need something complex.
>

Yes, but xtradb can be 5.5,5.1 or 5.6. But only 5.6 supports logs applying.

Revision history for this message

Alexey Kopytov (akopytov) wrote on 2013-08-01:

#

Hi Vlad,

On Thu, 01 Aug 2013 11:01:56 +0400, Vlad Lesin wrote:
>> You can do:
>>
>> require_xtradb
>> require_server_version_higher_than 5.6.10
>>
>> Or use is_* functions if you need something complex.
>>
>
> Yes, but xtradb can be 5.5,5.1 or 5.6. But only 5.6 supports logs applying.
>

The above 2 lines will make the test run only on XtraDB > 5.6.10?

Revision history for this message

Vlad Lesin (vlad-lesin) wrote on 2013-08-01:

#

http://jenkins.percona.com/view/XtraBackup/job/percona-xtrabackup-2.1-param/415/ - testing with PS-5.6.11
http://jenkins.percona.com/view/XtraBackup/job/percona-xtrabackup-2.1-param/414/ - testing with PS-5.6.12 and statically built openssl

Revision history for this message

Alexey Kopytov (akopytov) wrote on 2013-08-02:

#

Vlad,

Do you have any explanations for multiple test failures and crashes in both builds?

review: Needs Information

Revision history for this message

Vlad Lesin (vlad-lesin) wrote on 2013-08-09:

#

> Vlad,
>
> Do you have any explanations for multiple test failures and crashes in both
> builds?

Yes, there were bug concerned with wrong lsn writing to data files on "prepare" step when logs applying is not used. I fixed it, here is the new test results:
http://jenkins.percona.com/view/XtraBackup/job/percona-xtrabackup-2.1-param/420/

Revision history for this message

Alexey Kopytov (akopytov) wrote on 2013-08-23:

#

Vlad,

Approved, but please rebase on the current trunk. I get many conflicts in innodb56.patch. The good news is that it's going to be the last rebase for this branch. Sorry about the stretched review process. Please also do another Jenkins param build after rebasing.

Thanks.

review: Approve

Revision history for this message

Vlad Lesin (vlad-lesin) wrote on 2013-08-26:

#

> Vlad,
>
> Approved, but please rebase on the current trunk. I get many conflicts in
> innodb56.patch. The good news is that it's going to be the last rebase for
> this branch. Sorry about the stretched review process. Please also do another
> Jenkins param build after rebasing.
>
> Thanks.

Done.

http://jenkins.percona.com/view/XtraBackup/job/percona-xtrabackup-2.1-param/443/

Revision history for this message

Alexey Kopytov (akopytov) on 2013-08-27:

#

review: Approve

Percona XtraBackup moved to https://jira.percona.com/projects/PXB

Merge lp://staging/~vlad-lesin/percona-xtrabackup/2.1-apply-archived-logs-innodb5.6 into lp://staging/percona-xtrabackup/2.1

Commit message

Description of the change

Preview Diff

Subscribers