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 <monty(a)mariadb.org>
> 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<Virtual_column_info> *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(a)mariadb.org
