- developers - lists.mariadb.org

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

Re: [Maria-developers] 031efde365c: MDEV-16217: Assertion `!table || (!table->read_set || bitmap_is_set(table->read_set, field_index))' failed in Field_num::get_date
by Sergei Golubchik 13 Nov '18

13 Nov '18

Hi, Oleksandr! On Nov 07, Oleksandr Byelkin wrote: > revision-id: 031efde365c674dbdbaada95aa6d42a4274db438 (mariadb-10.2.18-65-g031efde365c) > parent(s): 89f948c766721a26e110bc9da0ca5ebc20f65112 > author: Oleksandr Byelkin > committer: Oleksandr Byelkin > timestamp: 2018-11-07 14:29:47 +0100 > message: > > MDEV-16217: Assertion `!table || (!table->read_set || bitmap_is_set(table->read_set, field_index))' failed in Field_num::get_date > > - clean up DEFAULT() to work only with default value and correctly print > itself. > - fix of DBUG_ASSERT about fields read/write > - fix of field marking for write based really on the thd->mark_used_columns flag > diff --git a/sql/field.cc b/sql/field.cc > index caa84dc9932..6cd8940a893 100644 > --- a/sql/field.cc > +++ b/sql/field.cc > @@ -70,8 +70,21 @@ const char field_separator=','; > #define BLOB_PACK_LENGTH_TO_MAX_LENGH(arg) \ > ((ulong) ((1LL << MY_MIN(arg, 4) * 8) - 1)) > > -#define ASSERT_COLUMN_MARKED_FOR_READ DBUG_ASSERT(!table || (!table->read_set || bitmap_is_set(table->read_set, field_index))) > -#define ASSERT_COLUMN_MARKED_FOR_WRITE_OR_COMPUTED DBUG_ASSERT(is_stat_field || !table || (!table->write_set || bitmap_is_set(table->write_set, field_index) || (table->vcol_set && bitmap_is_set(table->vcol_set, field_index)))) > +// Column marked for read or the field set to read out or record[0] or [1] > +#define ASSERT_COLUMN_MARKED_FOR_READ \ > + DBUG_ASSERT(!table || \ > + (!table->read_set || \ > + bitmap_is_set(table->read_set, field_index) || \ > + (!(ptr >= table->record[0] && \ > + ptr < table->record[0] + table->s->reclength)))) > + > +#define ASSERT_COLUMN_MARKED_FOR_WRITE_OR_COMPUTED \ > + DBUG_ASSERT(is_stat_field || !table || \ > + (!table->write_set || \ > + bitmap_is_set(table->write_set, field_index) || \ > + (!(ptr >= table->record[0] && \ > + ptr < table->record[0] + table->s->reclength))) || \ > + (table->vcol_set && bitmap_is_set(table->vcol_set, field_index))) Do you need this ptr check in ASSERT_COLUMN_MARKED_FOR_WRITE_OR_COMPUTED ? I'd expect you only needing it in ASSERT_COLUMN_MARKED_FOR_READ. > > #define FLAGSTR(S,F) ((S) & (F) ? #F " " : "") > > diff --git a/sql/item.cc b/sql/item.cc > index 2adec33491b..6828a74f9ff 100644 > --- a/sql/item.cc > +++ b/sql/item.cc > @@ -8813,15 +8824,19 @@ bool Item_default_value::fix_fields(THD *thd, Item **items) > goto error; > memcpy((void *)def_field, (void *)field_arg->field, > field_arg->field->size_of()); > - IF_DBUG(def_field->is_stat_field=1,); // a hack to fool ASSERT_COLUMN_MARKED_FOR_WRITE_OR_COMPUTED > + // If non-constant default value expression > if (def_field->default_value && def_field->default_value->flags) > { > uchar *newptr= (uchar*) thd->alloc(1+def_field->pack_length()); > if (!newptr) > goto error; > + /* > + Even if DEFAULT() do not read tables fields, the default value > + expression can do it. > + */ > fix_session_vcol_expr_for_read(thd, def_field, def_field->default_value); > if (thd->mark_used_columns != MARK_COLUMNS_NONE) > - def_field->default_value->expr->walk(&Item::register_field_in_read_map, 1, 0); > + def_field->default_value->expr->update_used_tables(); what's the difference? > def_field->move_field(newptr+1, def_field->maybe_null() ? newptr : 0, 1); > } > else > diff --git a/sql/item.h b/sql/item.h > index 8d02d981d38..5866f328f38 100644 > --- a/sql/item.h > +++ b/sql/item.h > @@ -5229,6 +5229,11 @@ class Item_default_value : public Item_field > return false; > } > table_map used_tables() const; > + virtual void update_used_tables() > + { > + if (field && field->default_value) > + field->default_value->expr->walk(&Item::register_field_in_read_map, 1, 0); why not field->default_value->expr->update_used_tables() ? > + } > Field *get_tmp_table_field() { return 0; } > Item *get_tmp_table_item(THD *thd) { return this; } > Item_field *field_for_view_update() { return 0; } > diff --git a/sql/sql_base.cc b/sql/sql_base.cc > index c282db42fdd..0deb5ec1362 100644 > --- a/sql/sql_base.cc > +++ b/sql/sql_base.cc > @@ -5737,7 +5737,7 @@ find_field_in_table_ref(THD *thd, TABLE_LIST *table_list, > TABLE *table= field_to_set->table; > if (thd->mark_used_columns == MARK_COLUMNS_READ) > bitmap_set_bit(table->read_set, field_to_set->field_index); > - else > + else if (thd->mark_used_columns == MARK_COLUMNS_WRITE) > bitmap_set_bit(table->write_set, field_to_set->field_index); what does it affect? > } > } Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

