- developers - lists.mariadb.org

Re: [Maria-developers] [Commits] 1ed2d8b98ad: MDEV-17032: Estimates are higher for partitions of a table with @@use_stat_tables= PREFERABLY
by Sergey Petrunia 25 Nov '18

25 Nov '18

Hi Varun, On Fri, Nov 16, 2018 at 07:41:07PM +0530, Varun wrote: > revision-id: 1ed2d8b98ade099fe23b7d5c00d23364388e15aa (mariadb-10.0.36-80-g1ed2d8b98ad) > parent(s): a84d87fde8c0bc325c8e00f06ea02bcd84a75d55 > author: Varun Gupta > committer: Varun Gupta > timestamp: 2018-11-16 19:40:47 +0530 > message: > > MDEV-17032: Estimates are higher for partitions of a table with @@use_stat_tables= PREFERABLY > > The problem here is EITS statistics does not calculate statistics for the partitions of the table. > So a temporary solution would be to not read EITS statistics for partitioned tables. > > Also disabling reading of EITS for columns that participate in the partition list of a table. > > --- > mysql-test/r/partition.result | 88 +++++++++++++++++++++++++++++++++++++++++++ > mysql-test/t/partition.test | 58 ++++++++++++++++++++++++++++ > sql/opt_range.cc | 12 +++++- > sql/partition_info.cc | 26 +++++++++++++ > sql/partition_info.h | 1 + > sql/sql_statistics.cc | 16 ++++++++ > 6 files changed, 199 insertions(+), 2 deletions(-) > > diff --git a/mysql-test/r/partition.result b/mysql-test/r/partition.result > index c6669176b3d..aedf9f89f0e 100644 > --- a/mysql-test/r/partition.result > +++ b/mysql-test/r/partition.result > @@ -2645,3 +2645,91 @@ Warnings: > Note 1517 Duplicate partition name p2 > DEALLOCATE PREPARE stmt; > DROP TABLE t1; > +# > +# MDEV-17032: Estimates are higher for partitions of a table with @@use_stat_tables= PREFERABLY > +# > +create table t0(a int); > +insert into t0 values (0),(1),(2),(3),(4),(5),(6),(7),(8),(9); > +create table t1(a int); > +insert into t1 select A.a + B.a* 10 + C.a * 100 from t0 A, t0 B, t0 C; > +create table t2 ( > +part_key int, > +a int, > +b int > +) partition by list(part_key) ( > +partition p0 values in (0), > +partition p1 values in (1), > +partition p2 values in (2), > +partition p3 values in (3), > +partition p4 values in (4) > +); > +insert into t2 > +select mod(a,5), a/100, a from t1; > +set @save_use_stat_tables= @@use_stat_tables; > +set @save_optimizer_use_condition_selectivity=@@optimizer_use_condition_selectivity; > +# > +# Tests using stats provided by the storage engine > +# > +explain extended select * from t2 where part_key=1; > +id select_type table type possible_keys key key_len ref rows filtered Extra > +1 SIMPLE t2 ALL NULL NULL NULL NULL 200 100.00 Using where > +Warnings: > +Note 1003 select `test`.`t2`.`part_key` AS `part_key`,`test`.`t2`.`a` AS `a`,`test`.`t2`.`b` AS `b` from `test`.`t2` where (`test`.`t2`.`part_key` = 1) > +explain partitions select * from t2 where part_key=1; > +id select_type table partitions type possible_keys key key_len ref rows Extra > +1 SIMPLE t2 p1 ALL NULL NULL NULL NULL 200 Using where > +explain extended select * from t2 where part_key in (1,2); > +id select_type table type possible_keys key key_len ref rows filtered Extra > +1 SIMPLE t2 ALL NULL NULL NULL NULL 400 100.00 Using where > +Warnings: > +Note 1003 select `test`.`t2`.`part_key` AS `part_key`,`test`.`t2`.`a` AS `a`,`test`.`t2`.`b` AS `b` from `test`.`t2` where (`test`.`t2`.`part_key` in (1,2)) > +explain partitions select * from t2 where part_key=1; > +id select_type table partitions type possible_keys key key_len ref rows Extra > +1 SIMPLE t2 p1 ALL NULL NULL NULL NULL 200 Using where > +explain extended select * from t2 where part_key in (1,2); > +id select_type table type possible_keys key key_len ref rows filtered Extra > +1 SIMPLE t2 ALL NULL NULL NULL NULL 400 100.00 Using where > +Warnings: > +Note 1003 select `test`.`t2`.`part_key` AS `part_key`,`test`.`t2`.`a` AS `a`,`test`.`t2`.`b` AS `b` from `test`.`t2` where (`test`.`t2`.`part_key` in (1,2)) > +explain partitions select * from t2 where part_key in (1,2); > +id select_type table partitions type possible_keys key key_len ref rows Extra > +1 SIMPLE t2 p1,p2 ALL NULL NULL NULL NULL 400 Using where > +set @save_histogram_size=@@histogram_size; > +set @@histogram_size=100; > +set @@use_stat_tables= PREFERABLY; > +set @@optimizer_use_condition_selectivity=4; > +analyze table t2; > +Table Op Msg_type Msg_text > +test.t2 analyze status Engine-independent statistics collected > +test.t2 analyze status OK > +# > +# Tests using EITS > +# > +explain extended select * from t2 where part_key=1; > +id select_type table type possible_keys key key_len ref rows filtered Extra > +1 SIMPLE t2 ALL NULL NULL NULL NULL 200 100.00 Using where > +Warnings: > +Note 1003 select `test`.`t2`.`part_key` AS `part_key`,`test`.`t2`.`a` AS `a`,`test`.`t2`.`b` AS `b` from `test`.`t2` where (`test`.`t2`.`part_key` = 1) > +explain partitions select * from t2 where part_key=1; > +id select_type table partitions type possible_keys key key_len ref rows Extra > +1 SIMPLE t2 p1 ALL NULL NULL NULL NULL 200 Using where > +explain extended select * from t2 where part_key in (1,2); > +id select_type table type possible_keys key key_len ref rows filtered Extra > +1 SIMPLE t2 ALL NULL NULL NULL NULL 400 100.00 Using where > +Warnings: > +Note 1003 select `test`.`t2`.`part_key` AS `part_key`,`test`.`t2`.`a` AS `a`,`test`.`t2`.`b` AS `b` from `test`.`t2` where (`test`.`t2`.`part_key` in (1,2)) > +explain partitions select * from t2 where part_key=1; > +id select_type table partitions type possible_keys key key_len ref rows Extra > +1 SIMPLE t2 p1 ALL NULL NULL NULL NULL 200 Using where > +explain extended select * from t2 where part_key in (1,2); > +id select_type table type possible_keys key key_len ref rows filtered Extra > +1 SIMPLE t2 ALL NULL NULL NULL NULL 400 100.00 Using where > +Warnings: > +Note 1003 select `test`.`t2`.`part_key` AS `part_key`,`test`.`t2`.`a` AS `a`,`test`.`t2`.`b` AS `b` from `test`.`t2` where (`test`.`t2`.`part_key` in (1,2)) > +explain partitions select * from t2 where part_key in (1,2); > +id select_type table partitions type possible_keys key key_len ref rows Extra > +1 SIMPLE t2 p1,p2 ALL NULL NULL NULL NULL 400 Using where > +set @@use_stat_tables= @save_use_stat_tables; > +set @@optimizer_use_condition_selectivity= @save_optimizer_use_condition_selectivity; > +set @@histogram_size= @save_histogram_size; > +drop table t0,t1,t2; > diff --git a/mysql-test/t/partition.test b/mysql-test/t/partition.test > index 1c8cd0375d6..00c6f1ce77c 100644 > --- a/mysql-test/t/partition.test > +++ b/mysql-test/t/partition.test > @@ -2897,3 +2897,61 @@ EXECUTE stmt; > DEALLOCATE PREPARE stmt; > DROP TABLE t1; > > +--echo # > +--echo # MDEV-17032: Estimates are higher for partitions of a table with @@use_stat_tables= PREFERABLY > +--echo # > + > +create table t0(a int); > +insert into t0 values (0),(1),(2),(3),(4),(5),(6),(7),(8),(9); > + > +create table t1(a int); > +insert into t1 select A.a + B.a* 10 + C.a * 100 from t0 A, t0 B, t0 C; > + > + > +create table t2 ( > + part_key int, > + a int, > + b int > +) partition by list(part_key) ( > + partition p0 values in (0), > + partition p1 values in (1), > + partition p2 values in (2), > + partition p3 values in (3), > + partition p4 values in (4) > +); > +insert into t2 > +select mod(a,5), a/100, a from t1; > + > +set @save_use_stat_tables= @@use_stat_tables; > +set @save_optimizer_use_condition_selectivity=@@optimizer_use_condition_selectivity; > +--echo # > +--echo # Tests using stats provided by the storage engine > +--echo # > +explain extended select * from t2 where part_key=1; > +explain partitions select * from t2 where part_key=1; > +explain extended select * from t2 where part_key in (1,2); > +explain partitions select * from t2 where part_key=1; ^^ Isn't this query the same like one two lines above? > +explain extended select * from t2 where part_key in (1,2); > +explain partitions select * from t2 where part_key in (1,2); > + > +set @save_histogram_size=@@histogram_size; > +set @@histogram_size=100; > +set @@use_stat_tables= PREFERABLY; > +set @@optimizer_use_condition_selectivity=4; > +analyze table t2; > +--echo # > +--echo # Tests using EITS > +--echo # > +# filtered should be 100 Please change to use '--echo #' so that the above is visible in the .result file as well. > +explain extended select * from t2 where part_key=1; > +explain partitions select * from t2 where part_key=1; > +# filtered should be 100 > +explain extended select * from t2 where part_key in (1,2); > +explain partitions select * from t2 where part_key=1; > +# filtered should be 100 > +explain extended select * from t2 where part_key in (1,2); > +explain partitions select * from t2 where part_key in (1,2); Please add a test that shows that condition selectivity is taken into account for non-partitioned columns. > +set @@use_stat_tables= @save_use_stat_tables; > +set @@optimizer_use_condition_selectivity= @save_optimizer_use_condition_selectivity; > +set @@histogram_size= @save_histogram_size; > +drop table t0,t1,t2; > diff --git a/sql/opt_range.cc b/sql/opt_range.cc > index 3bcaa72e32f..59aa2f3f280 100644 > --- a/sql/opt_range.cc > +++ b/sql/opt_range.cc > @@ -3322,6 +3322,10 @@ bool create_key_parts_for_pseudo_indexes(RANGE_OPT_PARAM *param, > { > Field **field_ptr; > TABLE *table= param->table; > + partition_info *part_info= NULL; > + #ifdef WITH_PARTITION_STORAGE_ENGINE > + part_info= table->part_info; > + #endif > uint parts= 0; > > for (field_ptr= table->field; *field_ptr; field_ptr++) > @@ -3329,7 +3333,9 @@ bool create_key_parts_for_pseudo_indexes(RANGE_OPT_PARAM *param, > Column_statistics* col_stats= (*field_ptr)->read_stats; > if (bitmap_is_set(used_fields, (*field_ptr)->field_index) > && col_stats && !col_stats->no_stat_values_provided() > - && !((*field_ptr)->type() == MYSQL_TYPE_GEOMETRY)) > + && !((*field_ptr)->type() == MYSQL_TYPE_GEOMETRY) > + && (!part_info || > + part_info->field_in_partition_expr(*field_ptr))) > parts++; > } > > @@ -3350,7 +3356,9 @@ bool create_key_parts_for_pseudo_indexes(RANGE_OPT_PARAM *param, > if (bitmap_is_set(used_fields, (*field_ptr)->field_index)) > { > Field *field= *field_ptr; > - if (field->type() == MYSQL_TYPE_GEOMETRY) > + if (field->type() == MYSQL_TYPE_GEOMETRY > + || (!part_info || > + part_info->field_in_partition_expr(field))) > continue; This condition is wrong. (field->type() == MYSQL_TYPE_GEOMETRY || (!part_info || part_info->field_in_partition_expr(field))) is the same as (field->type() == MYSQL_TYPE_GEOMETRY || !part_info || ... which means if there is no partitioning, we won't count the selectivity. Indeed, if I apply this patch, all EITS tests start to fail, selectivity is no longer counted: --- /home/psergey/dev-git/10.1/mysql-test/r/selectivity_innodb.result 2018-11-16 14:06:27.231202907 +0300 +++ /home/psergey/dev-git/10.1/mysql-test/r/selectivity_innodb.reject 2018-11-25 01:03:14.661647893 +0300 @@ -28,13 +28,13 @@ explain extended select * from t1 where a is null; id select_type table type possible_keys key key_len ref rows filtered Extra -1 SIMPLE t1 ALL NULL NULL NULL NULL 10 40.00 Using where +1 SIMPLE t1 ALL NULL NULL NULL NULL 10 100.00 Using where Warnings: Note 1003 select `test`.`t1`.`a` AS `a` from `test`.`t1` where isnull(`test`.`t1`.`a`) explain extended select * from t1 where a is not null; id select_type table type possible_keys key key_len ref rows filtered Extra -1 SIMPLE t1 ALL NULL NULL NULL NULL 10 60.00 Using where +1 SIMPLE t1 ALL NULL NULL NULL NULL 10 100.00 Using where Warnings: See also the comment below about the return value of field_in_partition_expr. > > uint16 store_length; > diff --git a/sql/partition_info.cc b/sql/partition_info.cc > index 52bda560c1c..0111fc1451d 100644 > --- a/sql/partition_info.cc > +++ b/sql/partition_info.cc > @@ -3164,6 +3164,32 @@ void partition_info::print_debug(const char *str, uint *value) > DBUG_PRINT("info", ("parser: %s", str)); > DBUG_VOID_RETURN; > } > + > +/* > + > + Disabling reading EITS statistics for columns involved in the > + partition list of a table. > + We assume the selecticivity for such columns would be handled > + during partition pruning. > + > +*/ > + This comment here doesn't make any sense anymore. Please move it to create_key_parts_for_pseudo_indexes. Also, the name and return value of the function is confusing now. If one calls the function for the partitioning column: field_in_partition_expr(partition_column) == false. Please change the return value to be true in this case, and false otherwise. > +bool partition_info::field_in_partition_expr(Field *field) const > +{ > + uint i; > + for (i= 0; i < num_part_fields; i++) > + { > + if (field->eq(part_field_array[i])) > + return FALSE; > + } > + for (i= 0; i < num_subpart_fields; i++) > + { > + if (field->eq(subpart_field_array[i])) > + return FALSE; > + } > + return TRUE; > +} > + > #else /* WITH_PARTITION_STORAGE_ENGINE */ > /* > For builds without partitioning we need to define these functions > diff --git a/sql/partition_info.h b/sql/partition_info.h > index f250c5496bf..10b8954ace7 100644 > --- a/sql/partition_info.h > +++ b/sql/partition_info.h > @@ -384,6 +384,7 @@ class partition_info : public Sql_alloc > bool is_full_part_expr_in_fields(List<Item> &fields); > public: > bool has_unique_name(partition_element *element); > + bool field_in_partition_expr(Field *field) const; > }; > > uint32 get_next_partition_id_range(struct st_partition_iter* part_iter); > diff --git a/sql/sql_statistics.cc b/sql/sql_statistics.cc > index cb75a5c2176..02dd4970c99 100644 > --- a/sql/sql_statistics.cc > +++ b/sql/sql_statistics.cc > @@ -3589,6 +3589,22 @@ void set_statistics_for_table(THD *thd, TABLE *table) > (use_stat_table_mode <= COMPLEMENTARY || > !table->stats_is_read || read_stats->cardinality_is_null) ? > table->file->stats.records : read_stats->cardinality; > + > + /* > + For partitioned table, EITS statistics is based on data from all partitions. > + > + On the other hand, Partition Pruning figures which partitions will be > + accessed and then computes the estimate of rows in used_partitions. > + > + Use the estimate from Partition Pruning as it is typically more precise. > + Ideally, EITS should provide per-partition statistics but this is not > + implemented currently. > + */ Good. > + #ifdef WITH_PARTITION_STORAGE_ENGINE > + if (table->part_info) > + table->used_stat_records= table->file->stats.records; > + #endif > + > KEY *key_info, *key_info_end; > for (key_info= table->key_info, key_info_end= key_info+table->s->keys; > key_info < key_info_end; key_info++) -- BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://s.petrunia.net/blog

