Re: ea9a05f0821: This commit is about improving repair of tables
![](https://secure.gravatar.com/avatar/39b623a1559cf9c69ac3d9d4fb44e7fe.jpg?s=120&d=mm&r=g)
Hi, Michael, First, instead of situationally fixing repair code here and there, let's take a step back and see how repair works. I've walked the whole REPAIR code line by line, including all storage engines, and that's a simplified description: * if USE_FRM was specified * < FRM_VER_TRUE_VARCHAR(≈2005) && has varchar -> "too old" * if not MyISAM or Aria or CSV -> silently ignore USE_FRM * rename/truncate/rename * if ALGORITHM=COPY is needed (and not FORCE/USE_FRM), use it * otherwise, per engine: * MyISAM run repair, QUICK or EXTENDED, or QUICK then EXTENDED * Aria, same as MyISAM * S3 -> "Table '%-.192s' is read only" * CSV crashes * Archive, perform OPTIMIZE * all other engines * if needs upgrade: use ALTER ALGORITHM=COPY * otherwise "The storage engine for the table doesn't support REPAIR" * if ALGORITHM=COPY is needed but wasn't used -> "Operation failed" I think it's too complex to explain and not user friendly. Note that MyISAM internally can try one repair method and if that fails fall back to another one. So does Aria. Why cannot the server do the same? So, I'd suggest the following logic instead: * if USE_FRM - do USE_FRM stuff (or an warning) * if marked corrupted or EXTENDED or doesn't need upgrade - run repair * if repair isn't supported = ALTER IGNORE unless needs upgrade * if needs upgrade - run upgrade = ALTER IGNORE COPY * if fails because of corruption - go to step 2 if repair wasn't run yet here USE_FRM is handled as before (except that it adds a warning if USE_FRM isn't supported by the engine). After that the server runs engine's repair if the table is marked corrupted or if EXTENDED was used (so no need for a new FORCE syntax). There is no error "engine does not support REPAIR", the server falls back to ALTER automatically in that case. If the data need to be upgraded and ALTER fails, the server tries repair and then ALTER again - just like MyISAM and Aria try different repair methods. Also, note that ALTER should always use IGNORE to delete duplicates in unique after collation of hash changes - that's what repair in MyISAM/Aria does. Anyway, some detailed comments to your patch are inline, please, see below. On Feb 21, Michael Widenius wrote:
commit ea9a05f0821 Author: Michael Widenius
Date: Mon Jan 29 11:52:44 2024 +0200 This commit is about improving repair of tables
now as there is an MDEV, it should be here in the subject line
The 3 ways to repair a table are: 1) ALTER TABLE table_name FORCE" (not other options). As an alias we allow: "ALTER TABLE table_name ENGINE=original_engine" 2) "REPAIR TABLE" (without FORCE) 3) "OPTIMIZE TABLE"
All of the above commands will optimize row space usage (which means that space will be needed to hold a temporary copy of the table) and re-genereate all indexes. They will also try to replicate the original table definition as exact as possible.
For ALTER TABLE and "REPAIR TABLE without FORCE", the following will hold: If the table is from an older MariaDB version and data conversion is needed (for example for old type HASH columns, MySQL JSON type or new TIMESTAMP format) "ALTER TABLE table_name FORCE, algorithm=COPY" will be used.
The differences between the algoritms are 1) Will use the fastest algorithm the engine supports to do a full repair of the table (except if data conversions are is needed). 2) Will use the storage engine internal REPAIR facility (MyISAM, Aria). If the engine does not support REPAIR then "ALTER TABLE FORCE, ALGORITHM=COPY" will be used. If there was data incompatibilites (which means that FORCE was used) then there will be a warning after REPAIR that ALTER TABLE FORCE is still needed. The reason for this is that REPAIR may be able to go around data errors (wrong incompatible data, crashed or unreadable sectors) that ALTER TABLE cannot do. 3) Will use the storage engine internal OPTIMIZE. If engine does not support optimize, then "ALTER TABLE FORCE" is used.
The above will ensure that ALTER TABLE FORCE is able to correct almost any errors in the row or index data. In case of corrupted blocks then REPAIR possibile followed by ALTER TABLE is needed. This is important as mariadb-upgrade executes ALTER TABLE table_name FORCE for any table that must be re-created.
Bugs fixed with InnoDB tables: - No error for INNODB_DEFAULT_ROW_FORMAT=COMPACT even if row length would be too wide. (Independent of innodb_strict_mode). - Tables using symlinks will be symlinked after any of the above commands (Independent of the setting of --symbolic-links)
If one specifies an algorithm together with ALTER TABLE FORCE, things will work as before (except if data conversion is required as then the COPY algoritm is enforced).
ALTER TABLE .. OPTIMIZE ALL PARTITIONS will work as before.
Other things: - FORCE argument added to REPAIR to allow one to first run internal repair to fix damaged blocks and then follow it with ALTER TABLE. - REPAIR will not update frm_version if ha_check_for_upgrade() finds that table is still incompatible with current version. In this case the REPAIR will end with an error.
doesn't look like "the REPAIR will end with an error"
- Default error messages length for %M increased from 128 to 256 to not cut information from REPAIR. - Documented HA_ADMIN_XX variables related to repair. - Added HA_ADMIN_NEEDS_DATA_CONVERSION to signal that we have to do data conversions when converting the table (and thus ALTER TABLE copy algorithm is needed).
diff --git a/include/my_handler_errors.h b/include/my_handler_errors.h index df414888907..06dde5b3f74 100644 --- a/include/my_handler_errors.h +++ b/include/my_handler_errors.h @@ -101,7 +101,7 @@ static const char *handler_error_messages[]= "Operation was interrupted by end user (probably kill command?)", "Disk full", /* 190 */ - "Incompatible key or row definition between the MariaDB .frm file and the information in the storage engine. You may have retry or dump and restore the table to fix this", + "Incompatible key or row definition between the MariaDB .frm file and the information in the storage engine. You can try REPAIR TABLE ... USE_FRM possible followed by ALTER TABLE ... FORCE or dump and restore the table to fix this",
1. typo. "possibly" 2. please, don't recommend USE_FRM, it's a last resort lossy method, designed only for the case when MYI file is lost or corrupted. 3. REPAIR USE_FRM is likey broken for CSV, but I couldn't test it, because REPAIR crashes on CSV tables with or without USE_FRM.
"Too many words in a FTS phrase or proximity search", "Table encrypted but decryption failed. This could be because correct encryption management plugin is not loaded, used encryption key is not available or encryption method does not match.", "Foreign key cascade delete/update exceeds max depth", diff --git a/mysql-test/main/ctype_utf8_def_upgrade.result b/mysql-test/main/ctype_utf8_def_upgrade.result index d30f8670536..e7c21b76058 100644 --- a/mysql-test/main/ctype_utf8_def_upgrade.result +++ b/mysql-test/main/ctype_utf8_def_upgrade.result @@ -10,24 +10,39 @@ SELECT @@character_set_database; latin1 SET @@character_set_database="latin1"; SELECT COUNT(*) FROM t1; -ERROR HY000: Got error 190 "Incompatible key or row definition between the MariaDB .frm file and the information in the storage engine. You may have retry " from storage engine MyISAM +ERROR HY000: Got error 190 "Incompatible key or row definition between the MariaDB .frm file and the information in the storage engine. You can try REPAIR TABLE ... USE_FRM possible followed by ALTER TABLE ... FORCE or dump and restore the table to fix this" from storage engine MyISAM +ALTER TABLE t1 FORCE; +ERROR HY000: Got error 190 "Incompatible key or row definition between the MariaDB .frm file and the information in the storage engine. You can try REPAIR TABLE ... USE_FRM possible followed by ALTER TABLE ... FORCE or dump and restore the table to fix this" from storage engine MyISAM CHECK TABLE t1; Table Op Msg_type Msg_text -test.t1 check Error Got error 190 "Incompatible key or row definition between the MariaDB .frm file and the information in the storage engine. You may have retry " from storage engine MyISAM +test.t1 check Error Got error 190 "Incompatible key or row definition between the MariaDB .frm file and the information in the storage engine. You can try REPAIR TABLE ... USE_FRM possible followed by ALTER TABLE ... FORCE or dump and restore the table to fix this" from storage engine MyISAM test.t1 check error Corrupt REPAIR TABLE t1; Table Op Msg_type Msg_text -test.t1 repair Error Got error 190 "Incompatible key or row definition between the MariaDB .frm file and the information in the storage engine. You may have retry " from storage engine MyISAM +test.t1 repair Error Got error 190 "Incompatible key or row definition between the MariaDB .frm file and the information in the storage engine. You can try REPAIR TABLE ... USE_FRM possible followed by ALTER TABLE ... FORCE or dump and restore the table to fix this" from storage engine MyISAM +test.t1 repair error Corrupt +REPAIR TABLE t1 FORCE; +Table Op Msg_type Msg_text +test.t1 repair Warning Can't open table test.t1 repair error Corrupt REPAIR TABLE t1 USE_FRM; Table Op Msg_type Msg_text -test.t1 repair status OK +test.t1 repair note Table data recovered +t1 repair error Table rebuild required. Please do "ALTER TABLE `test.t1` FORCE" or dump/reload to fix it!
better to avoid exclamation marks in the error messages!
+test.t1 repair status Operation failed SELECT COUNT(*) FROM t1; COUNT(*) 0 CHECK TABLE t1; Table Op Msg_type Msg_text +test.t1 check error Table rebuild required. Please do "ALTER TABLE `t1` FORCE" or dump/reload to fix it! +ALTER TABLE t1 FORCE; +CHECK TABLE t1; +Table Op Msg_type Msg_text test.t1 check status OK +SELECT COUNT(*) FROM t1; +COUNT(*) +0 SHOW CREATE TABLE t1; Table Create Table t1 CREATE TABLE `t1` ( diff --git a/sql/handler.h b/sql/handler.h index fe272d841d9..f429cdacc6f 100644 --- a/sql/handler.h +++ b/sql/handler.h @@ -2249,6 +2267,8 @@ struct Table_scope_and_contents_source_pod_st // For trivial members enum_stats_auto_recalc stats_auto_recalc; bool varchar; ///< 1 if table has a VARCHAR bool sequence; // If SEQUENCE=1 was used + /* True if we are using OPTIMIZE TABLE, REPAIR TABLE or ALTER TABLE FORCE */
it'd be good to clarify here /* this is used to suppress certain warnings that are only used for CREATE/ALTER TABLE */
+ bool recreate_identical_table;
List
*check_constraint_list; diff --git a/sql/sql_admin.cc b/sql/sql_admin.cc index fcbd8a55c15..d4eabd7c34a 100644 --- a/sql/sql_admin.cc +++ b/sql/sql_admin.cc @@ -48,7 +48,8 @@ const LEX_CSTRING msg_optimize= { STRING_WITH_LEN("optimize") }; /* Prepare, run and cleanup for mysql_recreate_table() */
static bool admin_recreate_table(THD *thd, TABLE_LIST *table_list, - Recreate_info *recreate_info) + Recreate_info *recreate_info, + bool partition_admin, bool table_copy)
you don't need to pass partition_admin as an argument, if it's always thd->lex->alter_info.partition_flags & ALTER_PARTITION_ADMIN
{ bool result_code; DBUG_ENTER("admin_recreate_table"); @@ -859,20 +867,29 @@ static bool mysql_admin_table(THD* thd, TABLE_LIST* tables, /* purecov: end */ }
- if (operator_func == &handler::ha_repair && - !(check_opt->sql_flags & TT_USEFRM)) + if (operator_func == &handler::ha_repair) { handler *file= table->table->file; int check_old_types= file->check_old_types(); int check_for_upgrade= file->ha_check_for_upgrade(check_opt);
why check_old_types is not part of ha_check_for_upgrade? they're always used together, as far as I can see.
+ require_data_conversion= + (check_old_types == HA_ADMIN_NEEDS_DATA_CONVERSION || + check_for_upgrade == HA_ADMIN_NEEDS_DATA_CONVERSION); + require_alter_table= + (check_old_types == HA_ADMIN_NEEDS_ALTER || + check_for_upgrade == HA_ADMIN_NEEDS_ALTER);
- if (check_old_types == HA_ADMIN_NEEDS_ALTER || - check_for_upgrade == HA_ADMIN_NEEDS_ALTER) + if (!(check_opt->sql_flags & (TT_USEFRM | TT_FORCE))) + { + if (require_data_conversion || require_alter_table) { /* We use extra_open_options to be able to open crashed tables */ thd->open_options|= extra_open_options; - result_code= admin_recreate_table(thd, table, &recreate_info) ? + result_code= admin_recreate_table(thd, table, &recreate_info, + (lex->alter_info.partition_flags & + ALTER_PARTITION_ADMIN), 1) ? HA_ADMIN_FAILED : HA_ADMIN_OK; + recreate_used= 1; thd->open_options&= ~extra_open_options; goto send_result; } if (check_old_types || check_for_upgrade) { /* If repair is not implemented for the engine, run ALTER TABLE */ need_repair_or_alter= 1; } } - + } result_code= compl_result_code= HA_ADMIN_OK;
if (operator_func == &handler::ha_analyze) @@ -1229,7 +1277,10 @@ static bool mysql_admin_table(THD* thd, TABLE_LIST* tables, *save_next_global= table->next_global; table->next_local= table->next_global= 0;
- result_code= admin_recreate_table(thd, table, &recreate_info); + result_code= admin_recreate_table(thd, table, &recreate_info, + (lex->alter_info.partition_flags & + ALTER_PARTITION_ADMIN), 0); + recreate_used= 1;
that's a bit too late, you check recreate_used earlier
trans_commit_stmt(thd); trans_commit(thd); close_thread_tables(thd); @@ -1641,7 +1696,8 @@ bool Sql_cmd_repair_table::execute(THD *thd) WSREP_TO_ISOLATION_BEGIN_WRTCHK(NULL, NULL, first_table); res= mysql_admin_table(thd, first_table, &m_lex->check_opt, &msg_repair, TL_WRITE, 1, - MY_TEST(m_lex->check_opt.sql_flags & TT_USEFRM), + MY_TEST(m_lex->check_opt.sql_flags & + (TT_USEFRM | TT_FORCE)),
better rename repair_table_use_frm argument of mysql_admin_table and open_only_one_table, because it's not only TT_USEFRM anymore. a good name could be no_errors_from_open
HA_OPEN_FOR_REPAIR, &prepare_for_repair, &handler::ha_repair, &view_repair, true);
diff --git a/sql/sql_table.cc b/sql/sql_table.cc index 55319a2242f..7d5a968070a 100644 --- a/sql/sql_table.cc +++ b/sql/sql_table.cc @@ -10868,19 +10869,56 @@ do_continue:; } #endif /* WITH_WSREP */
+ /* + ALTER TABLE ... ENGINE to the same engine is a common way to + request table rebuild. Set ALTER_RECREATE flag to force table + rebuild. + */ + if (new_db_type == old_db_type) + { + if (create_info->used_fields & HA_CREATE_USED_ENGINE) + alter_info->flags|= ALTER_RECREATE; + + /* + Check if we are using ALTER TABLE FORCE without any other options + (except ENGINE == current_engine). + In this case we will try to recreate an identical to the original. + This code is also used with REPAIR and OPTIMIZE. + */ + if (alter_info->flags == ALTER_RECREATE && + ((create_info->used_fields & ~HA_CREATE_USED_ENGINE) == 0)) + create_info->recreate_identical_table= 1; + } + /* We can use only copy algorithm if one of the following is true: + - If the table is from an old MariaDB version and requires data + modifications. In this case we ignore --alter-algorithm as
eh, no, you cannot ignore it, if the user specified incompatible set of options, ALTER TABLE has to fail with an error, saying ALGORITHM=INPLACE (for example) is impossible here. The server cannot just silently ignore an explicit user command.
+ as we cannot use any other algorithm than COPY (using other + algorithms could open up the problem that the table .frm version + is updated without data transformations and the table would be + corrupted without any way for MariaDB to notice this during + check/upgrade). + This logic ensurses that ALTER TABLE ... FORCE (no other + options) will always be be able to repair a table structure and + convert data from any old format. - In-place is impossible for given operation. - Changes to partitioning which were not handled by fast_alter_part_table() needs to be handled using table copying algorithm unless the engine supports auto-partitioning as such engines can do some changes using in-place API. */ + check_opt.init(); + + require_copy_algorithm= (table->file->ha_check_for_upgrade(&check_opt) == + HA_ADMIN_NEEDS_DATA_CONVERSION); + if (require_copy_algorithm || - if (is_inplace_alter_impossible(table, create_info, alter_info) - || IF_PARTITIONING((partition_changed && + is_inplace_alter_impossible(table, create_info, alter_info) || + IF_PARTITIONING((partition_changed && !(old_db_type->partition_flags() & HA_USE_AUTO_PARTITION)), 0)) { - if (alter_info->algorithm_is_nocopy(thd)) + if (alter_info->algorithm_is_nocopy(thd) && + !(require_copy_algorithm && alter_info->algorithm_not_specified())) { my_error(ER_ALTER_OPERATION_NOT_SUPPORTED, MYF(0), alter_info->algorithm_clause(thd), "ALGORITHM=COPY"); @@ -12451,10 +12482,14 @@ bool mysql_recreate_table(THD *thd, TABLE_LIST *table_list, create_info.init(); create_info.row_type=ROW_TYPE_NOT_USED; create_info.alter_info= &alter_info; + create_info.recreate_identical_table= 1; /* Force alter table to recreate table */ alter_info.flags= (ALTER_CHANGE_COLUMN | ALTER_RECREATE); + alter_info.partition_flags= thd->lex->alter_info.partition_flags; + if (partition_admin) + alter_info.partition_flags= ALTER_PARTITION_ADMIN;
when is this needed? I couldn't find a single code path where alter_info.partition_flags is not ALTER_PARTITION_ADMIN at this point. Neither code analysys nor mtr have found anything. You can safely remove this if() and add DBUG_ASSERT(alter_info.partition_flags & ALTER_PARTITION_ADMIN);
- if (table_copy) + if (table_copy && !partition_admin) alter_info.set_requested_algorithm( Alter_info::ALTER_TABLE_ALGORITHM_COPY);
diff --git a/sql/table.cc b/sql/table.cc index f84eeda297f..b09e9b11f17 100644 --- a/sql/table.cc +++ b/sql/table.cc @@ -1846,6 +1846,8 @@ int TABLE_SHARE::init_from_binary_frm_image(THD *thd, bool write, */ if (share->frm_version == FRM_VER_TRUE_VARCHAR -1 && frm_image[33] == 5) share->frm_version= FRM_VER_TRUE_VARCHAR; + if (share->frm_version <= FRM_VER_TRUE_VARCHAR) + share->keep_original_mysql_version= 1;
this is wrong. see prepare_frm_header(), frm_version is *always* FRM_VER_TRUE_VARCHAR if there are no vcols. you've basically just disabled update_frm_version() for almost everyone.
new_field_pack_flag= frm_image[27]; new_frm_ver= (frm_image[2] - FRM_VER);
Regards, Sergei Chief Architect, MariaDB Server and security@mariadb.org
![](https://secure.gravatar.com/avatar/68cce0b6e372c2c66f4d66c6356102af.jpg?s=120&d=mm&r=g)
Hi! <cut> '
So, I'd suggest the following logic instead:
* if USE_FRM - do USE_FRM stuff (or an warning) * if marked corrupted or EXTENDED or doesn't need upgrade - run repair * if repair isn't supported = ALTER IGNORE unless needs upgrade * if needs upgrade - run upgrade = ALTER IGNORE COPY * if fails because of corruption - go to step 2 if repair wasn't run yet
I agree that a simplified repair logic is better than what we currently have. However, I would prefer the following order if things for REPAIR if USE_FRM - do USE_FRM stuff (or an warning) * if needs upgrade - run upgrade = ALTER IGNORE COPY. If fails run 'repair' if engine supports it. * if marked corrupted or FORCE or doesn't need upgrade - run repair * if repair isn't supported = ALTER IGNORE unless needs upgrade There are a few reasons for this: - Easier to document and understand - 'need upgrade' only happens during mariadb-upgrade, which is the first thing as user does when upgrading. In this case the table should not be crashed (if it is crashed, it should be repaired on the old version). - With most engines, we cannot know if a table is corrupted (especially during upgrade) , so it hard to both code and document under which conditions the repair would be run. - REPAIR EXTENDED is a bad way to force REPAIR as EXTENDED has well documented meaning. Most users will not want to run REPAIR EXTENDED as it can take 100x longer than just REPAIR (as EXTENDED use less memory (good) and does not use sorting to create indexes (safer but bad performance). The disadvantage in my suggestion is that if the table is both needing upgrade and corrupted then ALTER TABLE FORCE may not be able to recover as many rows as REPAIR followed by ALTER TABLE FORCE. It is a bit hard to now change the upgrade first tries ALTER TABLE IGNORE and the repair. I would like to have this been part of another MDEV to get the current already improved ALTER/REPAIR code into 11.5
here USE_FRM is handled as before (except that it adds a warning if USE_FRM isn't supported by the engine).
I agree with the warning.
After that the server runs engine's repair if the table is marked corrupted or if EXTENDED was used (so no need for a new FORCE syntax). See above why EXTENDED is a bad option for this.
There is no error "engine does not support REPAIR", the server falls back to ALTER automatically in that case.
Agree. This is also what Elena has been suggesting and this is what I had planned to do next on the repair task.
If the data need to be upgraded and ALTER fails, the server tries repair and then ALTER again - just like MyISAM and Aria try different repair methods.
Agree, but this is a bit tricky to get right. I suggest we leave this part to another MDEV to be done later.
Also, note that ALTER should always use IGNORE to delete duplicates in unique after collation of hash changes - that's what repair in MyISAM/Aria does.
Good point. We had not thought about this one. For REPAIR converted to AALER it is ok to run "ALTER TABLE FORCE IGNORE"
This commit is about improving repair of tables
now as there is an MDEV, it should be here in the subject line
It is already there in bb-11.4-timestamp <cut>
Other things: - FORCE argument added to REPAIR to allow one to first run internal repair to fix damaged blocks and then follow it with ALTER TABLE. - REPAIR will not update frm_version if ha_check_for_upgrade() finds that table is still incompatible with current version. In this case the REPAIR will end with an error.
doesn't look like "the REPAIR will end with an error"
It should. It is handled in the code: /* Give a warning if REPAIR TABLE was used but table still needs an ALTER TABLE. This can only happen for old type tables where REPAIR was using FORCE to recover old data. */ if (operator_func == &handler::ha_repair && ! recreate_used && (require_data_conversion || require_alter_table)) { There is even a test for this: ctype_utf8_def_upgrade.result: REPAIR TABLE t1 USE_FRM; Table Op Msg_type Msg_text -test.t1 repair status OK +test.t1 repair note Table data recovered +t1 repair error Table rebuild required. Please do "ALTER TABLE `test.t1` FORCE" or dump/reload to fix it! +test.t1 repair status Operation failed <cut>
diff --git a/include/my_handler_errors.h b/include/my_handler_errors.h index df414888907..06dde5b3f74 100644
<cut>
+ "Incompatible key or row definition between the MariaDB .frm file and the information in the storage engine. You can try REPAIR TABLE ... USE_FRM possible followed by ALTER TABLE ... FORCE or dump and restore the table to fix this",
1. typo. "possibly"
2. please, don't recommend USE_FRM, it's a last resort lossy method, designed only for the case when MYI file is lost or corrupted.
This error ONLY is given in the case where there is a mismatch between the .frm and the table definition. There is NO other way to recover any data without using USE_FRM or dump & restore. The USE_FRM is designed to work for any engine that has data and index in different files and the data file format is compatible with MariaDB. In practice it is as safe or error prone as dump and restore.
3. REPAIR USE_FRM is likey broken for CSV, but I couldn't test it, because REPAIR crashes on CSV tables with or without USE_FRM.
csv.test includes test for check and repair table. No crashes for me. USE_FRM did not work, but that was a bug in CSV where it listed the file extensions in the wrong order. I fixed this and now USE_FRM also works for CSV. <cut>
--- a/mysql-test/main/ctype_utf8_def_upgrade.result +++ b/mysql-test/main/ctype_utf8_def_upgrade.result @@ -10,24 +10,39 @@ SELECT @@character_set_database; latin1 SET @@character_set_database="latin1"; SELECT COUNT(*) FROM t1; -ERROR HY000: Got error 190 "Incompatible key or row definition between the MariaDB .frm file and the information in the storage engine. You may have retry " from storage engine MyISAM +ERROR HY000: Got error 190 "Incompatible key or row definition between the MariaDB .frm file and the information in the storage engine. You can try REPAIR TABLE ... USE_FRM possible followed by ALTER TABLE ... FORCE or dump and restore the table to fix this" from storage engine MyISAM +ALTER TABLE t1 FORCE; +ERROR HY000: Got error 190 "Incompatible key or row definition between the MariaDB .frm file and the information in the storage engine. You can try REPAIR TABLE ... USE_FRM possible followed by ALTER TABLE ... FORCE or dump and restore the table to fix this" from storage engine MyISAM CHECK TABLE t1; Table Op Msg_type Msg_text -test.t1 check Error Got error 190 "Incompatible key or row definition between the MariaDB .frm file and the information in the storage engine. You may have retry " from storage engine MyISAM +test.t1 check Error Got error 190 "Incompatible key or row definition between the MariaDB .frm file and the information in the storage engine. You can try REPAIR TABLE ... USE_FRM possible followed by ALTER TABLE ... FORCE or dump and restore the table to fix this" from storage engine MyISAM test.t1 check error Corrupt REPAIR TABLE t1; Table Op Msg_type Msg_text -test.t1 repair Error Got error 190 "Incompatible key or row definition between the MariaDB .frm file and the information in the storage engine. You may have retry " from storage engine MyISAM +test.t1 repair Error Got error 190 "Incompatible key or row definition between the MariaDB .frm file and the information in the storage engine. You can try REPAIR TABLE ... USE_FRM possible followed by ALTER TABLE ... FORCE or dump and restore the table to fix this" from storage engine MyISAM +test.t1 repair error Corrupt +REPAIR TABLE t1 FORCE; +Table Op Msg_type Msg_text +test.t1 repair Warning Can't open table test.t1 repair error Corrupt REPAIR TABLE t1 USE_FRM; Table Op Msg_type Msg_text -test.t1 repair status OK +test.t1 repair note Table data recovered +t1 repair error Table rebuild required. Please do "ALTER TABLE `test.t1` FORCE" or dump/reload to fix it!
better to avoid exclamation marks in the error messages!
This is an old error. Better not change it. (And in this specific case I think that ! is a good thing to use) Note that this error is the one that you thought we didn't have. <cut>
+++ b/sql/handler.h @@ -2249,6 +2267,8 @@ struct Table_scope_and_contents_source_pod_st // For trivial members enum_stats_auto_recalc stats_auto_recalc; bool varchar; ///< 1 if table has a VARCHAR bool sequence; // If SEQUENCE=1 was used + /* True if we are using OPTIMIZE TABLE, REPAIR TABLE or ALTER TABLE FORCE */
it'd be good to clarify here
/* this is used to suppress certain warnings that are only used for CREATE/ALTER TABLE */
The above is not what the flag does. Before this flag InnoDB could try, 'invisible for the user', change the data storage format in ALTER TABLE which could lead to: - ALTER TABLE FAILING - Loose of InnoDB symlinks - Change of file formats. The flag is there to tell InnoDB that it is not allowed to do that! I updated the comment to: True if we are using OPTIMIZE TABLE, REPAIR TABLE or ALTER TABLE FORCE in which case the 'new' table should have identical storage layout as the original.
+ bool recreate_identical_table;
+++ b/sql/sql_admin.cc @@ -48,7 +48,8 @@ const LEX_CSTRING msg_optimize= { STRING_WITH_LEN("optimize") }; /* Prepare, run and cleanup for mysql_recreate_table() */
static bool admin_recreate_table(THD *thd, TABLE_LIST *table_list, - Recreate_info *recreate_info) + Recreate_info *recreate_info, + bool partition_admin, bool table_copy)
you don't need to pass partition_admin as an argument, if it's always thd->lex->alter_info.partition_flags & ALTER_PARTITION_ADMIN
I did not want to have this and the calling function dependent on LEX. I also added the argument to make it clear that recreate needs and will use the flag (and no other flags in thd->lex->alter_info.partition_flags). I could remove it if you insist, but I think it makes the function call a bit clear.
{ bool result_code; DBUG_ENTER("admin_recreate_table"); @@ -859,20 +867,29 @@ static bool mysql_admin_table(THD* thd, TABLE_LIST* tables, /* purecov: end */ }
- if (operator_func == &handler::ha_repair && - !(check_opt->sql_flags & TT_USEFRM)) + if (operator_func == &handler::ha_repair) { handler *file= table->table->file; int check_old_types= file->check_old_types(); int check_for_upgrade= file->ha_check_for_upgrade(check_opt);
why check_old_types is not part of ha_check_for_upgrade? they're always used together, as far as I can see.
Yes, they are always used together. Old code, didn't want to do too many changes in this patch. In any case, I have now moved call to check_old_types() to ha_check_for_upgrade() and removed it from all other places. <cut>
@@ -1229,7 +1277,10 @@ static bool mysql_admin_table(THD* thd, TABLE_LIST* tables, *save_next_global= table->next_global; table->next_local= table->next_global= 0;
- result_code= admin_recreate_table(thd, table, &recreate_info); + result_code= admin_recreate_table(thd, table, &recreate_info, + (lex->alter_info.partition_flags & + ALTER_PARTITION_ADMIN), 0); + recreate_used= 1;
that's a bit too late, you check recreate_used earlier
The flag is to tell future code that 'admin_recreate_table()' has been called (see one line above). We checked earlier if an earlier admin_recreate_table() had been called (it that would have happened, we would never have come to this part of the code). We come to this code when we tried REPAIR and got the error TRY_ALTER, in which we do recreate and set the flag.
@@ -1641,7 +1696,8 @@ bool Sql_cmd_repair_table::execute(THD *thd) WSREP_TO_ISOLATION_BEGIN_WRTCHK(NULL, NULL, first_table); res= mysql_admin_table(thd, first_table, &m_lex->check_opt, &msg_repair, TL_WRITE, 1, - MY_TEST(m_lex->check_opt.sql_flags & TT_USEFRM), + MY_TEST(m_lex->check_opt.sql_flags & + (TT_USEFRM | TT_FORCE)),
better rename repair_table_use_frm argument of mysql_admin_table and open_only_one_table, because it's not only TT_USEFRM anymore. a good name could be no_errors_from_open
Done.
--- a/sql/sql_table.cc +++ b/sql/sql_table.cc @@ -10868,19 +10869,56 @@ do_continue:; } #endif /* WITH_WSREP */
+ /* + ALTER TABLE ... ENGINE to the same engine is a common way to + request table rebuild. Set ALTER_RECREATE flag to force table + rebuild. + */ + if (new_db_type == old_db_type) + { + if (create_info->used_fields & HA_CREATE_USED_ENGINE) + alter_info->flags|= ALTER_RECREATE; + + /* + Check if we are using ALTER TABLE FORCE without any other options + (except ENGINE == current_engine). + In this case we will try to recreate an identical to the original. + This code is also used with REPAIR and OPTIMIZE. + */ + if (alter_info->flags == ALTER_RECREATE && + ((create_info->used_fields & ~HA_CREATE_USED_ENGINE) == 0)) + create_info->recreate_identical_table= 1; + } + /* We can use only copy algorithm if one of the following is true: + - If the table is from an old MariaDB version and requires data + modifications. In this case we ignore --alter-algorithm as
eh, no, you cannot ignore it, if the user specified incompatible set of options, ALTER TABLE has to fail with an error, saying ALGORITHM=INPLACE (for example) is impossible here. The server cannot just silently ignore an explicit user command.
Yes, we can and what we must do! ALTER TABLE FORCE has a very special, documented meaning (it should always be able to recreate the table). If it would honor the alter_algorithm variable, you would get: - Wrong data in table on master or slave. - ALTER TABLE FORCE stop with error (if alter_algorithm is not copy). In case of ALTER TABLE FORCE, the user variable 'alter-algorithm' MUST be ignored for thing to work smoothly The reason for this is that mariadb-upgrade uses (and has always used) ALTER TABLE FORCE for upgrading tables and assume this will NEVER give an error. The flag also causes slaves to possible stop or have bad data if ALTER TABLE FORCE is used together with any value for --alter-algorithm other than copy. If the user would use --alter-algorithm=inplace, in a config file, upgrades would not work. I rather remove the flag --alter-algorithm than the above behavior.
@@ -12451,10 +12482,14 @@ bool mysql_recreate_table(THD *thd, TABLE_LIST *table_list, create_info.init(); create_info.row_type=ROW_TYPE_NOT_USED; create_info.alter_info= &alter_info; + create_info.recreate_identical_table= 1; /* Force alter table to recreate table */ alter_info.flags= (ALTER_CHANGE_COLUMN | ALTER_RECREATE); + alter_info.partition_flags= thd->lex->alter_info.partition_flags; + if (partition_admin) + alter_info.partition_flags= ALTER_PARTITION_ADMIN;
when is this needed? I couldn't find a single code path where alter_info.partition_flags is not ALTER_PARTITION_ADMIN at this point. Neither code analysys nor mtr have found anything. You can safely remove this if() and add
alter_info is a local variable and does not have ALTER_PARTION_ADMIN SET.
DBUG_ASSERT(alter_info.partition_flags & ALTER_PARTITION_ADMIN);
This would always be false and abort.
+++ b/sql/table.cc @@ -1846,6 +1846,8 @@ int TABLE_SHARE::init_from_binary_frm_image(THD *thd, bool write, */ if (share->frm_version == FRM_VER_TRUE_VARCHAR -1 && frm_image[33] == 5) share->frm_version= FRM_VER_TRUE_VARCHAR; + if (share->frm_version <= FRM_VER_TRUE_VARCHAR) + share->keep_original_mysql_version= 1;
this is wrong. see prepare_frm_header(), frm_version is *always* FRM_VER_TRUE_VARCHAR if there are no vcols.
you've basically just disabled update_frm_version() for almost everyone.
Good catch. I changed it to: if (share->frm_version < FRM_VER_TRUE_VARCHAR) This makes me even more worried about this code: new_frm_ver= (frm_image[2] - FRM_VER); field_pack_length= new_frm_ver < 2 ? 11 : 17; FRM_VER_TRUE_VARCHAR= 10 FRM_VER=6 This means that in FRM_VERSION 8 we changed something that made things incompatible. I don't understand why we are not using frm_image[2] <= FRM_CHANGE_OF_PACK_LENGTH. which would be much more clear than the above code. To make things even more unclear, we are using new_frm_ver in the rest of the code. like (new_frm_ver >= 3) It is not clear what this mean. Would be much better to compared the stored frm_version variable directly with the FRM_VER variables. Regards, Monty
![](https://secure.gravatar.com/avatar/39b623a1559cf9c69ac3d9d4fb44e7fe.jpg?s=120&d=mm&r=g)
Hi, Michael, On Mar 01, Michael Widenius wrote:
Hi!
<cut> '
So, I'd suggest the following logic instead:
* if USE_FRM - do USE_FRM stuff (or an warning) * if marked corrupted or EXTENDED or doesn't need upgrade - run repair * if repair isn't supported = ALTER IGNORE unless needs upgrade * if needs upgrade - run upgrade = ALTER IGNORE COPY * if fails because of corruption - go to step 2 if repair wasn't run yet
I agree that a simplified repair logic is better than what we currently have. However, I would prefer the following order if things for REPAIR
* if USE_FRM - do USE_FRM stuff (or an warning) * if needs upgrade - run upgrade = ALTER IGNORE COPY. * If fails run 'repair' if engine supports it. * if marked corrupted or FORCE or doesn't need upgrade - run repair * if repair isn't supported = ALTER IGNORE unless needs upgrade
There are a few reasons for this: - Easier to document and understand - 'need upgrade' only happens during mariadb-upgrade, which is the first thing as user does when upgrading. In this case the table should not be crashed (if it is crashed, it should be repaired on the old version). - With most engines, we cannot know if a table is corrupted (especially during upgrade) , so it hard to both code and document under which conditions the repair would be run. - REPAIR EXTENDED is a bad way to force REPAIR as EXTENDED has well documented meaning. Most users will not want to run REPAIR EXTENDED as it can take 100x longer than just REPAIR (as EXTENDED use less memory (good) and does not use sorting to create indexes (safer but bad performance).
I don't understand. I agree that your suggestion is easier to document and understand, as it's shorter. But that's it. If the table is not crashed or the engine doesn't know if the table is crashed, then both suggestions are exactly the same, because the second step in mine will be skipped. As for EXTENDED - consider MyISAM logic: * QUICK, repair by sort, don't touch the data file * EXTENDED, copy row by row, build indexes row by row * neither is used - try repair by sort, if it fails do EXTENDED So, basically, EXTENDED is very slow (yes, 100x in my benchmarks) and it generally shouldn't be used, because REPAIR falls back to it automatically. EXTENDED forces the slowest and safest repair method. So I thought it's fine if it'll force REPAIR of an non-corrupted table on upgrade. I cannot imagine a use case when anyone would need to use it anyway.
The disadvantage in my suggestion is that if the table is both needing upgrade and corrupted then ALTER TABLE FORCE may not be able to recover as many rows as REPAIR followed by ALTER TABLE FORCE.
I just think the server can do what MyISAM is already doing for 20+ years - try faster repair and if it fails, try a slower one.
It is a bit hard to now change the upgrade first tries ALTER TABLE IGNORE and the repair. I would like to have this been part of another MDEV to get the current already improved ALTER/REPAIR code into 11.5
May be, admittedly I didn't try to see how difficult it would be to do what I was suggesting :) I can take a look.
Other things: - FORCE argument added to REPAIR to allow one to first run internal repair to fix damaged blocks and then follow it with ALTER TABLE. - REPAIR will not update frm_version if ha_check_for_upgrade() finds that table is still incompatible with current version. In this case the REPAIR will end with an error.
doesn't look like "the REPAIR will end with an error"
It should. It is handled in the code:
/* Give a warning if REPAIR TABLE was used but table still needs an ALTER TABLE. This can only happen for old type tables where REPAIR was using FORCE to recover old data. */ if (operator_func == &handler::ha_repair && ! recreate_used && (require_data_conversion || require_alter_table)) {
Ouch. This shows how convoluted the code is. handler::ha_repair() does simply if (result == HA_ADMIN_OK && table->file->ha_check_for_upgrade(check_opt) == HA_ADMIN_OK) result= update_frm_version(table); return result; so, no warning and the same return code, whether frm version was updated or not. And the caller repeats the check independently and fails with error. It would've been simpler if ha_repair() would fail with an error here or at least printed a warning.
+ "Incompatible key or row definition between the MariaDB .frm file and the information in the storage engine. You can try REPAIR TABLE ... USE_FRM possible followed by ALTER TABLE ... FORCE or dump and restore the table to fix this",
1. typo. "possibly"
2. please, don't recommend USE_FRM, it's a last resort lossy method, designed only for the case when MYI file is lost or corrupted.
This error ONLY is given in the case where there is a mismatch between the .frm and the table definition. There is NO other way to recover any data without using USE_FRM or dump & restore. The USE_FRM is designed to work for any engine that has data and index in different files and the data file format is compatible with MariaDB. In practice it is as safe or error prone as dump and restore.
well, if there's a mismatch between .frm and the actual table definition, then USE_FRM will most likely simply destroy the table, because MYD file will not match the frm. Only if the table definition matches the frm, but MYI header is corrupted - that's when USE_FRM will help with certainty. And yes, USE_FRM can technically work for other engines, but practically it's only Aria and MyISAM, I've looked though all engines to see if any other would support it.
+++ b/sql/handler.h @@ -2249,6 +2267,8 @@ struct Table_scope_and_contents_source_pod_st // For trivial members enum_stats_auto_recalc stats_auto_recalc; bool varchar; ///< 1 if table has a VARCHAR bool sequence; // If SEQUENCE=1 was used + /* True if we are using OPTIMIZE TABLE, REPAIR TABLE or ALTER TABLE FORCE */
it'd be good to clarify here
/* this is used to suppress certain warnings that are only used for CREATE/ALTER TABLE */
The above is not what the flag does. Before this flag InnoDB could try, 'invisible for the user', change the data storage format in ALTER TABLE which could lead to: - ALTER TABLE FAILING - Loose of InnoDB symlinks - Change of file formats.
The flag is there to tell InnoDB that it is not allowed to do that!
I updated the comment to:
True if we are using OPTIMIZE TABLE, REPAIR TABLE or ALTER TABLE FORCE in which case the 'new' table should have identical storage layout as the original.
This isn't any better, because all it says was already perfectly clear from the old comment. This new version is just longer. But it seems I've missed one place in InnoDB where recreate_identical_table indeed affects something in the table, so you're right, it's not only warnings.
+ bool recreate_identical_table;
+++ b/sql/sql_admin.cc @@ -48,7 +48,8 @@ const LEX_CSTRING msg_optimize= { STRING_WITH_LEN("optimize") }; /* Prepare, run and cleanup for mysql_recreate_table() */
static bool admin_recreate_table(THD *thd, TABLE_LIST *table_list, - Recreate_info *recreate_info) + Recreate_info *recreate_info, + bool partition_admin, bool table_copy)
you don't need to pass partition_admin as an argument, if it's always thd->lex->alter_info.partition_flags & ALTER_PARTITION_ADMIN
I did not want to have this and the calling function dependent on LEX. I also added the argument to make it clear that recreate needs and will use the flag (and no other flags in thd->lex->alter_info.partition_flags).
I could remove it if you insist, but I think it makes the function call a bit clear.
I am not insisting. But the function is getting a THD anyway, so it has access to LEX and alter_info.partition_flags. If at some point later it'll need to use another flag from there, I'm pretty sure whoever will be doing it won't add a new argument to admin_recreate_table(), but will simply check thd->lex->alter_info.partition_flags, as it's already available. So I don't think your new argument accomplished anything or made anything clearer. At least, not to me, for me it's just confusing, that is, the opposite.
{ bool result_code; DBUG_ENTER("admin_recreate_table"); @@ -859,20 +867,29 @@ static bool mysql_admin_table(THD* thd, TABLE_LIST* tables, /* purecov: end */ }
- if (operator_func == &handler::ha_repair && - !(check_opt->sql_flags & TT_USEFRM)) + if (operator_func == &handler::ha_repair) { handler *file= table->table->file; int check_old_types= file->check_old_types(); int check_for_upgrade= file->ha_check_for_upgrade(check_opt);
why check_old_types is not part of ha_check_for_upgrade? they're always used together, as far as I can see.
Yes, they are always used together. Old code, didn't want to do too many changes in this patch. In any case, I have now moved call to check_old_types() to ha_check_for_upgrade() and removed it from all other places.
I asked you on slack not to do that and to ignore this my review comment. Because after I wrote that review, check_old_types was moved into ha_check_for_upgrade in a recent commit to 10.11. Now you've created a merge conflict to resolve. You can revert this your change and let be merged from 10.11. Otherwise it'll be removed later in a merge.
<cut>
@@ -1229,7 +1277,10 @@ static bool mysql_admin_table(THD* thd, TABLE_LIST* tables, *save_next_global= table->next_global; table->next_local= table->next_global= 0;
- result_code= admin_recreate_table(thd, table, &recreate_info); + result_code= admin_recreate_table(thd, table, &recreate_info, + (lex->alter_info.partition_flags & + ALTER_PARTITION_ADMIN), 0); + recreate_used= 1;
that's a bit too late, you check recreate_used earlier
The flag is to tell future code that 'admin_recreate_table()' has been called (see one line above).
Oh, the "future code". Okay.
--- a/sql/sql_table.cc +++ b/sql/sql_table.cc @@ -10868,19 +10869,56 @@ do_continue:; } #endif /* WITH_WSREP */
+ /* + ALTER TABLE ... ENGINE to the same engine is a common way to + request table rebuild. Set ALTER_RECREATE flag to force table + rebuild. + */ + if (new_db_type == old_db_type) + { + if (create_info->used_fields & HA_CREATE_USED_ENGINE) + alter_info->flags|= ALTER_RECREATE; + + /* + Check if we are using ALTER TABLE FORCE without any other options + (except ENGINE == current_engine). + In this case we will try to recreate an identical to the original. + This code is also used with REPAIR and OPTIMIZE. + */ + if (alter_info->flags == ALTER_RECREATE && + ((create_info->used_fields & ~HA_CREATE_USED_ENGINE) == 0)) + create_info->recreate_identical_table= 1; + } + /* We can use only copy algorithm if one of the following is true: + - If the table is from an old MariaDB version and requires data + modifications. In this case we ignore --alter-algorithm as
eh, no, you cannot ignore it, if the user specified incompatible set of options, ALTER TABLE has to fail with an error, saying ALGORITHM=INPLACE (for example) is impossible here. The server cannot just silently ignore an explicit user command.
Yes, we can and what we must do! ALTER TABLE FORCE has a very special, documented meaning (it should always be able to recreate the table). If it would honor the alter_algorithm variable, you would get: - Wrong data in table on master or slave. - ALTER TABLE FORCE stop with error (if alter_algorithm is not copy).
well, alter_algorithm also has a documented meaning. and ALTER TABLE also has a documented behavior - it errors out if it cannot do the ALTER with the specified algorithm or lock level.
The reason for this is that mariadb-upgrade uses (and has always used) ALTER TABLE FORCE for upgrading tables and assume this will NEVER give an error.
I don't see any reason why mariadb-upgrade cannot specify the algorithm explicitly or set its own alter_algorithm.
If the user would use --alter-algorithm=inplace, in a config file, upgrades would not work.
They will, if mariadb-upgrade won't rely on specific defaults or magic "ignore" behavior, but will use complete self-contained commands.
@@ -12451,10 +12482,14 @@ bool mysql_recreate_table(THD *thd, TABLE_LIST *table_list, create_info.init(); create_info.row_type=ROW_TYPE_NOT_USED; create_info.alter_info= &alter_info; + create_info.recreate_identical_table= 1; /* Force alter table to recreate table */ alter_info.flags= (ALTER_CHANGE_COLUMN | ALTER_RECREATE); + alter_info.partition_flags= thd->lex->alter_info.partition_flags; + if (partition_admin) + alter_info.partition_flags= ALTER_PARTITION_ADMIN;
when is this needed? I couldn't find a single code path where alter_info.partition_flags is not ALTER_PARTITION_ADMIN at this point. Neither code analysys nor mtr have found anything. You can safely remove this if() and add
alter_info is a local variable and does not have ALTER_PARTION_ADMIN SET.
You missed that the line before the if() is alter_info.partition_flags= thd->lex->alter_info.partition_flags and thd->lex->alter_info.partition_flags always has ALTER_PARTION_ADMIN at this point.
DBUG_ASSERT(alter_info.partition_flags & ALTER_PARTITION_ADMIN);
This would always be false and abort.
I believe I've added that assert and run the full test suite before writing this suggestion into a review (almost sure - I did add few asserts and run the full test suite, but not 100% sure this was one of them)
+++ b/sql/table.cc @@ -1846,6 +1846,8 @@ int TABLE_SHARE::init_from_binary_frm_image(THD *thd, bool write, */ if (share->frm_version == FRM_VER_TRUE_VARCHAR -1 && frm_image[33] == 5) share->frm_version= FRM_VER_TRUE_VARCHAR; + if (share->frm_version <= FRM_VER_TRUE_VARCHAR) + share->keep_original_mysql_version= 1;
this is wrong. see prepare_frm_header(), frm_version is *always* FRM_VER_TRUE_VARCHAR if there are no vcols.
you've basically just disabled update_frm_version() for almost everyone.
Good catch. I changed it to: if (share->frm_version < FRM_VER_TRUE_VARCHAR)
This makes me even more worried about this code: new_frm_ver= (frm_image[2] - FRM_VER); field_pack_length= new_frm_ver < 2 ? 11 : 17;
FRM_VER_TRUE_VARCHAR= 10 FRM_VER=6
This means that in FRM_VERSION 8 we changed something that made things incompatible.
Why?
I don't understand why we are not using frm_image[2] <= FRM_CHANGE_OF_PACK_LENGTH. which would be much more clear than the above code. To make things even more unclear, we are using new_frm_ver in the rest of the code. like (new_frm_ver >= 3) It is not clear what this mean. Would be much better to compared the stored frm_version variable directly with the FRM_VER variables.
Agree, but it's all trivial to fix by creating FRM_VER_this and FRM_VER_that Regards, Sergei Chief Architect, MariaDB Server and security@mariadb.org
![](https://secure.gravatar.com/avatar/68cce0b6e372c2c66f4d66c6356102af.jpg?s=120&d=mm&r=g)
Hi!
On Mon, Mar 4, 2024 at 3:26 PM Sergei Golubchik
Hi, Michael,
On Mar 01, Michael Widenius wrote:
Hi!
<cut> '
So, I'd suggest the following logic instead:
* if USE_FRM - do USE_FRM stuff (or an warning) * if marked corrupted or EXTENDED or doesn't need upgrade - run repair * if repair isn't supported = ALTER IGNORE unless needs upgrade * if needs upgrade - run upgrade = ALTER IGNORE COPY * if fails because of corruption - go to step 2 if repair wasn't run yet
I agree that a simplified repair logic is better than what we currently have. However, I would prefer the following order if things for REPAIR
* if USE_FRM - do USE_FRM stuff (or an warning) * if needs upgrade - run upgrade = ALTER IGNORE COPY. * If fails run 'repair' if engine supports it. * if marked corrupted or FORCE or doesn't need upgrade - run repair * if repair isn't supported = ALTER IGNORE unless needs upgrade
<cut>
I don't understand. I agree that your suggestion is easier to document and understand, as it's shorter. But that's it.
The reason for my suggestion is that it makes 'mariadb-upgrade' easier to understand. - It will run alter table if table needs to be upgraded. This will only fail if the table is corrupted and there is no data loss. - REPAIR should be seen as last resort to get back 'some of the data'. I prefer to suggest a non-loss solution before a lossy one.
If the table is not crashed or the engine doesn't know if the table is crashed, then both suggestions are exactly the same, because the second step in mine will be skipped.
For most engines we don't know if a table is crashed at the time of mariadb-upgrade.
As for EXTENDED - consider MyISAM logic: * QUICK, repair by sort, don't touch the data file * EXTENDED, copy row by row, build indexes row by row * neither is used - try repair by sort, if it fails do EXTENDED
So, basically, EXTENDED is very slow (yes, 100x in my benchmarks) and it generally shouldn't be used, because REPAIR falls back to it automatically. EXTENDED forces the slowest and safest repair method.
So I thought it's fine if it'll force REPAIR of an non-corrupted table on upgrade. I cannot imagine a use case when anyone would need to use it anyway.
This does not answer my reply that we cannot use EXTENDED to FORCE REPAIR if the table needs to be upgraded first. It not good to use an existing keyword for something else.
The disadvantage in my suggestion is that if the table is both needing upgrade and corrupted then ALTER TABLE FORCE may not be able to recover as many rows as REPAIR followed by ALTER TABLE FORCE.
I just think the server can do what MyISAM is already doing for 20+ years - try faster repair and if it fails, try a slower one.
It is a bit hard to now change the upgrade first tries ALTER TABLE IGNORE and the repair. I would like to have this been part of another MDEV to get the current already improved ALTER/REPAIR code into 11.5
May be, admittedly I didn't try to see how difficult it would be to do what I was suggesting :) I can take a look.
Feel free to try, but in a separate MDEV please. <cut>
doesn't look like "the REPAIR will end with an error"
It should. It is handled in the code:
/* Give a warning if REPAIR TABLE was used but table still needs an ALTER TABLE. This can only happen for old type tables where REPAIR was using FORCE to recover old data. */ if (operator_func == &handler::ha_repair && ! recreate_used && (require_data_conversion || require_alter_table)) {
Ouch. This shows how convoluted the code is. handler::ha_repair() does simply
if (result == HA_ADMIN_OK && table->file->ha_check_for_upgrade(check_opt) == HA_ADMIN_OK) result= update_frm_version(table); return result;
so, no warning and the same return code, whether frm version was updated or not. And the caller repeats the check independently and fails with error. It would've been simpler if ha_repair() would fail with an error here or at least printed a warning.
It cannot fail with an error. That is the whole point. REPAIR, CHECK and OPTIMIZE returns a result set of notes, warnings and errors. There can be many of these during any of the above operations. This is why ha_repair() cannot give a normal error. Instead all upper level sql errors are generated in one place, in mysql_admin_table(). The caller will get an error, but not in ha_repair, but in the function that called ha_repair().
This error ONLY is given in the case where there is a mismatch between the .frm and the table definition. There is NO other way to recover any data without using USE_FRM or dump & restore. The USE_FRM is designed to work for any engine that has data and index in different files and the data file format is compatible with MariaDB. In practice it is as safe or error prone as dump and restore.
well, if there's a mismatch between .frm and the actual table definition, then USE_FRM will most likely simply destroy the table, because MYD file will not match the frm.
USE_FRM will not touch the data file, so it is safer than other options.
Only if the table definition matches the frm, but MYI header is corrupted - that's when USE_FRM will help with certainty.
And yes, USE_FRM can technically work for other engines, but practically it's only Aria and MyISAM, I've looked though all engines to see if any other would support it.
CSV now supports it. Any engine that has a definition and data file can trivially support it. (If not already supported).
+++ b/sql/sql_admin.cc @@ -48,7 +48,8 @@ const LEX_CSTRING msg_optimize= { STRING_WITH_LEN("optimize") }; /* Prepare, run and cleanup for mysql_recreate_table() */
static bool admin_recreate_table(THD *thd, TABLE_LIST *table_list, - Recreate_info *recreate_info) + Recreate_info *recreate_info, + bool partition_admin, bool table_copy)
you don't need to pass partition_admin as an argument, if it's always thd->lex->alter_info.partition_flags & ALTER_PARTITION_ADMIN
I did not want to have this and the calling function dependent on LEX. I also added the argument to make it clear that recreate needs and will use the flag (and no other flags in thd->lex->alter_info.partition_flags).
I could remove it if you insist, but I think it makes the function call a bit clear.
I am not insisting. But the function is getting a THD anyway, so it has access to LEX and alter_info.partition_flags. If at some point later it'll need to use another flag from there, I'm pretty sure whoever will be doing it won't add a new argument to admin_recreate_table(), but will simply check thd->lex->alter_info.partition_flags, as it's already available. So I don't think your new argument accomplished anything or made anything clearer. At least, not to me, for me it's just confusing, that is, the opposite.
Originally the 'partition_admin' flag was different for different functions. In the last patch it is indeed always thd->lex->alter_info.partition_flags & ALTER_PARTITION_ADMIN ok, I will remove it.
{ bool result_code; DBUG_ENTER("admin_recreate_table"); @@ -859,20 +867,29 @@ static bool mysql_admin_table(THD* thd, TABLE_LIST* tables, /* purecov: end */ }
- if (operator_func == &handler::ha_repair && - !(check_opt->sql_flags & TT_USEFRM)) + if (operator_func == &handler::ha_repair) { handler *file= table->table->file; int check_old_types= file->check_old_types(); int check_for_upgrade= file->ha_check_for_upgrade(check_opt);
why check_old_types is not part of ha_check_for_upgrade? they're always used together, as far as I can see.
Yes, they are always used together. Old code, didn't want to do too many changes in this patch. In any case, I have now moved call to check_old_types() to ha_check_for_upgrade() and removed it from all other places.
I asked you on slack not to do that and to ignore this my review comment. Because after I wrote that review, check_old_types was moved into ha_check_for_upgrade in a recent commit to 10.11.
Sorry, missed that. I have now reverted that part. <cut>
--- a/sql/sql_table.cc +++ b/sql/sql_table.cc @@ -10868,19 +10869,56 @@ do_continue:; } #endif /* WITH_WSREP */
+ /* + ALTER TABLE ... ENGINE to the same engine is a common way to + request table rebuild. Set ALTER_RECREATE flag to force table + rebuild. + */ + if (new_db_type == old_db_type) + { + if (create_info->used_fields & HA_CREATE_USED_ENGINE) + alter_info->flags|= ALTER_RECREATE; + + /* + Check if we are using ALTER TABLE FORCE without any other options + (except ENGINE == current_engine). + In this case we will try to recreate an identical to the original. + This code is also used with REPAIR and OPTIMIZE. + */ + if (alter_info->flags == ALTER_RECREATE && + ((create_info->used_fields & ~HA_CREATE_USED_ENGINE) == 0)) + create_info->recreate_identical_table= 1; + } + /* We can use only copy algorithm if one of the following is true: + - If the table is from an old MariaDB version and requires data + modifications. In this case we ignore --alter-algorithm as
eh, no, you cannot ignore it, if the user specified incompatible set of options, ALTER TABLE has to fail with an error, saying ALGORITHM=INPLACE (for example) is impossible here. The server cannot just silently ignore an explicit user command.
Yes, we can and what we must do! ALTER TABLE FORCE has a very special, documented meaning (it should always be able to recreate the table). If it would honor the alter_algorithm variable, you would get: - Wrong data in table on master or slave. - ALTER TABLE FORCE stop with error (if alter_algorithm is not copy).
well, alter_algorithm also has a documented meaning. and ALTER TABLE also has a documented behavior - it errors out if it cannot do the ALTER with the specified algorithm or lock level.
The reason for this is that mariadb-upgrade uses (and has always used) ALTER TABLE FORCE for upgrading tables and assume this will NEVER give an error.
I don't see any reason why mariadb-upgrade cannot specify the algorithm explicitly or set its own alter_algorithm.
It could, but that would be major logic change. It would also make it much harder for anyone wanting to upgrade their tables to latest frm version and data compatibility as they would have to do: SET OPTION @@innodb_strict_mode=0 FOR ALTER TABLE table_name FORCE --alter-algorithm=copy Which is a mouthful and not something we should ask user to remember or use. Currently we just document that one has to do: ALTER TABLE FORCE to do a rebuild. This does however not work in some cases without my changes.
If the user would use --alter-algorithm=inplace, in a config file, upgrades would not work.
They will, if mariadb-upgrade won't rely on specific defaults or magic "ignore" behavior, but will use complete self-contained commands.
mariadb-upgrade has been using ALTER TABLE FORCE for upgrades since 5.5.
@@ -12451,10 +12482,14 @@ bool mysql_recreate_table(THD *thd, TABLE_LIST *table_list, create_info.init(); create_info.row_type=ROW_TYPE_NOT_USED; create_info.alter_info= &alter_info; + create_info.recreate_identical_table= 1; /* Force alter table to recreate table */ alter_info.flags= (ALTER_CHANGE_COLUMN | ALTER_RECREATE); + alter_info.partition_flags= thd->lex->alter_info.partition_flags; + if (partition_admin) + alter_info.partition_flags= ALTER_PARTITION_ADMIN;
when is this needed? I couldn't find a single code path where alter_info.partition_flags is not ALTER_PARTITION_ADMIN at this point. Neither code analysys nor mtr have found anything. You can safely remove this if() and add
alter_info is a local variable and does not have ALTER_PARTION_ADMIN SET.
You missed that the line before the if() is
alter_info.partition_flags= thd->lex->alter_info.partition_flags
and thd->lex->alter_info.partition_flags always has ALTER_PARTION_ADMIN at this point.
DBUG_ASSERT(alter_info.partition_flags & ALTER_PARTITION_ADMIN);
This would always be false and abort.
I believe I've added that assert and run the full test suite before writing this suggestion into a review (almost sure - I did add few asserts and run the full test suite, but not 100% sure this was one of them)
Sorry, looks like we talked of different things. You probably talked about the line if (partition_admin) While I thought you did not need to do: alter_info.partition_flags= thd->lex->alter_info.partition_flags; It was especially confusing when you said: "I couldn't find a single code path where alter_info.partition_flags is not ALTER_PARTITION_ADMIN at this point." I now understand you meant "I couldn't find a single code path where alter_info.partition_flags is not ALTER_PARTITION_ADMIN at this point WHEN parition_admin is set"
+++ b/sql/table.cc @@ -1846,6 +1846,8 @@ int TABLE_SHARE::init_from_binary_frm_image(THD *thd, bool write, */ if (share->frm_version == FRM_VER_TRUE_VARCHAR -1 && frm_image[33] == 5) share->frm_version= FRM_VER_TRUE_VARCHAR; + if (share->frm_version <= FRM_VER_TRUE_VARCHAR) + share->keep_original_mysql_version= 1;
this is wrong. see prepare_frm_header(), frm_version is *always* FRM_VER_TRUE_VARCHAR if there are no vcols.
you've basically just disabled update_frm_version() for almost everyone.
Good catch. I changed it to: if (share->frm_version < FRM_VER_TRUE_VARCHAR)
This makes me even more worried about this code: new_frm_ver= (frm_image[2] - FRM_VER); field_pack_length= new_frm_ver < 2 ? 11 : 17;
FRM_VER_TRUE_VARCHAR= 10 FRM_VER=6
This means that in FRM_VERSION 8 we changed something that made things incompatible.
Why?
Not sure why we did things incompatible. From the code it is clear that things are incompatible at version 8 as pack_length is different.
I don't understand why we are not using frm_image[2] <= FRM_CHANGE_OF_PACK_LENGTH. which would be much more clear than the above code. To make things even more unclear, we are using new_frm_ver in the rest of the code. like (new_frm_ver >= 3) It is not clear what this mean. Would be much better to compared the stored frm_version variable directly with the FRM_VER variables.
Agree, but it's all trivial to fix by creating FRM_VER_this and FRM_VER_that
Yes, and we should.
Regards, Sergei Chief Architect, MariaDB Server and security@mariadb.org
participants (2)
-
Michael Widenius
-
Sergei Golubchik