2 3

Re: [Maria-developers] 031efde365c: MDEV-16217: Assertion `!table || (!table->read_set || bitmap_is_set(table->read_set, field_index))' failed in Field_num::get_date
by Sergei Golubchik 12 Nov '18

12 Nov '18

Hi, Oleksandr! On Nov 12, Oleksandr Byelkin wrote: > > On Nov 07, Oleksandr Byelkin wrote: > > > revision-id: 031efde365c674dbdbaada95aa6d42a4274db438 (mariadb-10.2.18-65-g031efde365c) > > > parent(s): 89f948c766721a26e110bc9da0ca5ebc20f65112 > > > author: Oleksandr Byelkin > > > committer: Oleksandr Byelkin > > > timestamp: 2018-11-07 14:29:47 +0100 > > > message: > > > > > > MDEV-16217: Assertion `!table || (!table->read_set || > > bitmap_is_set(table->read_set, field_index))' failed in Field_num::get_date > > > > > > - clean up DEFAULT() to work only with default value and correctly print > > > itself. > > > - fix of DBUG_ASSERT about fields read/write > > > - fix of field marking for write based really on the thd->mark_used_columns flag > > > > > diff --git a/sql/field.cc b/sql/field.cc > > > index caa84dc9932..6cd8940a893 100644 > > > --- a/sql/field.cc > > > +++ b/sql/field.cc > > > @@ -70,8 +70,21 @@ const char field_separator=','; > > > #define BLOB_PACK_LENGTH_TO_MAX_LENGH(arg) \ > > > ((ulong) ((1LL << MY_MIN(arg, 4) * 8) - 1)) > > > > > > -#define ASSERT_COLUMN_MARKED_FOR_READ DBUG_ASSERT(!table || > > (!table->read_set || bitmap_is_set(table->read_set, field_index))) > > > -#define ASSERT_COLUMN_MARKED_FOR_WRITE_OR_COMPUTED > > DBUG_ASSERT(is_stat_field || !table || (!table->write_set || > > bitmap_is_set(table->write_set, field_index) || (table->vcol_set && > > bitmap_is_set(table->vcol_set, field_index)))) > > > +// Column marked for read or the field set to read out or record[0] or > > [1] > > > +#define ASSERT_COLUMN_MARKED_FOR_READ \ > > > + DBUG_ASSERT(!table || \ > > > + (!table->read_set || \ > > > + bitmap_is_set(table->read_set, field_index) || \ > > > + (!(ptr >= table->record[0] && \ > > > + ptr < table->record[0] + table->s->reclength)))) > > > + > > > +#define ASSERT_COLUMN_MARKED_FOR_WRITE_OR_COMPUTED \ > > > + DBUG_ASSERT(is_stat_field || !table || \ > > > + (!table->write_set || \ > > > + bitmap_is_set(table->write_set, field_index) || \ > > > + (!(ptr >= table->record[0] && \ > > > + ptr < table->record[0] + table->s->reclength))) || \ > > > + (table->vcol_set && bitmap_is_set(table->vcol_set, > > field_index))) > > > > Do you need this ptr check in ASSERT_COLUMN_MARKED_FOR_WRITE_OR_COMPUTED ? > > I'd expect you only needing it in ASSERT_COLUMN_MARKED_FOR_READ. > > I'd prefer to have them symmetric Right, but these macros already have non-symmetric names and non-symmetric definition - the second checks for vcol_set. The way you did it only makes the reader to pause and think in what case one can get `ptr` outside of table->record[0] for writes. Because your assert says "yes, it's ok to write into the column if ptr is outside of table->record[0]". If ptr cannot possibly be outside of record[0] for write, then it shouldn't be in the assert. > > > #define FLAGSTR(S,F) ((S) & (F) ? #F " " : "") > > > > > > diff --git a/sql/sql_base.cc b/sql/sql_base.cc > > > index c282db42fdd..0deb5ec1362 100644 > > > --- a/sql/sql_base.cc > > > +++ b/sql/sql_base.cc > > > @@ -5737,7 +5737,7 @@ find_field_in_table_ref(THD *thd, TABLE_LIST > > *table_list, > > > TABLE *table= field_to_set->table; > > > if (thd->mark_used_columns == MARK_COLUMNS_READ) > > > bitmap_set_bit(table->read_set, field_to_set->field_index); > > > - else > > > + else if (thd->mark_used_columns == MARK_COLUMNS_WRITE) > > > bitmap_set_bit(table->write_set, field_to_set->field_index); > > > > what does it affect? > > > > enum enum_mark_columns > { MARK_COLUMNS_NONE, MARK_COLUMNS_READ, MARK_COLUMNS_WRITE}; > > MARK_COLUMNS_NONE really used in the code, I remember that something was > marked incorrectly (it is how I found it) but what it is too late to ask > after many month as fix is done. this code is inside of if (thd->mark_used_columns != MARK_COLUMNS_NONE) so I don't see what your change could possibly do. Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