1 0

Re: [Maria-developers] MDEV-16991 review.
by Alexander Barkov 25 Nov '18

25 Nov '18

Hi Alexey, On 11/25/2018 03:00 AM, Alexey Botchkov wrote: > Hi, Alexander. > > First question about the sql/sql_basic_types.h. > To me it looks overcomplicated. Do we really need classes over > the simple enums? > I attached my version of this file here. In my opinion it's shorter itself, > and makes other code look nicer. This is intentional, for strict data type control. Simple enums have some problems. For example, you can pass enums to a function that expects an integer data type. I want any mistakes like this to cause a compilation error. I would avoid using the traditional enums. Can you please leave it as is? I just found that C++11 introduced enum classes. They don't convert to int implicitly, and don't export their values to the public name space. https://www.codeguru.com/cpp/cpp/article.php/c19083/C-2011-Stronglytyped-En… We have enabled C++11 in 10.4 So now this could be changed to: class enum date_conv_mode_t { CONV_NONE= 0U, FUZZY_DATES= 1U, TIME_ONLY= 4U, INTERVAL_hhmmssff= 8U, INTERVAL_DAY= 16U, RANGE0_LAST= INTERVAL_DAY, NO_ZERO_IN_DATE= (1UL << 23), // MODE_NO_ZERO_IN_DATE NO_ZERO_DATE= (1UL << 24), // MODE_NO_ZERO_DATE INVALID_DATES= (1UL << 25) // MODE_INVALID_DATES }; We could try this. > > Other than that > > lines like this can be shortened > -TIME_FUZZY_DATES (date_mode_t::value_t::FUZZY_DATES), > +TIME_FUZZY_DATES (date_mode_t::FUZZY_DATES), Thanks. Fixed. > > Three variables all defined differently. Looks funny :) > const date_conv_mode_t > ... > static const date_conv_mode_t > ... > static time_round_mode_t > Why is that? Thanks for noticing this. I've changed them all to "static const". > > > What do you think if we do this: > - add THD* memver to the MYSQL_TIME_STATUS structure, > so we can get rid of the THD * parameter to many functions? > - send one MYSQL_TIME_STATUS* to the add_nanoseconds() > instead of three {thd, &status->warnings, and status->nanoseconds} ? Changing all functions that accept both &warn and nsec to accept a pointer to MYSQL_TIME_STATUS instead, like this: < xxxx(thd, &status->warnings, and status->nanoseconds); > xxxx(thd, &status); is a very good idea. I'd avoid mix "thd" to MYSQL_TIME_STATUS though. It's a pure C structure. Does it sound OK? > > You do like this: > Datetime::Options opt(TIME_CONV_NONE, thd); > Datetime dt(thd, item->arguments()[0], opt); > > Why not just > Datetime dt(thd, item->arguments()[0], Datetime::Options > opt(TIME_CONV_NONE, thd))? > I'm not sure about this very case, but ofter code like yours translated > with extra > constructor call. When the line fits the entire call, I do like this: a. Datetime dt(thd, item, Datetime::Options opt(mode, thd)); When the line does not fit, I tried both ways: b. Datetime dt(thd, item->arguments()[0], Datetime::Options opt(TIME_CONV_NONE, thd)); and c. Datetime::Options opt(TIME_CONV_NONE, thd); Datetime dt(thd, item->arguments()[0], opt); and "c" looked easier to read for me than "b". But this causes a constructor call indeed. Although, in case of date_mode_t this is very cheap (as cheap as copying of a 4 byte m_mode value), it's still a good idea to avoid this. I'll change like you suggest. Moreover, it's very likely that date_mode_t can change to something mode complex in the future. So an extra constructor call can get more expensive. > > Best regards. > HF >

