[Maria-developers] [MDEV-7362] ANALYZE TABLES crash with table-independent-statistics gathering
Hi Sergei! I've implemented a fix for the bug described in MDEV-7362, that also fixes MDEV-7380. The test case also covers MDEV-7380. With the following fix,the I've spent a bit of time trying to figure out the architecture of things there. I am not confident my fix is complete, or if this is the way we want to go about fixing it. The fix does what we discussed over IRC, that we are not interested in computing statistics for a FULLTEXT index. What I think would be the root cause of the problem is calling ha_index_init() on an index that does not support this. By calling table->file->ha_index_init(), the table->field array gets corrupted and that ultimately leads to a crash in the function using it afterwards. Then again, this module is completely new to me, so any comments are appreciated. Here is the diff of the first changes. diff --git a/mysql-test/t/mdev-7362.test b/mysql-test/t/mdev-7362.test new file mode 100644 index 0000000..6551893 --- /dev/null +++ b/mysql-test/t/mdev-7362.test @@ -0,0 +1,14 @@ +CREATE TABLE t1 (a longtext, FULLTEXT KEY (`a`)) ENGINE=MyISAM; +insert into t1 values (unhex('3E0D0A4141414142334E7A6143317963324541414141424977414141674541726D')); +analyze table t1 persistent for all; +drop table t1; + +CREATE TABLE t1 (a longtext, FULLTEXT KEY (`a`)) ENGINE=MyISAM; +insert into t1 values (unhex('3E0D0A4141414142334E7A6143317963324541414141424977414141674541')); +analyze table t1 persistent for all; +drop table t1; + +CREATE TABLE t1 (f longtext NOT NULL, FULLTEXT KEY f (f)) ENGINE=MyISAM; +INSERT INTO t1 VALUES (REPEAT('a',27)),('foo'); +ANALYZE TABLE t1 PERSISTENT FOR ALL; +drop table t1; diff --git a/sql/sql_statistics.cc b/sql/sql_statistics.cc index d368145..2c1cccf 100644 --- a/sql/sql_statistics.cc +++ b/sql/sql_statistics.cc @@ -2367,6 +2367,9 @@ int collect_statistics_for_index(THD *thd, TABLE *table, uint index) DBUG_RETURN(rc); } + if (key_info->flags & HA_FULLTEXT) // skip if FULLTEXT index + DBUG_RETURN(rc); + table->key_read= 1; table->file->extra(HA_EXTRA_KEYREAD); Regards, Vicențiu
I've implemented a fix for the bug described in MDEV-7362, that also fixes MDEV-7380. The test case also covers MDEV-7380. With the following fix, *there are no longer any crashes within the server.*
* Pressed send by mistake.
----- Original Message -----
Hi Sergei!
Hi Vicentiu, Thanks for fixing my reported bug :-)
I've implemented a fix for the bug described in MDEV-7362, that also fixes MDEV-7380. The test case also covers MDEV-7380.
I've spent a bit of time trying to figure out the architecture of things there. I am not confident my fix is complete, or if this is the way we want to go about fixing it. The fix does what we discussed over IRC, that we are not interested in computing statistics for a FULLTEXT index.
makes sense.
What I think would be the root cause of the problem is calling ha_index_init() on an index that does not support this. By calling table->file->ha_index_init(), the table->field array gets corrupted and that ultimately leads to a crash in the function using it afterwards.
That sounds like another bug that need to be fixed in ha_maria::index_init / ha_myisam::index_init or the API is being used differently to usual.
Then again, this module is completely new to me, so any comments are appreciated. Here is the diff of the first changes.
The test case should also look at the result of the: select * from mysql.index_stats where index_name='a' and table_name='t1';
diff --git a/mysql-test/t/mdev-7362.test b/mysql-test/t/mdev-7362.test new file mode 100644 index 0000000..6551893 --- /dev/null +++ b/mysql-test/t/mdev-7362.test
more recent test cases include the area of the test failure in the filename or include the test with the general t/statistics.test
@@ -0,0 +1,14 @@ +CREATE TABLE t1 (a longtext, FULLTEXT KEY (`a`)) ENGINE=MyISAM; +insert into t1 values (unhex('3E0D0A4141414142334E7A6143317963324541414141424977414141674541726D')); +analyze table t1 persistent for all; +drop table t1; + +CREATE TABLE t1 (a longtext, FULLTEXT KEY (`a`)) ENGINE=MyISAM; +insert into t1 values (unhex('3E0D0A4141414142334E7A6143317963324541414141424977414141674541')); +analyze table t1 persistent for all; +drop table t1; + +CREATE TABLE t1 (f longtext NOT NULL, FULLTEXT KEY f (f)) ENGINE=MyISAM; +INSERT INTO t1 VALUES (REPEAT('a',27)),('foo'); +ANALYZE TABLE t1 PERSISTENT FOR ALL; +drop table t1;
Can you please add a test case for innodb too as it supports FT in 10.0.
diff --git a/sql/sql_statistics.cc b/sql/sql_statistics.cc index d368145..2c1cccf 100644 --- a/sql/sql_statistics.cc +++ b/sql/sql_statistics.cc @@ -2367,6 +2367,9 @@ int collect_statistics_for_index(THD *thd, TABLE *table, uint index) DBUG_RETURN(rc); } + if (key_info->flags & HA_FULLTEXT) // skip if FULLTEXT index + DBUG_RETURN(rc); +
If this is the fix the if..return can go much earlier in collect_statistics_for_index, before Index_prefix_calc() even. -- -- Daniel Black, Engineer @ Open Query (http://openquery.com.au) Remote expertise & maintenance for MySQL/MariaDB server environments.
Hi Daniel!
The test case should also look at the result of the:
select * from mysql.index_stats where index_name='a' and table_name='t1';
Added in the test case. Also, I got the explanation slightly wrong. It's not index_init that causes the problem, it's calling ha_index_first with a garbage buffer as a parameter that messes things up. The flags in table specifically say that (in that case) the table->record[0] is garbage, but we used it anyway.
diff --git a/mysql-test/t/mdev-7362.test b/mysql-test/t/mdev-7362.test new file mode 100644 index 0000000..6551893 --- /dev/null +++ b/mysql-test/t/mdev-7362.test
more recent test cases include the area of the test failure in the filename or include the test with the general t/statistics.test
Hmm, I made a new file to test for this bug and named it statistics_index_crash.test. I don't think it fits going into the statistics.test.
@@ -0,0 +1,14 @@ +CREATE TABLE t1 (a longtext, FULLTEXT KEY (`a`)) ENGINE=MyISAM; +insert into t1 values (unhex('3E0D0A4141414142334E7A61433179633245414141414249774141416745 41726D')); +analyze table t1 persistent for all; +drop table t1; + +CREATE TABLE t1 (a longtext, FULLTEXT KEY (`a`)) ENGINE=MyISAM; +insert into t1 values (unhex('3E0D0A4141414142334E7A61433179633245414141414249774141416745 41')); +analyze table t1 persistent for all; +drop table t1; + +CREATE TABLE t1 (f longtext NOT NULL, FULLTEXT KEY f (f)) ENGINE=MyISAM; +INSERT INTO t1 VALUES (REPEAT('a',27)),('foo'); +ANALYZE TABLE t1 PERSISTENT FOR ALL; +drop table t1;
Can you please add a test case for innodb too as it supports FT in 10.0.
Done.
diff --git a/sql/sql_statistics.cc b/sql/sql_statistics.cc index d368145..2c1cccf 100644 --- a/sql/sql_statistics.cc +++ b/sql/sql_statistics.cc @@ -2367,6 +2367,9 @@ int collect_statistics_for_index(THD *thd, TABLE *table, uint index) DBUG_RETURN(rc); } + if (key_info->flags & HA_FULLTEXT) // skip if FULLTEXT index + DBUG_RETURN(rc); +
If this is the fix the if..return can go much earlier in collect_statistics_for_index, before Index_prefix_calc() even.
Done. Regards, Vicențiu
----- Original Message -----
Hi Daniel!
The test case should also look at the result of the:
select * from mysql.index_stats where index_name='a' and table_name='t1';
Added in the test case.
Thanks
Also, I got the explanation slightly wrong. It's not index_init that causes the problem, it's calling ha_index_first with a garbage buffer as a parameter that messes things up. The flags in table specifically say that (in that case) the table->record[0] is garbage, but we used it anyway.
Good to know there isn't a lower level lurking bug because of this. Thanks for the explanation.
diff --git a/mysql-test/t/mdev-7362.test b/mysql-test/t/mdev-7362.test new file mode 100644 index 0000000..6551893 --- /dev/null +++ b/mysql-test/t/mdev-7362.test
more recent test cases include the area of the test failure in the filename or include the test with the general t/statistics.test
Hmm, I made a new file to test for this bug and named it statistics_index_crash.test. I don't think it fits going into the statistics.test.
Good description. like it.
Can you please add a test case for innodb too as it supports FT in 10.0.
Done.
thanks
If this is the fix the if..return can go much earlier in collect_statistics_for_index, before Index_prefix_calc() even.
Done.
thanks -- -- Daniel Black, Engineer @ Open Query (http://openquery.com.au) Remote expertise & maintenance for MySQL/MariaDB server environments.
Hi, Vicențiu! On Jan 15, Vicențiu Ciorbaru wrote:
Hi Sergei!
I've implemented a fix for the bug described in MDEV-7362, that also fixes MDEV-7380. The test case also covers MDEV-7380. With the following fix,the
good
I've spent a bit of time trying to figure out the architecture of things there. I am not confident my fix is complete, or if this is the way we want to go about fixing it. The fix does what we discussed over IRC, that we are not interested in computing statistics for a FULLTEXT index.
right
What I think would be the root cause of the problem is calling ha_index_init() on an index that does not support this. By calling table->file->ha_index_init(), the table->field array gets corrupted and that ultimately leads to a crash in the function using it afterwards.
right
diff --git a/mysql-test/t/mdev-7362.test b/mysql-test/t/mdev-7362.test new file mode 100644 index 0000000..6551893 --- /dev/null +++ b/mysql-test/t/mdev-7362.test @@ -0,0 +1,14 @@ +CREATE TABLE t1 (a longtext, FULLTEXT KEY (`a`)) ENGINE=MyISAM; +insert into t1 values (unhex('3E0D0A4141414142334E7A6143317963324541414141424977414141674541726D')); +analyze table t1 persistent for all; +drop table t1; + +CREATE TABLE t1 (a longtext, FULLTEXT KEY (`a`)) ENGINE=MyISAM; +insert into t1 values (unhex('3E0D0A4141414142334E7A6143317963324541414141424977414141674541')); +analyze table t1 persistent for all; +drop table t1; + +CREATE TABLE t1 (f longtext NOT NULL, FULLTEXT KEY f (f)) ENGINE=MyISAM; +INSERT INTO t1 VALUES (REPEAT('a',27)),('foo'); +ANALYZE TABLE t1 PERSISTENT FOR ALL; +drop table t1;
1. Why three tests? 2. You forgot to add tests for GIS indexes. https://mariadb.com/kb/en/spatial-index/
diff --git a/sql/sql_statistics.cc b/sql/sql_statistics.cc index d368145..2c1cccf 100644 --- a/sql/sql_statistics.cc +++ b/sql/sql_statistics.cc @@ -2367,6 +2367,9 @@ int collect_statistics_for_index(THD *thd, TABLE *table, uint index) DBUG_RETURN(rc); }
+ if (key_info->flags & HA_FULLTEXT) // skip if FULLTEXT index + DBUG_RETURN(rc); +
I agree that this could've been done earlier. The constructor for Index_prefix_calc is pretty heavy, best to skip it when it's not needed.
table->key_read= 1; table->file->extra(HA_EXTRA_KEYREAD);
Regards, Sergei
participants (3)
-
Daniel Black -
Sergei Golubchik -
Vicențiu Ciorbaru