Merge lp:~sergei.glushchenko/percona-xtrabackup/xb20-bug976945 into lp:percona-xtrabackup/2.0

Proposed by Sergei Glushchenko
Status: Merged
Approved by: Alexey Kopytov
Approved revision: no longer in the source branch.
Merged at revision: 451
Proposed branch: lp:~sergei.glushchenko/percona-xtrabackup/xb20-bug976945
Merge into: lp:percona-xtrabackup/2.0
Diff against target: 125 lines (+69/-20)
2 files modified
src/xtrabackup.c (+34/-20)
test/t/bug976945.sh (+35/-0)
To merge this branch: bzr merge lp:~sergei.glushchenko/percona-xtrabackup/xb20-bug976945
Reviewer Review Type Date Requested Status
Alexey Kopytov (community) Approve
Review via email: mp+110468@code.launchpad.net

Description of the change

Bug976945: innodb_log_block_size=4096 is not supported
When called with --prepare xtrabackup didn't initialize
log_block_size properly.
Solution is to initialize log_block_size from inside
xtrabackup_init_temp_log when --prepare called.

http://jenkins.percona.com/view/XtraBackup/job/percona-xtrabackup-2.0-param/211/

#24231

To post a comment you must log in.
Revision history for this message
Alexey Kopytov (akopytov) wrote :

Sergei,

- let's do some cleanups instead of copy-pasting the code. Please remove the ####EXPERIMENTAL#### warning, as it's not clear what users are supposed to do with that information.

- I would do the error handling in the xb_init_log_block_size() rather than rely on the callers. I.e. print the error if srv_log_block_size is zero and return TRUE.

- I would rather put #ifdef XTRADB_BASED inside xb_init_log_block_size() so it's an empty function for other configurations and we have less #ifdefs in the code.

- there's no need to check for xtrabackup_prepare in innodb_init_param(). It's OK to call this function twice. And it doesn't hurt to print the informational message twice. In fact, in my compact backups branch innodb_init_param() may be called multiple times at prepare.

- the test case is incompatible with the changes to the test suite I pushed recently. "run_mysqld" and "stop_mysqld" have been renamed to "start_server" and "stop_server" respectively, and "init" has been removed. Please pull from 2.0 and update the test case.

review: Needs Fixing
Revision history for this message
Sergei Glushchenko (sergei.glushchenko) wrote :

Fixed

Revision history for this message
Alexey Kopytov (akopytov) :
review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/xtrabackup.c'
2--- src/xtrabackup.c 2012-06-14 10:50:28 +0000
3+++ src/xtrabackup.c 2012-06-18 03:44:19 +0000
4@@ -1254,7 +1254,7 @@
5 (G_PTR*) &innobase_page_size, (G_PTR*) &innobase_page_size, 0,
6 GET_LONG, REQUIRED_ARG, (1 << 14), (1 << 12), (1 << UNIV_PAGE_SIZE_SHIFT_MAX), 0, 1L, 0},
7 {"innodb_log_block_size", OPT_INNODB_LOG_BLOCK_SIZE,
8- "###EXPERIMENTAL###: The log block size of the transaction log file. "
9+ "The log block size of the transaction log file. "
10 "Changing for created log file is not supported. Use on your own risk!",
11 (G_PTR*) &innobase_log_block_size, (G_PTR*) &innobase_log_block_size, 0,
12 GET_ULONG, REQUIRED_ARG, 512, 512, 1 << UNIV_PAGE_SIZE_SHIFT_MAX, 0, 1L, 0},
13@@ -2112,6 +2112,34 @@
14 return (value >> 1) ? 0 : shift;
15 }
16
17+/***********************************************************************
18+Initializes log_block_size*/
19+static
20+ibool
21+xb_init_log_block_size(void)
22+{
23+#ifdef XTRADB_BASED
24+ srv_log_block_size = 0;
25+ if (innobase_log_block_size != 512) {
26+ uint n_shift = get_bit_shift(innobase_log_block_size);;
27+
28+ if (n_shift > 0) {
29+ srv_log_block_size = (1 << n_shift);
30+ msg("InnoDB: The log block size is set to %lu.\n",
31+ srv_log_block_size);
32+ }
33+ } else {
34+ srv_log_block_size = 512;
35+ }
36+ if (!srv_log_block_size) {
37+ msg("InnoDB: Error: %lu is not valid value for "
38+ "innodb_log_block_size.\n", innobase_log_block_size);
39+ return FALSE;
40+ }
41+#endif
42+ return TRUE;
43+}
44+
45 static my_bool
46 innodb_init_param(void)
47 {
48@@ -2153,25 +2181,7 @@
49 srv_page_size = (1 << srv_page_size_shift);
50 }
51
52- srv_log_block_size = 0;
53- if (innobase_log_block_size != 512) {
54- uint n_shift = get_bit_shift(innobase_log_block_size);;
55-
56- msg("InnoDB: Warning: innodb_log_block_size has "
57- "been changed from its default value. "
58- "(###EXPERIMENTAL### operation)\n");
59- if (n_shift > 0) {
60- srv_log_block_size = (1 << n_shift);
61- msg("InnoDB: The log block size is set to %lu.\n",
62- srv_log_block_size);
63- }
64- } else {
65- srv_log_block_size = 512;
66- }
67-
68- if (!srv_log_block_size) {
69- msg("InnoDB: Error: %lu is not valid value for "
70- "innodb_log_block_size.\n", innobase_log_block_size);
71+ if (!xb_init_log_block_size()) {
72 goto error;
73 }
74
75@@ -5117,6 +5127,10 @@
76
77 max_no = ut_dulint_zero;
78
79+ if (!xb_init_log_block_size()) {
80+ goto error;
81+ }
82+
83 if(!xtrabackup_incremental_dir) {
84 sprintf(dst_path, "%s/ib_logfile0", xtrabackup_target_dir);
85 sprintf(src_path, "%s/%s", xtrabackup_target_dir,
86
87=== added file 'test/t/bug976945.sh'
88--- test/t/bug976945.sh 1970-01-01 00:00:00 +0000
89+++ test/t/bug976945.sh 2012-06-18 03:44:19 +0000
90@@ -0,0 +1,35 @@
91+############################################################################
92+# Bug #976945: innodb_log_block_size=4096 is not supported
93+############################################################################
94+. inc/common.sh
95+
96+if [ -z "$XTRADB_VERSION" ]; then
97+ echo "Requires XtraDB" > $SKIPPED_REASON
98+ exit $SKIPPED_EXIT_CODE
99+fi
100+
101+start_server --innodb_log_block_size=4096
102+echo innodb_log_block_size=4096 >> ${MYSQLD_VARDIR}/my.cnf
103+load_sakila
104+
105+# Full backup
106+vlog "Starting backup"
107+
108+full_backup_dir=${MYSQLD_VARDIR}/full_backup
109+innobackupex --no-timestamp $full_backup_dir
110+
111+vlog "Preparing backup"
112+innobackupex --apply-log --redo-only $full_backup_dir
113+vlog "Log applied to full backup"
114+
115+# Destroying mysql data
116+stop_server
117+rm -rf $mysql_datadir/*
118+vlog "Data destroyed"
119+
120+# Restore backup
121+vlog "Copying files to their original locations"
122+innobackupex --copy-back $full_backup_dir
123+vlog "Data restored"
124+
125+start_server --innodb_log_block_size=4096

Subscribers

People subscribed via source and target branches