maria-developers team mailing list archive

[Alexander Barkov <bar@xxxxxxxxxxx>]

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@xxxxxxxxxxx