I realized that the MySQL 5.6/MariaDB 10.1 innochecksum does not have code to update the data files. 5.7/10.2 got the ability to rewrite page checksums. I did a quick attempt at porting innochecksum from 10.2 to 10.1, but that felt too risky. So, instead of writing a proper innochecksum tool, I wrote a quick&dirty Perl routine that downgrades the FSP_SPACE_FLAGS to the old 10.1.x format. That routine is used in the test innodb.101_compatibility that I added. Jan, please review bb-10.1-mdev-11623. I hope to push it to 10.1 tomorrow if the tests are fine. Elena is running some tests. Marko On Wed, Jan 11, 2017 at 3:42 PM, Marko Mäkelä <marko.makela@mariadb.com> wrote:
Sorry for the delay. This patch looks great to me, except for one thing: I think that we should write the corrected flags back to the data file.
Given that 10.1 is already in GA status, we should consider our options carefully. My preference would be (1) below:
(1) If we adjust 10.1.21 so that it writes back corrected flags for both old and new data files, then older 10.1.x would be unable to open the files (if compression, DATA_DIRECTORY or non-default innodb_page_size was used).
(2) If we adjust 10.1.21 so that it writes correct flags for new data files (like the current patch does), then older 10.1.x would not be able to read those created-with-newer-version data files.
(3) If we adjust 10.2 only, so that it writes the correct flags for new files and also adjusts the flags in old files, then we would seem to have a clear upgrade path.
Whatever we choose, I would like to remove the flag-adjustment code from 10.3 or 10.4. This would mean that an upgrade (or import) directly from 10.1 is only possible if the problematic features (DATA_DIRECTORY, compression, non-standard innodb_page_size) were not used. Otherwise the upgrade would have to be done through 10.2.
On Thu, Dec 29, 2016 at 10:41 PM, Jan Lindström <jan.lindstrom@mariadb.com> wrote:
revision-id: a2a42a9181729b661ba308f2d0a75d8b547ed09d (mariadb-10.1.20-15-ga2a42a9) parent(s): 23cc1be270c7304963643947d8e5ab88f4e312ee author: Jan Lindström committer: Jan Lindström timestamp: 2016-12-29 22:40:30 +0200 message:
MDEV-11623: MariaDB 10.1 fails to start datadir created with MariaDB 10.0/MySQL 5.6 using innodb-page-size!=16K
Problem was that in MariaDB 10.1 page size flag is on different position (13) on tablespace flags compared to MySQL 5.6 and MariaDB 10.0 or older position (6). DATA_DIR flag position difference was already handled.
/** Tablespace flags position and name in MySQL 5.6/MariaDB 10.0 or older and MariaDB 10.1.20 or older MariaDB 10.1 and in MariaDB 10.1.21 or newer. MySQL 5.6 MariaDB 10.1.x MariaDB 10.1.21 ==================================================================== Below flags in same offset ==================================================================== 0: POST_ANTELOPE 0:POST_ANTELOPE 0: POST_ANTELOPE 1: ZIP_SSIZE 1:ZIP_SSIZE 1: ZIP_SSIZE 5: ATOMIC_BLOBS 5:ATOMIC_BLOBS 5: ATOMIC_BLOBS
It could be clearer to explicitly identify multi-bit fields, e.g., 1..4 for ZIP_SSIZE.
===================================================================== Below note the difference in order ===================================================================== 6: PAGE_SSIZE 6:PAGE_COMPRESSION 6: PAGE_SSIZE 10: DATA_DIR 7:PAGE_COMPRESSION_LEVEL 10: DATA_DIR 11: UNUSED 11:ATOMIC_WRITES ===================================================================== Note that below flags have been moved ===================================================================== 13:PAGE_SSIZE 11: RESERVED (5.7 SHARED) 17:DATA_DIR 12: RESERVED (5.7 TEMPORARY) 18:UNUSED 13: RESERVED (5.7 ENCRYPTION) 14: PAGE_COMPRESSION 15: PAGE_COMPRESSION_LEVEL 19: ATOMIC_WRITES 21: UNUSED
s/have been moved/were in incorrect position in MariaDB 10.1.x/
It could be good to explain the impact of this. That is, what would happen if we upgrade from MariaDB 10.0 to the buggy MariaDB 10.1, or if we upgrade from the buggy MariaDB 10.1 to 10.1.21 where this has been fixed.
As far as I can tell, with respect to the DATA_DIR flag we should be mostly OK, because that flag should only play a role in the table flags when determining where the data file is located, not so much in the tablespace flags inside the data file. (Moving files from MariaDB 10.1 to MySQL 5.7 could be hurt, because the DATA_DIR flag would be interpreted as the 5.7 ENCRYPTION flag.)
Next, let us consider PAGE_SSIZE (bits 13..16 in the buggy MariaDB 10.1.x, 6..9 elsewhere).
Upgrade from 10.0 or 5.6 to the buggy 10.1.x: We read the bits 13..16. These would always be 0 in the old data files, so we will interpret the data file as if it had innodb_page_size=16k. That is, upgrade or import from 5.6 or 10.0 to the buggy 10.1.x will be refused if innodb_page_size differs from 16k. Is this correct? (This is something that we cannot fix, because we cannot retroactively change old MariaDB 10.1.x versions.)
Upgrade from the buggy 10.1.x to the fixed 10.1.x or 10.2: We read the bits 6..9 which were incorrectly used as PAGE_COMPRESSION, PAGE_COMPRESSION_LEVEL. That is, upgrade should work if innodb_page_size=16k (the flags are expected to be 0) and compression is not used (the bits 6..9 were written as 0 by the buggy 10.1.x).
To help users who use the buggy 10.1.x with innodb_page_size!=16k or with compression, we can adjust the flags:
fsp0fsp.ic: fsp_flags_get_page_size() : By default read used page size from original position i.e. after ATOMIC_BLOBS. But if we detect that buggy MariaDB 10.1 flags are used, then we read it from FSP_FLAGS_PAGE_SSIZE_MARIADB101. [snip] Tested also with MySQL 5.7 datadir (after deleting redo logs and copying mysql/* files from 10.1) but that failed because SYS_INDEXES got an extra column in 5.7 SYS_INDEXES.MERGE_THRESHOLD.
You could theoretically have initialized the database in MySQL 5.6 and then upgraded it to 5.7. All tables that were created in 5.6 should be accessible. But would not have added much value.
After shutdown, started MariaDB 10.1.21 with fix and verified that all tables created are accessible.
Were the flags corrected in the data file afterwards? I think that we should correct the flags.
Please add a mysql-test case that demonstrates that the flags are corrected. Something like this:
--source include/have_innodb.inc --source include/not_embedded.inc
# restart will restore the old values SET GLOBAL innodb_file_per_table=1; SET GLOBAL innodb_file_format=Barracuda; let INNODB_PAGE_SIZE=`select @@innodb_page_size`; let MYSQLD_DATADIR=`select @@datadir`;
CREATE TABLE tr(a INT)ENGINE=InnoDB ROW_FORMAT=REDUNDANT; CREATE TABLE tc(a INT)ENGINE=InnoDB ROW_FORMAT=COMPACT; CREATE TABLE td(a INT)ENGINE=InnoDB ROW_FORMAT=DYNAMIC; CREATE TABLE tz(a INT)ENGINE=InnoDB ROW_FORMAT=COMPRESSED KEY_BLOCK_SIZE=1; # TODO: repeat the same with DATA_DIRECTORY and COMPRESSION --source include/shutdown_mysqld.inc
perl; # corrupt the FSP_FLAGS in each file; write the page checksum as 0xdeadbeef # (see log_data_file_size.test or some recent test from MySQL) EOF;
--let $restart_parameters = --innodb-read-only; --source include/start_mysqld.inc CHECK TABLE tr; CHECK TABLE tc; CHECK TABLE td; CHECK TABLE tz; --source include/shutdown_mysqld.inc
perl; # check that the FSP_FLAGS are still corrupted # (they must not be adjusted in read-only mode) EOF;
--let $restart_parameters=; --source include/start_mysqld.inc CHECK TABLE tr; CHECK TABLE tc; CHECK TABLE td; CHECK TABLE tz; --source include/shutdown_mysqld.inc
perl; # check that the FSP_FLAGS now correspond to $ENV{INNODB_PAGE_SIZE} EOF; --source include/start_mysqld.inc DROP TABLE tr,tc,td,tz;
@@ -3859,21 +3837,11 @@ fil_open_single_table_tablespace( + /* Validate this single-table-tablespace with SYS_TABLES. */ + bool flags_correct = dict_compare_flags(def.flags, flags);
I think that the function name should be more descriptive, making clear that the first argument contains the table flags and the second the FIL_SPACE_FLAGS:
dict_tf_match_space(def.flags, flags)
The comment does not really add any value. I would omit it.
diff --git a/storage/innobase/fsp/fsp0fsp.cc b/storage/innobase/fsp/fsp0fsp.cc index 87aa5f7..7ff5244 100644 --- a/storage/innobase/fsp/fsp0fsp.cc +++ b/storage/innobase/fsp/fsp0fsp.cc @@ -4182,3 +4182,141 @@ fsp_page_is_free_func( return xdes_mtr_get_bit( descr, XDES_FREE_BIT, page_no % FSP_EXTENT_SIZE, mtr); } + +/********************************************************************//** +Verify that dictionary flags modified to tablespace flags +and actual tablespace flags stored to FSP header match. +@param[in] dict_flags dict_tf_to_fsp_flags(dict_table_t::flags) +@param[in] fsp_flags Actual flags stored to FSP header in page 0 +@return true if flags match, false if not */ +bool +fsp_verify_flags( + ulint dict_flags, + ulint fsp_flags) +{ + ulint dict_unused = FSP_FLAGS_GET_UNUSED(dict_flags); + ulint dict_antelope = FSP_FLAGS_GET_POST_ANTELOPE(dict_flags); + ulint dict_zssize = FSP_FLAGS_GET_ZIP_SSIZE(dict_flags); + ulint dict_ablobs = FSP_FLAGS_HAS_ATOMIC_BLOBS(dict_flags);
Add an early return before all these computations if dict_flags==fsp_flags.
I think that the name dict_flags is misleading. Both flags are supposed to be tablespace flags, not data dictionary (or table) flags.
How about this:
s/dict_flags/expected/ s/fsp_flags/actual/
And then define the local variables with expected_ and actual_ prefixes, instead of dict_ and fsp_. That would make the code easier to follow.
+ DBUG_EXECUTE_IF("fsp_verify_flags_failure", + return(false););
Where is the test to exercise this? Do we even need this? I think it is better to inject faults into the data files using Perl code in mysql-test (see my previous suggestion).
Other than this, the function looks OK to me.
diff --git a/storage/innobase/include/dict0dict.h b/storage/innobase/include/dict0dict.h index 42f93b5..479954d 100644 --- a/storage/innobase/include/dict0dict.h +++ b/storage/innobase/include/dict0dict.h @@ -1904,6 +1904,17 @@ dict_table_get_index_on_first_col( #endif /* !UNIV_HOTBACKUP */
+/** Compare tablespace flags. +@param[in] flags SYS_TABLES.flags +@param[in] mod_flags Tablespace flags. +@return true if flags match, false if not */ +UNIV_INLINE +bool +dict_compare_flags( + ulint flags, + ulint mod_flags) + MY_ATTRIBUTE((warn_unused_result)); +
Can we declare the inline function directly in the .h file, instead of splitting it into the .ic file?
I would suggest different names:
bool dict_tf_match_space(ulint table_flags, ulint fsp_flags);
@@ -980,12 +982,11 @@ dict_sys_tables_type_to_tf( PAGE_COMPRESSION_LEVEL, ATOMIC_WRITES are the same. */ flags |= type & (DICT_TF_MASK_ZIP_SSIZE | DICT_TF_MASK_ATOMIC_BLOBS - | DICT_TF_MASK_DATA_DIR - | DICT_TF_MASK_PAGE_COMPRESSION - | DICT_TF_MASK_PAGE_COMPRESSION_LEVEL - | DICT_TF_MASK_ATOMIC_WRITES + | DICT_TF_MASK_DATA_DIR + | DICT_TF_MASK_PAGE_COMPRESSION + | DICT_TF_MASK_PAGE_COMPRESSION_LEVEL + | DICT_TF_MASK_ATOMIC_WRITES);
- );
return(flags); }
This appears to be a white-space-only change. We could avoid it, unless this is actually fixing the indentation. The spaces and TABs got lost in transit, so I cannot tell..
@@ -1018,8 +1019,8 @@ dict_tf_to_sys_tables_type( type |= flags & (DICT_TF_MASK_ZIP_SSIZE | DICT_TF_MASK_ATOMIC_BLOBS | DICT_TF_MASK_DATA_DIR - | DICT_TF_MASK_PAGE_COMPRESSION - | DICT_TF_MASK_PAGE_COMPRESSION_LEVEL + | DICT_TF_MASK_PAGE_COMPRESSION + | DICT_TF_MASK_PAGE_COMPRESSION_LEVEL | DICT_TF_MASK_ATOMIC_WRITES);
return(type);
Same here.
DBUG_EXECUTE_IF("dict_tf_verify_flags_failure", - return(ULINT_UNDEFINED);); + return(false););
Nice catch. But is there any test to exercise this? If not, I would remove the DBUG_EXECUTE_IF altogether.
ut_a(page_ssize == 0 || page_ssize != 0); /* silence compiler */ ut_a(compact == 0 || compact == 1); /* silence compiler */ ut_a(data_dir == 0 || data_dir == 1); /* silence compiler */ - ut_a(post_antelope == 0 || post_antelope == 1); /* silence compiler */ + ut_a(post_antelope == 0 || post_antelope == 1); /* silence compiler + */
I think it would be better to declare these variables as __attribute__((unused)) or to remove the variables altogether if they are unused.
+ + if (table_unused || fsp_unused) { + ib_logf(IB_LOG_LEVEL_ERROR, + "Table flags has unused %ld" + " in the data dictionary" + " but the flags in file has unused %ld\n", + table_unused, fsp_unused); + + return (false); + }
0x%lx would be more user-friendly here.
+ ib_logf(IB_LOG_LEVEL_ERROR, + "Table flags has page_compression %ld" + " in the data dictionary" + " but the flags in file ahas page_compression %ld\n", page_compression, fsp_page_compression);
s/ahas/has/
+/********************************************************************//** +Verify that dictionary flags modified to tablespace flags +and actual tablespace flags stored to FSP header match. +@param[in] dict_flags dict_tf_to_fsp_flags(dict_table_t::flags) +@param[in] fsp_flags Actual flags stored to FSP header in page 0 +@return true if flags match, false if not */ +bool +fsp_verify_flags( + ulint dict_flags, + ulint fsp_flags) + MY_ATTRIBUTE((warn_unused_result)); +
This seems to be missing UNIV_INTERN? I think we need it in 10.1 at least for innodb_plugin.
+ /** In MariaDB 10.1.20 or older MariaDB 10.1 PAGE_SSIZE + was stored after ATOMIC_WRITES not after ATOMIC_BLOBS + as in MySQL 5.6, MariaDB 10.0 or older. New tables + in MariDB 10.1.21 or newer store PAGE_SSIZE after ATOMIC_BLOBS. + */ + ssize = FSP_FLAGS_GET_PAGE_SSIZE(flags);
Do not use /** comments before local variables.
I think that one of the ‘after’ in the comment should be ‘before’. Also fix the typo MariDB. -- DON’T MISS M|17 April 11 - 12, 2017 The Conrad Hotel New York City https://m17.mariadb.com/
Marko Mäkelä, Lead Developer InnoDB MariaDB Corporation
-- DON’T MISS M|17 April 11 - 12, 2017 The Conrad Hotel New York City https://m17.mariadb.com/ Marko Mäkelä, Lead Developer InnoDB MariaDB Corporation