1 0

Re: [Maria-developers] bc7d600a27f: MDEV-15951 system versioning by trx id doesn't work with partitioning
by Sergei Golubchik 12 Nov '18

12 Nov '18

Hi, Nikita! On Nov 12, Nikita Malyavin wrote: > revision-id: bc7d600a27f4c6c14deea5f1d674c621f85b23ba (versioning-1.0.5-141-gbc7d600a27f) > parent(s): 90b292ce31170be33796b07723ffcef56bf8a679 > author: Nikita Malyavin <nikitamalyavin(a)gmail.com> > committer: Nikita Malyavin <nikitamalyavin(a)gmail.com> > timestamp: 2018-09-21 22:42:03 +1000 > message: > > MDEV-15951 system versioning by trx id doesn't work with partitioning > > Fix partitioning for trx_id-versioned tables. > `partition by hash`, `range` and others now work. > `partition by system_time` is forbidden. > Currently we cannot use row_start and row_end in `partition by`, because > insertion of versioned field is done by engine's handler, as well as > row_start/row_end's value set up, which is a transaction id -- so it's > also forbidden. > > The drawback is that it's now impossible to use `partition by key()` > without parameters for such tables, because it references row_start and > row_end implicitly. > > * add handler::vers_can_native() > * drop Table_scope_and_contents_source_st::vers_native() > * drop partition_element::find_engine_flag as unused > * forbid versioning partitioning for trx_id as not supported > * adopt vers tests for trx_id partitioning > * forbid any row_end referencing in `partition by` clauses, > including implicit `by key()` > > --- > mysql-test/suite/versioning/r/partition.result | 79 ++++++++++++++++-- > .../suite/versioning/t/partition.combinations | 3 + > mysql-test/suite/versioning/t/partition.test | 97 ++++++++++++++++++++-- > sql/ha_partition.h | 10 +++ > sql/handler.cc | 34 ++------ > sql/handler.h | 8 +- > sql/partition_element.h | 15 ---- > sql/sql_class.h | 4 +- > sql/sql_partition.cc | 9 ++ > sql/sql_table.cc | 6 +- > sql/table.cc | 4 +- > sql/temporary_tables.cc | 10 ++- > 12 files changed, 214 insertions(+), 65 deletions(-) > > diff --git a/mysql-test/suite/versioning/r/partition.result b/mysql-test/suite/versioning/r/partition.result > index bfec0ce2d4b..cf4203b7949 100644 > --- a/mysql-test/suite/versioning/r/partition.result > +++ b/mysql-test/suite/versioning/r/partition.result > @@ -1,6 +1,6 @@ > set system_versioning_alter_history=keep; > # Check conventional partitioning on temporal tables > -create table t1 (x int) > +create or replace table t1 (x int, ROW_START, ROW_END, period for system_time(row_start, row_end)) ouch. that looks very weird. please, only replace the type, not the whole column definition. Like everywhere else: --replace_result $sys_datatype_expl SYS_DATATYPE > with system versioning > partition by range columns (x) ( > partition p0 values less than (100), > @@ -501,6 +504,72 @@ set timestamp=1523466002.799571; > insert into t1 values (11),(12); > set timestamp=1523466004.169435; > delete from t1 where pk in (11, 12); > +# MDEV-15951 system versioning by trx id doesn't work with partitioning > +# currently trx_id does not support partitioning by system_time > +create or replace table t1( > +i int, > +row_start bigint unsigned generated always as row start, > +row_end bigint unsigned generated always as row end, > +period for system_time(row_start, row_end) > +) engine=InnoDB with system versioning partition by system_time ( if you have tests with innodb and trx_id, please, put them in a separate test file, say, partition_innodb.test. partition.test is for common tests that are repeated three times, with myisam, innodb, timestamps, and trx_id. There is no need to repeat your test three times. > diff --git a/mysql-test/suite/versioning/t/partition.combinations b/mysql-test/suite/versioning/t/partition.combinations > index 4d73ef5a5ea..561c5656929 100644 > --- a/mysql-test/suite/versioning/t/partition.combinations > +++ b/mysql-test/suite/versioning/t/partition.combinations > @@ -1,5 +1,8 @@ > [timestamp] > default-storage-engine=innodb > > +[trx_id] > +default-storage-engine=innodb > + > [myisam] > default-storage-engine=myisam But then you don't need partition.combinations at all, just add --source engines.inc like other tests do. > diff --git a/sql/ha_partition.h b/sql/ha_partition.h > index 8a251016703..ef49011b7f8 100644 > --- a/sql/ha_partition.h > +++ b/sql/ha_partition.h > @@ -413,6 +413,16 @@ class ha_partition :public handler > > virtual void return_record_by_parent(); > > + virtual bool vers_can_native(THD *thd) > + { > + // PARTITION BY SYSTEM_TIME is not supported for now > + bool can= !thd->lex->part_info > + || thd->lex->part_info->part_type != VERSIONING_PARTITION; > + for (uint i= 0; i < m_tot_parts && can; i++) > + can= can && m_file[i]->vers_can_native(thd); > + return can; > + } > + > /* > ------------------------------------------------------------------------- > MODULE create/delete handler object > diff --git a/sql/handler.cc b/sql/handler.cc > index 306b0868d15..a7962039463 100644 > --- a/sql/handler.cc > +++ b/sql/handler.cc > @@ -4938,6 +4938,7 @@ int ha_create_table(THD *thd, const char *path, > !create_info->tmp_table(); > > share.frm_image= frm; > + share.orig_table_name= table_name; why? if orig_table_name is NULL, error_table_name() will use table_name, so there's never need to make orig_table_name==table_name. and I don't understand all your other manipulations with orig_table_name. Could you please explain that? > > // open an frm image > if (share.init_from_binary_frm_image(thd, write_frm_now, > @@ -6952,29 +6953,6 @@ bool Vers_parse_info::fix_implicit(THD *thd, Alter_info *alter_info) > return false; > } > > -bool Table_scope_and_contents_source_st::vers_native(THD *thd) const > -{ > - if (ha_check_storage_engine_flag(db_type, HTON_NATIVE_SYS_VERSIONING)) > - return true; > - > -#ifdef WITH_PARTITION_STORAGE_ENGINE > - partition_info *info= thd->work_part_info; > - if (info && !(used_fields & HA_CREATE_USED_ENGINE)) > - { > - if (handlerton *hton= info->default_engine_type) > - return ha_check_storage_engine_flag(hton, HTON_NATIVE_SYS_VERSIONING); > - > - List_iterator_fast<partition_element> it(info->partitions); > - while (partition_element *partition_element= it++) > - { > - if (partition_element->find_engine_flag(HTON_NATIVE_SYS_VERSIONING)) > - return true; > - } > - } > -#endif > - return false; > -} what was wrong with that (and all other changed code in this file) ? > bool Table_scope_and_contents_source_st::vers_fix_system_fields( > THD *thd, Alter_info *alter_info, const TABLE_LIST &create_table, > bool create_select) Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

1 0

Re: [Maria-developers] MDEV-17292.
by Sergei Golubchik 12 Nov '18

12 Nov '18

Hi, Alexey! On Nov 09, Alexey Botchkov wrote: > Hi, Sergei. > > I'm not sure what we want to do about this issue > https://jira.mariadb.org/browse/MDEV-17292 > > It's about pam_user_map.so packaging. > So we do like this compiled an put into the 'appropriate' > directory as /lib/security during install. Right? Yes. Not sure whether it should be hard-coded /lib/security or there's some distro-specific way of determining the correct path. Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

1 0