maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

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