Hi! On Mon, Mar 4, 2024 at 3:26 PM Sergei Golubchik <serg@mariadb.org> wrote:
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