1 1

Re: [Maria-developers] [Commits] 68b7e231cb3: MDEV-17255: New optimizer defaults and ANALYZE TABLE
by Sergey Petrunia 24 Nov '18

24 Nov '18

Hi Varun, On Fri, Nov 16, 2018 at 11:48:59PM +0530, Varun wrote: > revision-id: 68b7e231cb3a1cdfbd968c2cbf72687b29b6d2da (mariadb-10.3.6-209-g68b7e231cb3) > parent(s): b9a9055793ab8b9a50200e2f55602463627dedd3 Does the above say the patch is going into 10.3? The MDEV has fixVersion=10.4 and the parent revision is also in 10.4. So I assume the target version is 10.4? > author: Varun Gupta > committer: Varun Gupta > timestamp: 2018-11-16 23:48:11 +0530 > message: > > MDEV-17255: New optimizer defaults and ANALYZE TABLE > > Added another value for the use_stat_tables system variable Please mention in the commit comment: - the name of the new value - the fact that the new value is now the default. > --- > sql/opt_range.cc | 2 +- > sql/sql_admin.cc | 3 ++- > sql/sql_statistics.h | 1 + > sql/sys_vars.cc | 4 ++-- > 4 files changed, 6 insertions(+), 4 deletions(-) Please run the testsuite. I did the 'main' testsuite and: - main.mysqld--help, main.stat_tables_disabled - these need updates - a few other tests need to be updated as now opening any table will now cause the server to attept to read EITS stats for it. Please also add test coverage for this particular MDEV. This should be a testcase that shows that - with PREFERABLY, ANALYZE will collect EITS stats, SELECT will use them. - with PREFERABLY_FOR_READS : -- ANALYZE will not collect EITS stats -- ANALYZE .. PERSISTENT FOR ... will update the EITS stats. -- SELECT will use them. . > > diff --git a/sql/opt_range.cc b/sql/opt_range.cc > index d6db365a8a2..f501a5ae085 100644 > --- a/sql/opt_range.cc > +++ b/sql/opt_range.cc > @@ -3158,7 +3158,7 @@ bool calculate_cond_selectivity_for_table(THD *thd, TABLE *table, Item **cond) > > if (thd->variables.optimizer_use_condition_selectivity > 2 && > !bitmap_is_clear_all(used_fields) && > - thd->variables.use_stat_tables > 0) > + get_use_stat_tables_mode(thd) > NEVER) > { > PARAM param; > MEM_ROOT alloc; > diff --git a/sql/sql_admin.cc b/sql/sql_admin.cc > index d0d959de8f9..b5c732737b7 100644 > --- a/sql/sql_admin.cc > +++ b/sql/sql_admin.cc > @@ -767,7 +767,8 @@ static bool mysql_admin_table(THD* thd, TABLE_LIST* tables, > } > collect_eis= > (table->table->s->table_category == TABLE_CATEGORY_USER && > - (get_use_stat_tables_mode(thd) > NEVER || > + ((get_use_stat_tables_mode(thd) > NEVER && > + get_use_stat_tables_mode(thd) < PREFERABLY_FOR_READS) || > lex->with_persistent_for_clause)); > > > diff --git a/sql/sql_statistics.h b/sql/sql_statistics.h > index 39cddf95188..a942c05be09 100644 > --- a/sql/sql_statistics.h > +++ b/sql/sql_statistics.h > @@ -22,6 +22,7 @@ enum enum_use_stat_tables_mode > NEVER, > COMPLEMENTARY, > PREFERABLY, Please add a comment describing the new value. > + PREFERABLY_FOR_READS, > } Use_stat_tables_mode; > > typedef > diff --git a/sql/sys_vars.cc b/sql/sys_vars.cc > index 92c7d329bb9..3652d728fc1 100644 > --- a/sql/sys_vars.cc > +++ b/sql/sys_vars.cc > @@ -5841,12 +5841,12 @@ static Sys_var_ulong Sys_progress_report_time( > VALID_RANGE(0, UINT_MAX), DEFAULT(5), BLOCK_SIZE(1)); > > const char *use_stat_tables_modes[] = > - {"NEVER", "COMPLEMENTARY", "PREFERABLY", 0}; > + {"NEVER", "COMPLEMENTARY", "PREFERABLY", "PREFERABLY_FOR_READS", 0}; > static Sys_var_enum Sys_optimizer_use_stat_tables( > "use_stat_tables", > "Specifies how to use system statistics tables", > SESSION_VAR(use_stat_tables), CMD_LINE(REQUIRED_ARG), > - use_stat_tables_modes, DEFAULT(0)); > + use_stat_tables_modes, DEFAULT(3)); > > static Sys_var_ulong Sys_histogram_size( > "histogram_size", BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://s.petrunia.net/blog

1 0

Re: [Maria-developers] afedc29f773: MDEV-14024 PCRE2.
by Sergei Golubchik 23 Nov '18

23 Nov '18

Hi, Alexey! Thanks! Few minor comments, see below On Nov 23, Alexey Botchkov wrote: > revision-id: afedc29f773362f342756e9cdd9bbd9ed0f1abe2 (versioning-1.0.3-68-gafedc29f773) > parent(s): 4ec8598c1dcb63499bce998142b8e5b8b09b2d30 > author: Alexey Botchkov <holyfoot(a)askmonty.org> > committer: Alexey Botchkov <holyfoot(a)askmonty.org> > timestamp: 2018-06-26 14:10:46 +0400 > message: > > MDEV-14024 PCRE2. > > Related changes in the server code. > > diff --git a/cmake/pcre.cmake b/cmake/pcre.cmake > index 4c113929866..264f93a60a2 100644 > --- a/cmake/pcre.cmake > +++ b/cmake/pcre.cmake > @@ -5,24 +5,17 @@ SET(WITH_PCRE "auto" CACHE STRING > > MACRO (CHECK_PCRE) > IF(WITH_PCRE STREQUAL "system" OR WITH_PCRE STREQUAL "auto") > - CHECK_LIBRARY_EXISTS(pcre pcre_stack_guard "" HAVE_PCRE_STACK_GUARD) > + CHECK_LIBRARY_EXISTS(pcre2-8 pcre2_match "" HAVE_PCRE2) > - IF(NOT CMAKE_CROSSCOMPILING) > - SET(CMAKE_REQUIRED_LIBRARIES "pcre") > - CHECK_C_SOURCE_RUNS(" > - #include <pcre.h> > - int main() { > - return -pcre_exec(NULL, NULL, NULL, -999, -999, 0, NULL, 0) < 256; > - }" PCRE_STACK_SIZE_OK) > - SET(CMAKE_REQUIRED_LIBRARIES) > - ENDIF() > ENDIF() > - IF(NOT HAVE_PCRE_STACK_GUARD OR NOT PCRE_STACK_SIZE_OK OR > - WITH_PCRE STREQUAL "bundled") > + IF(NOT HAVE_PCRE2 OR WITH_PCRE STREQUAL "bundled") > IF (WITH_PCRE STREQUAL "system") > - MESSAGE(FATAL_ERROR "system pcre is not found or unusable") > + MESSAGE(FATAL_ERROR "system pcre2-8 library is not found or unusable") > ENDIF() > - SET(PCRE_INCLUDES ${CMAKE_BINARY_DIR}/pcre ${CMAKE_SOURCE_DIR}/pcre) > - ADD_SUBDIRECTORY(pcre) > + SET(PCRE_INCLUDES ${CMAKE_BINARY_DIR}/pcre2 ${CMAKE_SOURCE_DIR}/pcre2 > + ${CMAKE_BINARY_DIR}/pcre2/src ${CMAKE_SOURCE_DIR}/pcre2/src) That's a bit strange. I'd expect only .../pcre2/src paths, not .../pcre2. you need pcre2.h and pcre2posix.h, they're both in src/ right? > + SET(PCRE_BUILD_TESTS OFF CACHE BOOL "Disable tests.") > + SET(PCRE2_BUILD_PCRE2GREP OFF CACHE BOOL "Disable pcre2grep") > + ADD_SUBDIRECTORY(pcre2) > ENDIF() > ENDMACRO() > > diff --git a/extra/mariabackup/CMakeLists.txt b/extra/mariabackup/CMakeLists.txt > index 7df5fa17903..a9d7153b893 100644 > --- a/extra/mariabackup/CMakeLists.txt > +++ b/extra/mariabackup/CMakeLists.txt > @@ -37,7 +37,7 @@ INCLUDE_DIRECTORIES( > ) > > IF(NOT HAVE_SYSTEM_REGEX) > - INCLUDE_DIRECTORIES(${CMAKE_SOURCE_DIR}/pcre) > + INCLUDE_DIRECTORIES(${CMAKE_SOURCE_DIR}/pcre2 ${CMAKE_SOURCE_DIR}/pcre2/src) Hmm, not INCLUDE_DIRECTORIES(${PCRE_INCLUDES}) ? pcre2.h is in the ${CMAKE_BINARY_DIR}, it's generated. > ENDIF() > > IF(WITH_WSREP) > diff --git a/mysql-test/main/func_regexp_pcre.test b/mysql-test/main/func_regexp_pcre.test > index 21600390bb2..30969b3e9ae 100644 > --- a/mysql-test/main/func_regexp_pcre.test > +++ b/mysql-test/main/func_regexp_pcre.test > @@ -382,6 +382,7 @@ SELECT 'AB' RLIKE 'A B'; > SELECT 'AB' RLIKE 'A# this is a comment\nB'; > SET default_regex_flags=DEFAULT; > > +--error ER_REGEXP_ERROR > SELECT 'Aq' RLIKE 'A\\q'; you forgot to update func_regexp_pcre.result > SET default_regex_flags='EXTRA'; > --error ER_REGEXP_ERROR > diff --git a/sql/item_cmpfunc.cc b/sql/item_cmpfunc.cc > index 76f4788c1cf..450a9c54176 100644 > --- a/sql/item_cmpfunc.cc > +++ b/sql/item_cmpfunc.cc > @@ -5485,124 +5486,44 @@ bool Regexp_processor_pcre::compile(Item *item, bool send_error) > */ > void Regexp_processor_pcre::pcre_exec_warn(int rc) const > { > - char buf[64]; > - const char *errmsg= NULL; > + PCRE2_UCHAR8 buf[128]; > THD *thd= current_thd; > > - /* > - Make a descriptive message only for those pcre_exec() error codes > - that can actually happen in MariaDB. > - */ > - switch (rc) > + int errlen= pcre2_get_error_message(rc, buf, sizeof(buf)); > + if (errlen <= 0) > { > - case PCRE_ERROR_NULL: > - errmsg= "pcre_exec: null argument passed"; > - break; > - case PCRE_ERROR_BADOPTION: > - errmsg= "pcre_exec: bad option"; > - break; > - case PCRE_ERROR_BADMAGIC: > - errmsg= "pcre_exec: bad magic - not a compiled regex"; > - break; > - case PCRE_ERROR_UNKNOWN_OPCODE: > - errmsg= "pcre_exec: error in compiled regex"; > - break; > - case PCRE_ERROR_NOMEMORY: > - errmsg= "pcre_exec: Out of memory"; > - break; > - case PCRE_ERROR_NOSUBSTRING: > - errmsg= "pcre_exec: no substring"; > - break; > - case PCRE_ERROR_MATCHLIMIT: > - errmsg= "pcre_exec: match limit exceeded"; > - break; > - case PCRE_ERROR_CALLOUT: > - errmsg= "pcre_exec: callout error"; > - break; > - case PCRE_ERROR_BADUTF8: > - errmsg= "pcre_exec: Invalid utf8 byte sequence in the subject string"; > - break; > - case PCRE_ERROR_BADUTF8_OFFSET: > - errmsg= "pcre_exec: Started at invalid location within utf8 byte sequence"; > - break; > - case PCRE_ERROR_PARTIAL: > - errmsg= "pcre_exec: partial match"; > - break; > - case PCRE_ERROR_INTERNAL: > - errmsg= "pcre_exec: internal error"; > - break; > - case PCRE_ERROR_BADCOUNT: > - errmsg= "pcre_exec: ovesize is negative"; > - break; > - case PCRE_ERROR_RECURSIONLIMIT: > - my_snprintf(buf, sizeof(buf), "pcre_exec: recursion limit of %ld exceeded", > - m_pcre_extra.match_limit_recursion); > - errmsg= buf; > - break; > - case PCRE_ERROR_BADNEWLINE: > - errmsg= "pcre_exec: bad newline options"; > - break; > - case PCRE_ERROR_BADOFFSET: > - errmsg= "pcre_exec: start offset negative or greater than string length"; > - break; > - case PCRE_ERROR_SHORTUTF8: > - errmsg= "pcre_exec: ended in middle of utf8 sequence"; > - break; > - case PCRE_ERROR_JIT_STACKLIMIT: > - errmsg= "pcre_exec: insufficient stack memory for JIT compile"; > - break; > - case PCRE_ERROR_RECURSELOOP: > - errmsg= "pcre_exec: Recursion loop detected"; > - break; > - case PCRE_ERROR_BADMODE: > - errmsg= "pcre_exec: compiled pattern passed to wrong bit library function"; > - break; > - case PCRE_ERROR_BADENDIANNESS: > - errmsg= "pcre_exec: compiled pattern passed to wrong endianness processor"; > - break; > - case PCRE_ERROR_JIT_BADOPTION: > - errmsg= "pcre_exec: bad jit option"; > - break; > - case PCRE_ERROR_BADLENGTH: > - errmsg= "pcre_exec: negative length"; > - break; > - default: > - /* > - As other error codes should normally not happen, > - we just report the error code without textual description > - of the code. > - */ > - my_snprintf(buf, sizeof(buf), "pcre_exec: Internal error (%d)", rc); > - errmsg= buf; > + my_snprintf((char *)buf, sizeof(buf), "pcre_exec: Internal error (%d)", rc); > } > push_warning_printf(thd, Sql_condition::WARN_LEVEL_WARN, > - ER_REGEXP_ERROR, ER_THD(thd, ER_REGEXP_ERROR), errmsg); > + ER_REGEXP_ERROR, ER_THD(thd, ER_REGEXP_ERROR), buf); > } > > > /** > Call pcre_exec() and send a warning if pcre_exec() returned with an error. > */ > -int Regexp_processor_pcre::pcre_exec_with_warn(const pcre *code, > - const pcre_extra *extra, > +int Regexp_processor_pcre::pcre_exec_with_warn(const pcre2_code *code, > + pcre2_match_data *data, > const char *subject, > int length, int startoffset, > - int options, int *ovector, > - int ovecsize) > + uint options) > { > - int rc= pcre_exec(code, extra, subject, length, > - startoffset, options, ovector, ovecsize); > + int rc= pcre2_match(code, (PCRE2_SPTR8) subject, (PCRE2_SIZE) length, > + (PCRE2_SIZE) startoffset, options, data, NULL); > DBUG_EXECUTE_IF("pcre_exec_error_123", rc= -123;); > - if (unlikely(rc < PCRE_ERROR_NOMATCH)) > - pcre_exec_warn(rc); > + if (unlikely(rc < PCRE2_ERROR_NOMATCH)) > + m_SubStrVec= NULL; > + else > + m_SubStrVec= pcre2_get_ovector_pointer(data); > + The function name is still pcre_exec_with_warn, the comment still says "Call pcre_exec() and send a warning". But neither seems to be happening here. where do you do warnings now? > return rc; > } > > > bool Regexp_processor_pcre::exec(const char *str, size_t length, size_t offset) > { > - m_pcre_exec_rc= pcre_exec_with_warn(m_pcre, &m_pcre_extra, str, (int)length, (int)offset, 0, > - m_SubStrVec, array_elements(m_SubStrVec)); > + m_pcre_exec_rc= pcre_exec_with_warn(m_pcre, m_pcre_match_data, > + str, (int)length, (int)offset, 0); > return false; > } > > diff --git a/sql/sys_vars.cc b/sql/sys_vars.cc > index ccb7d3b29ed..e8abae3487d 100644 > --- a/sql/sys_vars.cc > +++ b/sql/sys_vars.cc > @@ -5692,19 +5692,21 @@ static const char *default_regex_flags_names[]= > "DOTALL", // (?s) . matches anything including NL > "DUPNAMES", // (?J) Allow duplicate names for subpatterns > "EXTENDED", // (?x) Ignore white space and # comments > - "EXTRA", // (?X) extra features (e.g. error on unknown escape character) > + "EXTENDED_MORE",//(?xx) Ignore white space and # comments inside cheracter perhaps, update all flag descriptions from man pcre2syntax? Like "EXTENDED", // (?x) extended: ignore white space except in classes "EXTENDED_MORE",//(?xx) as (?x) but also ignore space and tab in classes also, I don't quite like EXTENDED_MORE name. But I don't have a good suggestion. May be just re-purpose EXTENDED to do (?xx) without introducing a new flag? > + "EXTRA", // means nothing since PCRE2 may be a warning when it's used? > "MULTILINE", // (?m) ^ and $ match newlines within data > "UNGREEDY", // (?U) Invert greediness of quantifiers > 0 > }; > static const int default_regex_flags_to_pcre[]= > { > - PCRE_DOTALL, > - PCRE_DUPNAMES, > - PCRE_EXTENDED, > - PCRE_EXTRA, > - PCRE_MULTILINE, > - PCRE_UNGREEDY, > + PCRE2_DOTALL, > + PCRE2_DUPNAMES, > + PCRE2_EXTENDED, > + PCRE2_EXTENDED_MORE, > + 0, /* EXTRA flag not available since PCRE2 */ > + PCRE2_MULTILINE, > + PCRE2_UNGREEDY, > 0 > }; > int default_regex_flags_pcre(const THD *thd) > diff --git a/storage/tokudb/PerconaFT/cmake_modules/TokuFeatureDetection.cmake b/storage/tokudb/PerconaFT/cmake_modules/TokuFeatureDetection.cmake > index 2f04a33558a..51257febc0d 100644 > --- a/storage/tokudb/PerconaFT/cmake_modules/TokuFeatureDetection.cmake > +++ b/storage/tokudb/PerconaFT/cmake_modules/TokuFeatureDetection.cmake > @@ -1,6 +1,8 @@ > ## feature detection > find_package(Threads) > -find_package(ZLIB REQUIRED) > +IF(WITH_EXTERNAL_ZLIB) > + find_package(ZLIB REQUIRED) > +ENDIF() push it in separate commit, please. > > option(USE_VALGRIND "Build to run safely under valgrind (often slower)." ON) > if(USE_VALGRIND) > Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

1 0

Re: [Maria-developers] 49b63dcc048: MDEV-15073: Generic UDAF parser code in server for windows functions
by Sergei Golubchik 23 Nov '18

23 Nov '18

Hi, Oleksandr! On Nov 21, Oleksandr Byelkin wrote: > revision-id: 49b63dcc0482bcb0fc7f642b0c19c8e24740ab27 (mariadb-10.4.0-20-g49b63dcc048) > parent(s): b5ac863f1494920b5e7035c9dfa0ebfdaa50a15d > author: Oleksandr Byelkin <sanja(a)mariadb.com> > committer: Oleksandr Byelkin <sanja(a)mariadb.com> > timestamp: 2018-11-20 10:58:34 +0100 > message: > > MDEV-15073: Generic UDAF parser code in server for windows functions > > Added support for usual agreggate UDF (UDAF) > Added remove() call support for more efficient window function processing > Added example of aggregate UDF with efficient windows function support Looks good, see a couple of minor comments below. I wouldn't extend UDF API, really. It's ancient and it should be removed and replaced with a new plugin type, not extended. But if you still want to do it make sure it's documented https://mariadb.com/kb/en/user-defined-functions/ > diff --git a/mysql-test/main/udf.test b/mysql-test/main/udf.test > index c3a25c6bcce..e72f8b219af 100644 > --- a/mysql-test/main/udf.test > +++ b/mysql-test/main/udf.test > @@ -528,3 +528,62 @@ DROP FUNCTION METAPHON; > #INSERT INTO t1 (a) VALUES ('Hello'); > #SELECT * FROM t1; > DROP TABLE t1; > + > +--echo > +--echo MDEV-15073: Generic UDAF parser code in server for windows functions > +--echo use --echo # so that your echo commands would stand out in the result file. currently they look like an output from some query and it's confusing. The test itself is good. > diff --git a/sql/item_sum.cc b/sql/item_sum.cc > index 0e52b2988a3..d19645b5020 100644 > --- a/sql/item_sum.cc > +++ b/sql/item_sum.cc > @@ -3235,6 +3235,25 @@ bool Item_udf_sum::add() > DBUG_RETURN(0); > } > > + > +bool Item_udf_sum::supports_removal() > +{ > + DBUG_ENTER("Item_udf_sum::supports_remove"); > + DBUG_PRINT("info", ("support: %d", udf.supports_removal())); > + DBUG_RETURN(udf.supports_removal()); > +} > + > + > +void Item_udf_sum::remove() > +{ > + my_bool tmp_null_value; > + DBUG_ENTER("Item_udf_sum::remove"); > + udf.remove(&tmp_null_value); > + null_value= tmp_null_value; > + DBUG_VOID_RETURN; > +} > + > + > void Item_udf_sum::cleanup() > { > /* > diff --git a/sql/item_sum.h b/sql/item_sum.h > index 1a21c257221..663dea1ed7b 100644 > --- a/sql/item_sum.h > +++ b/sql/item_sum.h > @@ -574,7 +574,7 @@ class Item_sum :public Item_func_or_sum > virtual bool add()= 0; > virtual bool setup(THD *thd) { return false; } > > - virtual bool supports_removal() const { return false; } > + virtual bool supports_removal() { return false; } why? > virtual void remove() { DBUG_ASSERT(0); } Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

2 2

[Maria-developers] MariaDB Developers Unconference
by Ian Gilfillan 19 Nov '18

19 Nov '18

The MariaDB Developer Unconference is taking place in New York 23-24 Feb, just before OpenWorks: https://mariadb.org/2019-developers-unconference-new-york/ Please sign up on the Meetup page: https://www.meetup.com/MariaDB-Developers/events/256385430/ - security is very tight at the building, so everyone needs to be signed up in advance to be given access. There's a cap on the attendance, so don't delay too long. If you want to suggest a session, the spreadsheet is open for proposals.

1 0

[Maria-developers] 7c088b624f4: MDEV-14500 filesort to support engines with slow rnd_pos
by Sergei Golubchik 19 Nov '18

19 Nov '18

revision-id: 7c088b624f4f50e1e9f11891c371eba912228a2d (mariadb-10.4.0-14-g7c088b624f4) parent(s): da8a589a982c45ed161b9a25d61282a6bc1fa037 author: Sergei Golubchik <serg(a)mariadb.org> committer: Sergei Golubchik <serg(a)mariadb.org> timestamp: 2018-11-17 14:13:37 +0100 message: MDEV-14500 filesort to support engines with slow rnd_pos If the engine wants to avoid rnd_pos() - force a temporary table before a filesort. But don't do it if addon_fields are used. --- mysql-test/suite/archive/rnd_pos.result | 56 +++++++++++++++++++++++++++++++++ mysql-test/suite/archive/rnd_pos.test | 27 ++++++++++++++++ sql/filesort.cc | 55 ++++++++++++++++++-------------- sql/filesort.h | 3 ++ sql/handler.h | 7 +++++ sql/sql_select.cc | 15 +++++++++ storage/archive/ha_archive.h | 2 +- storage/csv/ha_tina.h | 2 +- 8 files changed, 142 insertions(+), 25 deletions(-) diff --git a/mysql-test/suite/archive/rnd_pos.result b/mysql-test/suite/archive/rnd_pos.result new file mode 100644 index 00000000000..b6b6748d53f --- /dev/null +++ b/mysql-test/suite/archive/rnd_pos.result @@ -0,0 +1,56 @@ +create table t1(c1 int not null, c2 double not null, c3 char(255) not null) engine=archive; +insert t1 select seq, seq+0.7, concat('row with c1 = ', seq) from seq_1_to_10; +explain partitions select c1,c3 from t1 order by c2; +id select_type table partitions type possible_keys key key_len ref rows Extra +1 SIMPLE t1 NULL ALL NULL NULL NULL NULL 10 Using filesort +set max_length_for_sort_data = 4; +explain partitions select c1,c3 from t1 order by c2; +id select_type table partitions type possible_keys key key_len ref rows Extra +1 SIMPLE t1 NULL ALL NULL NULL NULL NULL 10 Using temporary; Using filesort +flush status; +select c1,c3 from t1 order by c2; +c1 c3 +1 row with c1 = 1 +2 row with c1 = 2 +3 row with c1 = 3 +4 row with c1 = 4 +5 row with c1 = 5 +6 row with c1 = 6 +7 row with c1 = 7 +8 row with c1 = 8 +9 row with c1 = 9 +10 row with c1 = 10 +set max_length_for_sort_data = default; +show status where variable_name like '%tmp%' and value != 0; +Variable_name Value +Created_tmp_tables 1 +Handler_tmp_write 10 +Rows_tmp_read 20 +alter table t1 partition by hash (c1) partitions 3; +explain partitions select c1,c3 from t1 order by c2; +id select_type table partitions type possible_keys key key_len ref rows Extra +1 SIMPLE t1 p0,p1,p2 ALL NULL NULL NULL NULL 10 Using filesort +set max_length_for_sort_data = 4; +explain partitions select c1,c3 from t1 order by c2; +id select_type table partitions type possible_keys key key_len ref rows Extra +1 SIMPLE t1 p0,p1,p2 ALL NULL NULL NULL NULL 10 Using temporary; Using filesort +flush status; +select c1,c3 from t1 order by c2; +c1 c3 +1 row with c1 = 1 +2 row with c1 = 2 +3 row with c1 = 3 +4 row with c1 = 4 +5 row with c1 = 5 +6 row with c1 = 6 +7 row with c1 = 7 +8 row with c1 = 8 +9 row with c1 = 9 +10 row with c1 = 10 +set max_length_for_sort_data = default; +show status where variable_name like '%tmp%' and value != 0; +Variable_name Value +Created_tmp_tables 1 +Handler_tmp_write 10 +Rows_tmp_read 20 +drop table t1; diff --git a/mysql-test/suite/archive/rnd_pos.test b/mysql-test/suite/archive/rnd_pos.test new file mode 100644 index 00000000000..7a1d78ea003 --- /dev/null +++ b/mysql-test/suite/archive/rnd_pos.test @@ -0,0 +1,27 @@ +# +# MDEV-14500 Support engines without rnd_pos +# +source include/have_archive.inc; +source include/have_sequence.inc; +source include/have_partition.inc; + +create table t1(c1 int not null, c2 double not null, c3 char(255) not null) engine=archive; +insert t1 select seq, seq+0.7, concat('row with c1 = ', seq) from seq_1_to_10; +explain partitions select c1,c3 from t1 order by c2; +set max_length_for_sort_data = 4; +explain partitions select c1,c3 from t1 order by c2; +flush status; +select c1,c3 from t1 order by c2; +set max_length_for_sort_data = default; +show status where variable_name like '%tmp%' and value != 0; + +alter table t1 partition by hash (c1) partitions 3; +explain partitions select c1,c3 from t1 order by c2; +set max_length_for_sort_data = 4; +explain partitions select c1,c3 from t1 order by c2; +flush status; +select c1,c3 from t1 order by c2; +set max_length_for_sort_data = default; +show status where variable_name like '%tmp%' and value != 0; + +drop table t1; diff --git a/sql/filesort.cc b/sql/filesort.cc index e682f3389da..cbe79967647 100644 --- a/sql/filesort.cc +++ b/sql/filesort.cc @@ -1957,6 +1957,30 @@ sortlength(THD *thd, SORT_FIELD *sortorder, uint s_length, return length; } +bool filesort_use_addons(TABLE *table, uint sortlength, + uint *length, uint *fields, uint *null_fields) +{ + Field **pfield, *field; + *length= *fields= *null_fields= 0; + + for (pfield= table->field; (field= *pfield) ; pfield++) + { + if (!bitmap_is_set(table->read_set, field->field_index)) + continue; + if (field->flags & BLOB_FLAG) + return false; + (*length)+= field->max_packed_col_length(field->pack_length()); + if (field->maybe_null()) + (*null_fields)++; + (*fields)++; + } + if (!*fields) + return false; + (*length)+= (*null_fields+7)/8; + + return *length + sortlength < + table->in_use->variables.max_length_for_sort_data; +} /** Get descriptors of fields appended to sorted fields and @@ -1991,11 +2015,8 @@ get_addon_fields(TABLE *table, uint sortlength, LEX_STRING *addon_buf) Field **pfield; Field *field; SORT_ADDON_FIELD *addonf; - uint length= 0; - uint fields= 0; - uint null_fields= 0; + uint length, fields, null_fields; MY_BITMAP *read_set= table->read_set; - ulong max_sort_len= table->in_use->variables.max_length_for_sort_data; DBUG_ENTER("get_addon_fields"); /* @@ -2011,26 +2032,14 @@ get_addon_fields(TABLE *table, uint sortlength, LEX_STRING *addon_buf) addon_buf->str= 0; addon_buf->length= 0; - for (pfield= table->field; (field= *pfield) ; pfield++) - { - if (!bitmap_is_set(read_set, field->field_index)) - continue; - if (field->flags & BLOB_FLAG) - DBUG_RETURN(0); - length+= field->max_packed_col_length(field->pack_length()); - if (field->maybe_null()) - null_fields++; - fields++; - } - if (!fields) - DBUG_RETURN(0); - length+= (null_fields+7)/8; + // see remove_const() for HA_SLOW_RND_POS explanation + if (table->file->ha_table_flags() & HA_SLOW_RND_POS) + sortlength= 0; - if (length+sortlength > max_sort_len || - !my_multi_malloc(MYF(MY_WME | MY_THREAD_SPECIFIC), - &addonf, sizeof(SORT_ADDON_FIELD) * (fields+1), - &addon_buf->str, length, - NullS)) + if (!filesort_use_addons(table, sortlength, &length, &fields, &null_fields) || + !my_multi_malloc(MYF(MY_WME | MY_THREAD_SPECIFIC), &addonf, + sizeof(SORT_ADDON_FIELD) * (fields+1), + &addon_buf->str, length, NullS)) DBUG_RETURN(0); diff --git a/sql/filesort.h b/sql/filesort.h index bd1d81f91ef..359f44a3907 100644 --- a/sql/filesort.h +++ b/sql/filesort.h @@ -161,6 +161,9 @@ SORT_INFO *filesort(THD *thd, TABLE *table, Filesort *filesort, Filesort_tracker* tracker, JOIN *join=NULL, table_map first_table_bit=0); +bool filesort_use_addons(TABLE *table, uint sortlength, + uint *length, uint *fields, uint *null_fields); + void change_double_for_sort(double nr,uchar *to); #endif /* FILESORT_INCLUDED */ diff --git a/sql/handler.h b/sql/handler.h index b98244fe47e..ea514634b86 100644 --- a/sql/handler.h +++ b/sql/handler.h @@ -308,6 +308,13 @@ enum enum_alter_inplace_result { #define HA_SLOW_CMP_REF (1ULL << 54) #define HA_CMP_REF_IS_EXPENSIVE HA_SLOW_CMP_REF +/** + Some engines are unable to provide an efficient implementation for rnd_pos(). + Server will try to avoid it, if possible + + TODO better to do it with cost estimates, not with an explicit flag +*/ +#define HA_SLOW_RND_POS (1ULL << 55) /* bits in index_flags(index_number) for what you can do with index */ #define HA_READ_NEXT 1 /* TODO really use this flag */ diff --git a/sql/sql_select.cc b/sql/sql_select.cc index 3f5def0838e..c16521068c6 100644 --- a/sql/sql_select.cc +++ b/sql/sql_select.cc @@ -13181,6 +13181,21 @@ remove_const(JOIN *join,ORDER *first_order, COND *cond, JOIN_TAB *head= join->join_tab + join->const_tables; *simple_order= head->on_expr_ref[0] == NULL; + if (*simple_order && head->table->file->ha_table_flags() & HA_SLOW_RND_POS) + { + uint u1, u2, u3; + /* + normally the condition is (see filesort_use_addons()) + + length + sortlength <= max_length_for_sort_data + + but for HA_SLOW_RND_POS tables we relax it a bit, as the alternative + is to use a temporary table, which is rather expensive. + + TODO proper cost estimations + */ + *simple_order= filesort_use_addons(head->table, 0, &u1, &u2, &u3); + } } else { diff --git a/storage/archive/ha_archive.h b/storage/archive/ha_archive.h index 56ff566db8c..1f25fba4eed 100644 --- a/storage/archive/ha_archive.h +++ b/storage/archive/ha_archive.h @@ -108,7 +108,7 @@ class ha_archive: public handler return (HA_NO_TRANSACTIONS | HA_REC_NOT_IN_SEQ | HA_CAN_BIT_FIELD | HA_BINLOG_ROW_CAPABLE | HA_BINLOG_STMT_CAPABLE | HA_STATS_RECORDS_IS_EXACT | HA_CAN_EXPORT | - HA_HAS_RECORDS | HA_CAN_REPAIR | + HA_HAS_RECORDS | HA_CAN_REPAIR | HA_SLOW_RND_POS | HA_FILE_BASED | HA_CAN_INSERT_DELAYED | HA_CAN_GEOMETRY); } ulong index_flags(uint idx, uint part, bool all_parts) const diff --git a/storage/csv/ha_tina.h b/storage/csv/ha_tina.h index c75a64faa52..5b389d984d6 100644 --- a/storage/csv/ha_tina.h +++ b/storage/csv/ha_tina.h @@ -107,7 +107,7 @@ class ha_tina: public handler { return (HA_NO_TRANSACTIONS | HA_REC_NOT_IN_SEQ | HA_NO_AUTO_INCREMENT | HA_BINLOG_ROW_CAPABLE | HA_BINLOG_STMT_CAPABLE | HA_CAN_EXPORT | - HA_CAN_REPAIR); + HA_CAN_REPAIR | HA_SLOW_RND_POS); } ulong index_flags(uint idx, uint part, bool all_parts) const {

1 0

Re: [Maria-developers] [Commits] 3f4dab80912: MDEV-17032: Estimates are higher for partitions of a table with @@use_stat_tables= PREFERABLY
by Sergey Petrunia 14 Nov '18

14 Nov '18

Hi Varun, On Mon, Nov 12, 2018 at 02:41:51PM +0530, Varun wrote: > revision-id: 3f4dab80912682325b61b288178febe9ee55de49 (mariadb-10.0.36-78-g3f4dab80912) > parent(s): 6cecb10a2f8b6536bed78ab6d3791d8befc9d732 > author: Varun Gupta > committer: Varun Gupta > timestamp: 2018-11-12 14:40:07 +0530 > message: > > MDEV-17032: Estimates are higher for partitions of a table with @@use_stat_tables= PREFERABLY > > The problem here is EITS statistics does not calculate statistics for the partitions of the table. > So a temporary solution would be to not read EITS statistics for partitioned tables. > > Also disabling reading of EITS for columns that participate in the partition list of a table. > > --- > mysql-test/r/partition.result | 37 +++++++++++++++++++++++++++++++++++++ > mysql-test/t/partition.test | 35 +++++++++++++++++++++++++++++++++++ > sql/opt_range.cc | 12 ++++++++++-- > sql/partition_info.cc | 21 +++++++++++++++++++++ > sql/partition_info.h | 1 + > sql/sql_statistics.cc | 6 ++++++ > 6 files changed, 110 insertions(+), 2 deletions(-) > > diff --git a/mysql-test/r/partition.result b/mysql-test/r/partition.result > index c6669176b3d..abfbc21dccb 100644 > --- a/mysql-test/r/partition.result > +++ b/mysql-test/r/partition.result > @@ -2645,3 +2645,40 @@ Warnings: > Note 1517 Duplicate partition name p2 > DEALLOCATE PREPARE stmt; > DROP TABLE t1; > +# > +# MDEV-17032: Estimates are higher for partitions of a table with @@use_stat_tables= PREFERABLY > +# > +create table ten(a int); > +insert into ten values (0),(1),(2),(3),(4),(5),(6),(7),(8),(9); > +create table one_k(a int); > +insert into one_k select A.a + B.a* 10 + C.a * 100 from ten A, ten B, ten C; Please rename the tables to follow the t0,t1,t2... convention. > +create table t1 ( > +part_key int, > +a int, > +b int > +) partition by list(part_key) ( > +partition p0 values in (0), > +partition p1 values in (1), > +partition p2 values in (2), > +partition p3 values in (3), > +partition p4 values in (4) > +); > +insert into t1 > +select mod(a,5), a/100, a from one_k; > +set @save_use_stat_tables= @@use_stat_tables; > +set @save_optimizer_use_condition_selectivity=@@optimizer_use_condition_selectivity; > +set @@use_stat_tables= PREFERABLY; > +set @@optimizer_use_condition_selectivity=4; > +analyze table t1; > +Table Op Msg_type Msg_text > +test.t1 analyze status Engine-independent statistics collected > +test.t1 analyze status OK After some time passes, it gets hard to understand what exactly is being checked. Then, there's some change in the EXPLAIN (typically for an unrelated reason) and one is left wondering, whether the new EXPLAIN output is ok or not. * please use EXPLAIN PARTITIONS. * please add something like "--echo # rows should be X, not Y" before the query. > +explain select * from t1 where part_key in (1,2); > +id select_type table type possible_keys key key_len ref rows Extra > +1 SIMPLE t1 ALL NULL NULL NULL NULL 400 Using where > +explain select * from t1 where part_key > 1; > +id select_type table type possible_keys key key_len ref rows Extra > +1 SIMPLE t1 ALL NULL NULL NULL NULL 600 Using where This does not provide sufficient coverage. I get this result if I just apply the change in sql_statistics.cc. mysql> explain select * from t1 where part_key in (1,2); +------+-------------+-------+------+---------------+------+---------+------+------+-------------+ | id | select_type | table | type | possible_keys | key | key_len | ref | rows | Extra | +------+-------------+-------+------+---------------+------+---------+------+------+-------------+ | 1 | SIMPLE | t1 | ALL | NULL | NULL | NULL | NULL | 400 | Using where | +------+-------------+-------+------+---------------+------+---------+------+------+-------------+ 1 row in set (0.01 sec) mysql> explain select * from t1 where part_key > 1; +------+-------------+-------+------+---------------+------+---------+------+------+-------------+ | id | select_type | table | type | possible_keys | key | key_len | ref | rows | Extra | +------+-------------+-------+------+---------------+------+---------+------+------+-------------+ | 1 | SIMPLE | t1 | ALL | NULL | NULL | NULL | NULL | 600 | Using where | +------+-------------+-------+------+---------------+------+---------+------+------+-------------+ 1 row in set (0.00 sec) Please add testcases for other changes. > +set @@optimizer_use_condition_selectivity=@save_optimizer_use_condition_selectivity; > +set @@use_stat_tables= @save_use_stat_tables; > +drop table t1, one_k, ten; > diff --git a/sql/opt_range.cc b/sql/opt_range.cc > index 3bcaa72e32f..993b7d57e0a 100644 > --- a/sql/opt_range.cc > +++ b/sql/opt_range.cc > @@ -3322,6 +3322,10 @@ bool create_key_parts_for_pseudo_indexes(RANGE_OPT_PARAM *param, > { > Field **field_ptr; > TABLE *table= param->table; > + partition_info *part_info= NULL; > + #ifdef WITH_PARTITION_STORAGE_ENGINE > + part_info= table->part_info; > + #endif > uint parts= 0; > > for (field_ptr= table->field; *field_ptr; field_ptr++) > @@ -3329,7 +3333,9 @@ bool create_key_parts_for_pseudo_indexes(RANGE_OPT_PARAM *param, > Column_statistics* col_stats= (*field_ptr)->read_stats; > if (bitmap_is_set(used_fields, (*field_ptr)->field_index) > && col_stats && !col_stats->no_stat_values_provided() > - && !((*field_ptr)->type() == MYSQL_TYPE_GEOMETRY)) > + && !((*field_ptr)->type() == MYSQL_TYPE_GEOMETRY) > + && (!part_info || > + part_info->disable_eits_for_partitioning_columns(*field_ptr))) > parts++; > } > > @@ -3350,7 +3356,9 @@ bool create_key_parts_for_pseudo_indexes(RANGE_OPT_PARAM *param, > if (bitmap_is_set(used_fields, (*field_ptr)->field_index)) > { > Field *field= *field_ptr; > - if (field->type() == MYSQL_TYPE_GEOMETRY) > + if (field->type() == MYSQL_TYPE_GEOMETRY > + && (!part_info || > + part_info->disable_eits_for_partitioning_columns(*field_ptr))) > continue; Why use 'field_ptr' if you have 'field' ? The above condition seems to be wrong. Suppose that the following holds: field->type()==MYSQL_TYPE_GEOMETRY, part_info!=NULL part_info->disable_eits_for_partitioning_columns(field) == FALSE // makes // sense as one typically does not do partitioning on geometry columns. if-condition evaluates to: (TRUE && (FALSE || FALSE)) -> (TRUE && FALSE) -> FALSE. So we will not execute 'continue;' for a geometry column. This seems incorrect. > > uint16 store_length; > diff --git a/sql/partition_info.cc b/sql/partition_info.cc > index 52bda560c1c..91d74b28d0e 100644 > --- a/sql/partition_info.cc > +++ b/sql/partition_info.cc > @@ -3164,6 +3164,27 @@ void partition_info::print_debug(const char *str, uint *value) > DBUG_PRINT("info", ("parser: %s", str)); > DBUG_VOID_RETURN; > } > + > +/* > + > + Disabling reading EITS statistics for columns involved in the > + partition list of a table. > + We assume the selecticivity for such columns would be handled > + during partition pruning. > + > +*/ > + > +bool partition_info::disable_eits_for_partitioning_columns(Field *field) The function name is misleading. If a function is named disable_XYZ() I would have expected the code inside to actually take the steps to prevent XYZ from happening. I think it would be more straightforward if it was called bool partition_info::field_is_used_in_partition_expr() const or something like that. Please also declare the function as 'const' and maybe its parameter too. (As discussed on Slack, the function should also search for the field in the list of sub-partitioning columns) > +{ > + uint i; > + for (i= 0; i < num_part_fields; i++) > + { > + if (field->eq(part_field_array[i])) > + return FALSE; > + } > + return TRUE; > +} > + > #else /* WITH_PARTITION_STORAGE_ENGINE */ > /* > For builds without partitioning we need to define these functions > diff --git a/sql/sql_statistics.cc b/sql/sql_statistics.cc > index cb75a5c2176..33a12588434 100644 > --- a/sql/sql_statistics.cc > +++ b/sql/sql_statistics.cc > @@ -3589,6 +3589,12 @@ void set_statistics_for_table(THD *thd, TABLE *table) > (use_stat_table_mode <= COMPLEMENTARY || > !table->stats_is_read || read_stats->cardinality_is_null) ? > table->file->stats.records : read_stats->cardinality; > + > + #ifdef WITH_PARTITION_STORAGE_ENGINE > + if (table->part_info) > + table->used_stat_records= table->file->stats.records; > + #endif This looks cryptic. Please add a comment, something like: /* For partitioned table, EITS statistics is based on data from all partitions. On the other hand, Partition Pruning figures which partitions will be accessed and then computes the estimate of rows in used_partitions. Use the estimate from Partition Pruning as it is typically more precise. Ideally, EITS should provide per-partition statistics but this is not implemented currently. */ > KEY *key_info, *key_info_end; > for (key_info= table->key_info, key_info_end= key_info+table->s->keys; > key_info < key_info_end; key_info++) BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://s.petrunia.net/blog

1 0

Re: [Maria-developers] [Commits] a9c1420233f: MDEV-17589: Stack-buffer-overflow with indexed varchar (utf8) field
by Sergey Petrunia 14 Nov '18

14 Nov '18

Hi Varun, The patch is ok to push. One last question: why is it made against 10.2? I see the MDEV issue has AffectsVersion to start from 10.2, but to my knowledge the code and constants involved were not changed in 10.2, so the issue should be present in the earlier versions as well? On Mon, Nov 12, 2018 at 04:27:08PM +0530, Varun wrote: > revision-id: a9c1420233f3f65aee00a8e7bdd1c8481b0bbc37 (mariadb-10.2.18-74-ga9c1420233f) > parent(s): b290ef8c76e2d7dfbae7a85766694a6fd4648eac > author: Varun Gupta > committer: Varun Gupta > timestamp: 2018-11-12 16:25:25 +0530 > message: > > MDEV-17589: Stack-buffer-overflow with indexed varchar (utf8) field > > Create a new constant MAX_DATA_LENGTH_FOR_KEY. > Replace the value of MAX_KEY_LENGTH to also include the LENGTH and NULL BYTES. > > --- > mysql-test/r/func_group_innodb.result | 23 +++++++++++++++++++++++ > mysql-test/t/func_group_innodb.test | 18 ++++++++++++++++++ > sql/handler.h | 12 +++++++++--- > sql/partition_info.cc | 4 ++-- > sql/sql_const.h | 13 ++++++++++++- > sql/table.cc | 2 +- > 6 files changed, 65 insertions(+), 7 deletions(-) > > diff --git a/mysql-test/r/func_group_innodb.result b/mysql-test/r/func_group_innodb.result > index 52d5922df95..c2bfe9bf81c 100644 > --- a/mysql-test/r/func_group_innodb.result > +++ b/mysql-test/r/func_group_innodb.result > @@ -246,4 +246,27 @@ EXPLAIN SELECT MIN(c) FROM t1 GROUP BY b; > id select_type table type possible_keys key key_len ref rows Extra > 1 SIMPLE t1 range NULL b 263 NULL 3 Using index for group-by > DROP TABLE t1; > +# > +# MDEV-17589: Stack-buffer-overflow with indexed varchar (utf8) field > +# > +CREATE TABLE t1 (v1 varchar(1020), v2 varchar(2), v3 varchar(2), KEY k1 (v3,v2,v1)) ENGINE=InnoDB CHARACTER SET=utf8; > +INSERT INTO t1 VALUES ('king', 'qu','qu'), ('bad','go','go'); > +explain > +SELECT MIN(t1.v1) FROM t1 where t1.v2='qu' and t1.v3='qu'; > +id select_type table type possible_keys key key_len ref rows Extra > +1 SIMPLE NULL NULL NULL NULL NULL NULL NULL Select tables optimized away > +SELECT MIN(t1.v1) FROM t1 where t1.v2='qu' and t1.v3='qu'; > +MIN(t1.v1) > +king > +drop table t1; > +CREATE TABLE t1 (v1 varchar(1024) CHARACTER SET utf8, KEY v1 (v1)) ENGINE=InnoDB; > +INSERT INTO t1 VALUES ('king'), ('bad'); > +explain > +SELECT MIN(x.v1) FROM (SELECT t1.* FROM t1 WHERE t1.v1 >= 'p') x; > +id select_type table type possible_keys key key_len ref rows Extra > +1 SIMPLE NULL NULL NULL NULL NULL NULL NULL No matching min/max row > +SELECT MIN(x.v1) FROM (SELECT t1.* FROM t1 WHERE t1.v1 >= 'p') x; > +MIN(x.v1) > +NULL > +drop table t1; > End of 5.5 tests > diff --git a/mysql-test/t/func_group_innodb.test b/mysql-test/t/func_group_innodb.test > index c62d3d08496..0d24f37363b 100644 > --- a/mysql-test/t/func_group_innodb.test > +++ b/mysql-test/t/func_group_innodb.test > @@ -192,4 +192,22 @@ EXPLAIN SELECT MIN(c) FROM t1 GROUP BY b; > > DROP TABLE t1; > > +--echo # > +--echo # MDEV-17589: Stack-buffer-overflow with indexed varchar (utf8) field > +--echo # > + > +CREATE TABLE t1 (v1 varchar(1020), v2 varchar(2), v3 varchar(2), KEY k1 (v3,v2,v1)) ENGINE=InnoDB CHARACTER SET=utf8; > +INSERT INTO t1 VALUES ('king', 'qu','qu'), ('bad','go','go'); > +explain > +SELECT MIN(t1.v1) FROM t1 where t1.v2='qu' and t1.v3='qu'; > +SELECT MIN(t1.v1) FROM t1 where t1.v2='qu' and t1.v3='qu'; > +drop table t1; > + > +CREATE TABLE t1 (v1 varchar(1024) CHARACTER SET utf8, KEY v1 (v1)) ENGINE=InnoDB; > +INSERT INTO t1 VALUES ('king'), ('bad'); > +explain > +SELECT MIN(x.v1) FROM (SELECT t1.* FROM t1 WHERE t1.v1 >= 'p') x; > +SELECT MIN(x.v1) FROM (SELECT t1.* FROM t1 WHERE t1.v1 >= 'p') x; > +drop table t1; > + > --echo End of 5.5 tests > diff --git a/sql/handler.h b/sql/handler.h > index ed2ef822c88..6745312a17d 100644 > --- a/sql/handler.h > +++ b/sql/handler.h > @@ -378,6 +378,12 @@ enum enum_alter_inplace_result { > #define HA_KEY_NULL_LENGTH 1 > #define HA_KEY_BLOB_LENGTH 2 > > +/* Maximum length of any index lookup key, in bytes */ > + > +#define MAX_KEY_LENGTH (MAX_DATA_LENGTH_FOR_KEY \ > + +(MAX_REF_PARTS \ > + *(HA_KEY_NULL_LENGTH + HA_KEY_BLOB_LENGTH))) > + > #define HA_LEX_CREATE_TMP_TABLE 1U > #define HA_CREATE_TMP_ALTER 8U > > @@ -3421,14 +3427,14 @@ class handler :public Sql_alloc > uint max_key_parts() const > { return MY_MIN(MAX_REF_PARTS, max_supported_key_parts()); } > uint max_key_length() const > - { return MY_MIN(MAX_KEY_LENGTH, max_supported_key_length()); } > + { return MY_MIN(MAX_DATA_LENGTH_FOR_KEY, max_supported_key_length()); } > uint max_key_part_length() const > - { return MY_MIN(MAX_KEY_LENGTH, max_supported_key_part_length()); } > + { return MY_MIN(MAX_DATA_LENGTH_FOR_KEY, max_supported_key_part_length()); } > > virtual uint max_supported_record_length() const { return HA_MAX_REC_LENGTH; } > virtual uint max_supported_keys() const { return 0; } > virtual uint max_supported_key_parts() const { return MAX_REF_PARTS; } > - virtual uint max_supported_key_length() const { return MAX_KEY_LENGTH; } > + virtual uint max_supported_key_length() const { return MAX_DATA_LENGTH_FOR_KEY; } > virtual uint max_supported_key_part_length() const { return 255; } > virtual uint min_record_length(uint options) const { return 1; } > > diff --git a/sql/partition_info.cc b/sql/partition_info.cc > index f96882552fa..8be3f878f8d 100644 > --- a/sql/partition_info.cc > +++ b/sql/partition_info.cc > @@ -1713,12 +1713,12 @@ bool partition_info::check_partition_field_length() > > for (i= 0; i < num_part_fields; i++) > store_length+= get_partition_field_store_length(part_field_array[i]); > - if (store_length > MAX_KEY_LENGTH) > + if (store_length > MAX_DATA_LENGTH_FOR_KEY) > DBUG_RETURN(TRUE); > store_length= 0; > for (i= 0; i < num_subpart_fields; i++) > store_length+= get_partition_field_store_length(subpart_field_array[i]); > - if (store_length > MAX_KEY_LENGTH) > + if (store_length > MAX_DATA_LENGTH_FOR_KEY) > DBUG_RETURN(TRUE); > DBUG_RETURN(FALSE); > } > diff --git a/sql/sql_const.h b/sql/sql_const.h > index 7395ae3c08a..0c781855302 100644 > --- a/sql/sql_const.h > +++ b/sql/sql_const.h > @@ -33,7 +33,18 @@ > #define MAX_SYS_VAR_LENGTH 32 > #define MAX_KEY MAX_INDEXES /* Max used keys */ > #define MAX_REF_PARTS 32 /* Max parts used as ref */ > -#define MAX_KEY_LENGTH 3072 /* max possible key */ > + > +/* > + Maximum length of the data part of an index lookup key. > + > + The "data part" is defined as the value itself, not including the > + NULL-indicator bytes or varchar length bytes ("the Extras"). We need this > + value because there was a bug where length of the Extras were not counted. > + > + You probably need MAX_KEY_LENGTH, not this constant. > +*/ > + > +#define MAX_DATA_LENGTH_FOR_KEY 3072 > #if SIZEOF_OFF_T > 4 > #define MAX_REFLENGTH 8 /* Max length for record ref */ > #else > diff --git a/sql/table.cc b/sql/table.cc > index 2b278288784..5b163d501d1 100644 > --- a/sql/table.cc > +++ b/sql/table.cc > @@ -2169,7 +2169,7 @@ int TABLE_SHARE::init_from_binary_frm_image(THD *thd, bool write, > uint pk_part_length= key_first_info->key_part[i].store_length; > if (keyinfo->ext_key_part_map & 1<<i) > { > - if (ext_key_length + pk_part_length > MAX_KEY_LENGTH) > + if (ext_key_length + pk_part_length > MAX_DATA_LENGTH_FOR_KEY) > { > add_keyparts_for_this_key= i; > break; > _______________________________________________ > commits mailing list > commits(a)mariadb.org > https://lists.askmonty.org/cgi-bin/mailman/listinfo/commits -- BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://s.petrunia.net/blog

1 0

Re: [Maria-developers] a04c60e6c99: MDEV-16849 Extending indexed VARCHAR column should be instantaneous
by Sergei Golubchik 13 Nov '18

13 Nov '18

Hi, Thirunarayanan! On Nov 13, Thirunarayanan Balathandayuthapani wrote: > revision-id: a04c60e6c99097c9b52f582b2af891f48b8f662d (mariadb-10.2.18-40-ga04c60e6c99) > parent(s): ab1ce2204e959bea596817494e932754ab5cbe88 > author: Thirunarayanan Balathandayuthapani <thiru(a)mariadb.com> > committer: Thirunarayanan Balathandayuthapani <thiru(a)mariadb.com> > timestamp: 2018-11-12 16:30:34 +0530 > message: > > MDEV-16849 Extending indexed VARCHAR column should be instantaneous > > Analysis: > ======== > Increasing the length of the indexed varchar column is not an instant operation for > innodb. > > diff --git a/mysql-test/suite/innodb/r/alter_varchar_change.result b/mysql-test/suite/innodb/r/alter_varchar_change.result > new file mode 100644 > index 00000000000..b60bd7847db > --- /dev/null > +++ b/mysql-test/suite/innodb/r/alter_varchar_change.result > @@ -0,0 +1,269 @@ ... > +CALL get_table_id("test/t1", @tbl1_id); > +SELECT @tbl1_id - @tbl_id; > +@tbl1_id - @tbl_id > +1 That's a bit fragile, you test that table id was changed exactly by 1. I don't think the exact difference matters here, so it'd be safer to test for `SELECT @tbl1_id = @tbl_id` (here and everywhere). > +SHOW CREATE TABLE t1; > diff --git a/sql/sql_table.cc b/sql/sql_table.cc > index 02780e7d1e0..b9668ad0979 100644 > --- a/sql/sql_table.cc > +++ b/sql/sql_table.cc > @@ -6687,18 +6687,29 @@ static bool fill_alter_inplace_info(THD *thd, > key_part < end; > key_part++, new_part++) > { > + new_field= get_field_by_index(alter_info, new_part->fieldnr); > + old_field= table->field[key_part->fieldnr - 1]; > /* > + If there is a change in index length due to column expansion > + like varchar(X) changed to varchar(X + N) and has a compatible > + packed data representation, we mark it for fast/INPLACE change > + in index definition. InnoDB supports INPLACE for this cases no tabs in the server code, please. If you're using vim, I can show you my config that has different formatting style for innodb and the rest of the server :) > + > Key definition has changed if we are using a different field or > - if the used key part length is different. It makes sense to > - check lengths first as in case when fields differ it is likely > - that lengths differ too and checking fields is more expensive > - in general case. > + if the user key part length is different. > */ > - if (key_part->length != new_part->length) > + if (key_part->length != new_part->length && > + new_field->field != NULL && can new_field->field be NULL here? when? > + old_field->pack_length() < new_field->pack_length && Do you need it? I believe IS_EQUAL_PACK_LENGTH includes this check. > + (key_part->field->is_equal((Create_field*) new_field) > + == IS_EQUAL_PACK_LENGTH)) > + { > + ha_alter_info->handler_flags |= > + Alter_inplace_info::ALTER_COLUMN_INDEX_LENGTH; > + } > + else if (key_part->length != new_part->length) > goto index_changed; > > - new_field= get_field_by_index(alter_info, new_part->fieldnr); > - Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

1 0