Re: [Maria-developers] 06ce67c644b: MDEV-27653 long uniques don't work with unicode collations
Hi, Alexander, On Jan 15, Alexander Barkov wrote:
revision-id: 06ce67c644b (mariadb-10.4.27-41-g06ce67c644b) parent(s): 6cb84346e1b author: Alexander Barkov committer: Alexander Barkov timestamp: 2023-01-10 18:27:16 +0400 message:
MDEV-27653 long uniques don't work with unicode collations
diff --git a/sql/sql_admin.cc b/sql/sql_admin.cc index 213d77f8237..5dd19c877af 100644 --- a/sql/sql_admin.cc +++ b/sql/sql_admin.cc @@ -772,11 +772,14 @@ static bool mysql_admin_table(THD* thd, TABLE_LIST* tables, int check_for_upgrade= file->ha_check_for_upgrade(check_opt);
if (check_old_types == HA_ADMIN_NEEDS_ALTER || - check_for_upgrade == HA_ADMIN_NEEDS_ALTER) + check_old_types == HA_ADMIN_NEEDS_UPGRADE || + check_for_upgrade == HA_ADMIN_NEEDS_ALTER || + check_for_upgrade == HA_ADMIN_NEEDS_UPGRADE)
eh. So old code was returning HA_ADMIN_NEEDS_ALTER actually quite intentionally. REPAIR TABLE was automatically switching to ALTER if these checks were returning HA_ADMIN_NEEDS_ALTER. So it didn't matter if the message confusingly said "Please use REPAIR", because it would've been ALTER internally anyway. Meaning, I suspect, that the old code was fine and you didn't need to change `case HA_ADMIN_NEEDS_ALTER` and didn't need to replace `return HA_ADMIN_NEEDS_UPGRADE` with HA_ADMIN_NEEDS_ALTER. All you needed to do was to return HA_ADMIN_NEEDS_ALTER from the check_long_hash_compatibility() method and the rest would've likely worked automatically. Or at least with much smaller changes.
{ /* 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); + result_code= admin_recreate_table(thd, table) ? HA_ADMIN_FAILED : + HA_ADMIN_OK;
good catch
thd->open_options&= ~extra_open_options; goto send_result; }
Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
Hello Sergei, On 1/16/23 1:58 AM, Sergei Golubchik wrote:
Hi, Alexander,
On Jan 15, Alexander Barkov wrote:
revision-id: 06ce67c644b (mariadb-10.4.27-41-g06ce67c644b) parent(s): 6cb84346e1b author: Alexander Barkov committer: Alexander Barkov timestamp: 2023-01-10 18:27:16 +0400 message:
MDEV-27653 long uniques don't work with unicode collations
diff --git a/sql/sql_admin.cc b/sql/sql_admin.cc index 213d77f8237..5dd19c877af 100644 --- a/sql/sql_admin.cc +++ b/sql/sql_admin.cc @@ -772,11 +772,14 @@ static bool mysql_admin_table(THD* thd, TABLE_LIST* tables, int check_for_upgrade= file->ha_check_for_upgrade(check_opt);
if (check_old_types == HA_ADMIN_NEEDS_ALTER || - check_for_upgrade == HA_ADMIN_NEEDS_ALTER) + check_old_types == HA_ADMIN_NEEDS_UPGRADE || + check_for_upgrade == HA_ADMIN_NEEDS_ALTER || + check_for_upgrade == HA_ADMIN_NEEDS_UPGRADE)
eh. So old code was returning HA_ADMIN_NEEDS_ALTER actually quite intentionally.
REPAIR TABLE was automatically switching to ALTER if these checks were returning HA_ADMIN_NEEDS_ALTER.
So it didn't matter if the message confusingly said "Please use REPAIR", because it would've been ALTER internally anyway.
Meaning, I suspect, that the old code was fine and you didn't need to change `case HA_ADMIN_NEEDS_ALTER` and didn't need to replace `return HA_ADMIN_NEEDS_UPGRADE` with HA_ADMIN_NEEDS_ALTER.
All you needed to do was to return HA_ADMIN_NEEDS_ALTER from the check_long_hash_compatibility() method and the rest would've likely worked automatically. Or at least with much smaller changes.
REPAIR will not really work because of duplicate keys: MariaDB [test]> repair table mdev27653_100422_text; +----------------------------+--------+----------+----------------------------------+ | Table | Op | Msg_type | Msg_text | +----------------------------+--------+----------+----------------------------------+ | test.mdev27653_100422_text | repair | Error | Duplicate entry 'ä' for key 'a' | | test.mdev27653_100422_text | repair | status | Operation failed | +----------------------------+--------+----------+----------------------------------+ 2 rows in set (0.010 sec) ALTER FORCE (without IGNORE) won't work either: MariaDB [test]> alter table test.mdev27653_100422_text force; ERROR 1062 (23000): Duplicate entry 'ä' for key 'a' One needs to do ALTER IGNORE FORCE to fix this. MariaDB [test]> alter ignore table test.mdev27653_100422_text force; Query OK, 2 rows affected (0.003 sec) Records: 2 Duplicates: 1 Warnings: 0 It would be totally confusing to print this error: Please do "REPAIR TABLE `t1`". This error message is much better: Please do "ALTER TABLE `t1` FORCE" It does not say about IGNORE, but at least it suggests the correct verb statement. The above changes are needed to suggest ALTER (rather than REPAIR) in the error message text. A 100% clear error message would be something like this: Table rebuild required. Please do "ALTER TABLE `t1` FORCE" or dump/reload to fix it! If it returns duplicate key errors, get rid of duplicates and re-run ALTER again. Or do "ALTER IGNORE TABLE `t1` FORCE" if you want duplicates to be removed automatically (note, some data can be lost). Should we add a new message like this?
{ /* 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); + result_code= admin_recreate_table(thd, table) ? HA_ADMIN_FAILED : + HA_ADMIN_OK;
good catch
thd->open_options&= ~extra_open_options; goto send_result; }
Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
participants (2)
-
Alexander Barkov -
Sergei Golubchik