November 2016 - developers - lists.mariadb.org

[Maria-developers] Windows line endings in MariaDB sources again (
by Otto Kekäläinen 24 Jul '19

24 Jul '19

Hello! While trying to release 10.0.26 for Debian and Ubuntu I noticed git chokes again on Windows line endings. As our .gitattributes states that by default git should clean up the line endings, why do we keep getting new Windows line endings in new code? mariadb-10.0$ cat .gitattributes # Set the default behavior, in case people don't have core.autocrlf set. * text=auto ... storage/connect/mysql-test/connect/std_data/*.txt -text mariadb-10.0.26$ find . | xargs file | grep CRLF ./storage/connect/mysql-test/connect/std_data/boyswin.txt: ASCII text, with CRLF line terminators ./storage/connect/mysql-test/connect/std_data/expenses.txt: ASCII text, with CRLF line terminators ./storage/connect/mysql-test/connect/std_data/emp.txt: ASCII text, with CRLF line terminators ./mysql-test/std_data/loaddata7.dat: ASCII text, with CRLF line terminators ./mysql-test/r/loadxml.result: ASCII text, with CRLF, LF line terminators ./mysql-test/r/perror-win.result: ASCII text, with CRLF, LF line terminators ./mysql-test/r/mysql_binary_mode.result: ASCII English text, with CRLF, LF line terminators ./mysql-test/r/func_regexp_pcre.result: UTF-8 Unicode C++ program text, with CRLF, CR, LF line terminators ./pcre/testdata/grepoutputN: ASCII text, with CRLF, CR, LF line terminators ./pcre/testdata/greppatN4: ASCII text, with CRLF line terminators This leads for me to: dpkg-source: info: building mariadb-10.0 using existing ./mariadb-10.0_10.0.26.orig.tar.gz dpkg-source: info: local changes detected, the modified files are: mariadb-10.0/storage/connect/mysql-test/connect/std_data/boyswin.txt mariadb-10.0/storage/connect/mysql-test/connect/std_data/emp.txt mariadb-10.0/storage/connect/mysql-test/connect/std_data/expenses.txt dpkg-source: error: aborting due to unexpected upstream changes, see /tmp/mariadb-10.0_10.0.26-1.diff.Yzwwhw Which means I have to do extra work with importing the upstream sources.. - Otto

2 4

[Maria-developers] Please review MDEV-11292 Item_func_sp::fix_length_and_dec() does not set collation derivation properly
by Alexander Barkov 20 Nov '17

20 Nov '17

Hello Alexey, Please review a patch for MDEV-11292 (for 10.3). Thanks.

2 2

[Maria-developers] Fix for TokuDB and parallel replication
by Kristian Nielsen 09 Dec '16

09 Dec '16

Parallel replication so far did not work well with TokuDB, as some people who tried it found out. I have now pushed to 10.1 some patches to solve the problems. There are two main fixes: 1. Fix some races where a waiting transaction would miss its wakeup and get a lock timeout on a waiting row lock, even though the lock was released by the holding transaction. This fix is due to great work by Rich Prohaska. This problem is actually not specific to replication, normal transactions on a master will experience it too. But it hurts replication a lot, since replication must commit transactions in order, so one stalled transaction stalls all following transactions as well. 2. Implement the conflict detection and handling necessary for optimistic parallel replication to work. This basically implements thd_rpl_deadlock_check() and hton->kill_query methods. This should solve the problems where optimistic parallel replication with TokuDB breaks with lock wait timeouts. If someone wants to test it, I have made a tree available with just these fixes on top of MariaDB 10.1.19: https://github.com/knielsen/server/tree/tokudb_optimistic_parallel_replicat… The fix should appear in 10.1.20 eventually. The first part of the patch has also been submitted by Rich to upstream. When this is (hopefully) merged upstream, and upstream merged into MariaDB, the MariaDB version of the fix should be replaced with the Percona one. I tried making the MariaDB version of the fix identical to Rich's pull request and keeping it in a separate commit, so this should hopefully be simple to do when the time comes. - Kristian.

3 9

Re: [Maria-developers] Working on spider patches, MDEV-7698
by Michael Widenius 05 Dec '16

05 Dec '16

Hi! Now working on MDEV-7700 Spiral patch 002_mariadb-10.0.15.spider.diff +++ ./002_mariadb-10.1.8-spider/sql/ha_partition.cc 2015-10-14 01:48:53.392665313 +0900 @@ -327,7 +327,9 @@ void ha_partition::init_handler_variable m_file_buffer= NULL; m_name_buffer_ptr= NULL; m_engine_array= NULL; +/* m_connect_string= NULL; +*/ m_file= NULL; m_file_tot_parts= 0; m_reorged_file= NULL; @@ -1516,4 +1518,6 @@ int ha_partition::prepare_new_partition( if ((error= set_up_table_before_create(tbl, part_name, create_info, p_elem))) goto error_create; +/* tbl->s->connect_string = p_elem->connect_string; +*/ I don't think this it will work removing the usage of p_elem->connect_string This is because each partition may have a different connect string. Here is an example from fedarated_partion.test: eval create table t1 (s1 int primary key) engine=federated partition by list (s1) (partition p1 values in (1,3) connection='mysql://root@127.0.0.1:$SLAVE_MYPORT/federated/t1_1', partition p2 values in (2,4) connection='mysql://root@127.0.0.1:$SLAVE_MYPORT/federated/t1_2'); The above works in mariadb 10.2 but not in your spider tree. >From this patch I will, for now, only take the code related to HA_EXTRA_WRITE_CAN_REPLACE HA_EXTRA_WRITE_CANNOT_REPLACE --- ./001_mariadb-10.1.8-partition_cond_push/sql/sql_plugin.cc 2015-10-13 23:49:10.188129839 +0900 +++ ./002_mariadb-10.1.8-spider/sql/sql_plugin.cc 2015-10-14 01:48:54.296665317 +0900 @@ -2757,6 +2757,7 @@ static void update_func_str(THD *thd, st *(char**) tgt= my_strdup(value, MYF(0)); else *(char**) tgt= 0; - my_free(old); + if (old) + my_free(old); As my_free is safe to call with NULL, the above is not needed diff -Narup ./001_mariadb-10.1.8-partition_cond_push/sql/sql_priv.h ./002_mariadb-10.1.8-spider/sql/sql_priv.h --- ./001_mariadb-10.1.8-partition_cond_push/sql/sql_priv.h 2015-10-13 23:49:10.189129839 +0900 +++ ./002_mariadb-10.1.8-spider/sql/sql_priv.h 2015-10-14 01:48:54.642665315 +0900 @@ -27,6 +27,8 @@ #ifndef SQL_PRIV_INCLUDED #define SQL_PRIV_INCLUDED +#define PLUGIN_VAR_CAN_MEMALLOC + #ifndef MYSQL_CLIENT The above is not needed, as all code that is testing this is doing: #if defined(MARIADB_BASE_VERSION) && MYSQL_VERSION_ID >= 100000 Which is always true in MariaDB 10.x Regards, Monty

2 7

[Maria-developers] Working on spider patches, MDEV-7701
by Michael Widenius 05 Dec '16

05 Dec '16

Hi! Next patch, MDEV 7701 +++ ./003_mariadb-10.2.0-vp/sql/ha_partition.cc 2016-05-05 21:25:04.289731712 +0900 @@ -7238,13 +7239,30 @@ int ha_partition::extra(enum ha_extra_fu } /* Category 9) Operations only used by MERGE */ case HA_EXTRA_ADD_CHILDREN_LIST: + DBUG_RETURN(loop_extra(operation)); case HA_EXTRA_ATTACH_CHILDREN: + int result; + handler **file; + if ((result = loop_extra(operation))) + DBUG_RETURN(result); + m_num_locks = 0; + additional_table_flags = + (HA_CAN_BG_SEARCH | HA_CAN_BG_INSERT | HA_CAN_BG_UPDATE); + file = m_file; + do + { + m_num_locks += (*file)->lock_count(); + additional_table_flags &= ~((ulonglong) ((*file)->ha_table_flags() ^ + (HA_CAN_BG_SEARCH | HA_CAN_BG_INSERT | HA_CAN_BG_UPDATE))); The above test could be simple: additional_table_flags &= (ulonglong) ((*file)->ha_table_flags(); Should be the same thing. I did however remove this code, as HA_CAN_BG... are not used anywhere in the current spider code. See comment below. I also changed so that m_num_locks always contains the upper limit number of locks needed. Without this change, the call to lock_count() would have returneed m_total_parts * 'new-counted-locks' which would be been way too much. @@ -9123,6 +9141,19 @@ const COND *ha_partition::cond_push(cons handler **file= m_file; COND *res_cond = NULL; DBUG_ENTER("ha_partition::cond_push"); +#ifdef HANDLER_HAS_TOP_TABLE_FIELDS + if (set_top_table_fields) + { + do + { + if ((*file)->set_top_table_and_fields(top_table, + top_table_field, + top_table_fields)) + DBUG_RETURN(cond); + } while (*(++file)); + file= m_file; + } +#endif I added this without the define, but I merged it to the loop for cond_push. Instead of having defines in the sql code, I will fix that in spider and vp that I will set the proper defines based on the MariaDB version. However, I would like to have a clear comment the purpose of the function set_top_table_and_fields(). struct st_mysql_storage_engine partition_storage_engine= { MYSQL_HANDLERTON_INTERFACE_VERSION }; +++ ./003_mariadb-10.2.0-vp/sql/handler.h 2016-05-05 21:25:04.314731713 +0900 @@ -255,6 +257,11 @@ enum enum_alter_inplace_result { */ #define HA_BINLOG_FLAGS (HA_BINLOG_ROW_CAPABLE | HA_BINLOG_STMT_CAPABLE) +#define HA_CAN_BG_SEARCH (1LL << 45) +#define HA_CAN_BG_INSERT (1LL << 46) +#define HA_CAN_BG_UPDATE (1LL << 47) +#define HA_CAN_MULTISTEP_MERGE (1LL << 48) Do you really need HA_CAN_BG_XXX flags? I checked your spider tree and this is not used anywhere, so I think we should leave them out for now. If we don't need these, we can remove the loop for setting them in HA_EXTRA_ATTACH_CHILDREN too. If we need them, I would like to have a comment for why they are needed and when we will need then add these in a separate patch. @@ -3525,6 +3540,29 @@ public: Pops the top if condition stack, if stack is not empty. */ virtual void cond_pop() { return; }; + virtual int set_top_table_and_fields(TABLE *top_table, + Field **top_table_field, + uint top_table_fields) + { + if (!set_top_table_fields) + { + set_top_table_fields = TRUE; + this->top_table = top_table; + this->top_table_field = top_table_field; + this->top_table_fields = top_table_fields; + } + return 0; + } Why is this int and not void ? Do you have in mind that this may be possible to fail in the future? If it's never going to change, then better to do this void. I will make it void for now and change the relevant code. We can make it int again if needed in the future. diff -Narup ./002_mariadb-10.2.0-spider/sql/sql_base.cc ./003_mariadb-10.2.0-vp/sql/sql_base.cc --- ./002_mariadb-10.2.0-spider/sql/sql_base.cc 2016-04-17 02:47:27.000000000 +0900 +++ ./003_mariadb-10.2.0-vp/sql/sql_base.cc 2016-05-05 21:25:04.319731713 +0900 @@ -1475,6 +1475,10 @@ unique_table(THD *thd, TABLE_LIST *table table= table->find_table_for_update(); - if (table->table && table->table->file->ht->db_type == DB_TYPE_MRG_MYISAM) - { - TABLE_LIST *child; + if (table->table && + ( + table->table->file->ht->db_type == DB_TYPE_MRG_MYISAM || + (table->table->file->ha_table_flags() & HA_CAN_MULTISTEP_MERGE) + ) + ) + continue I added HA_CAN_MULTISTEP_MERGE as a flag to DB_TYPE_MRG_MYISAM, which allwoed me to make the above tests and the following identical ones much simpler. This is now pushed into 10.2-spider Regards, Monty

2 2

[Maria-developers] Review form MDEV-11297 patch
by Sergey Petrunia 30 Nov '16

30 Nov '16

Hi Varun, Please find my review comments below. We'll need another review pass once these are addressed. > commit e9d1969a9758801674828f26431306350bb4c0ab > Author: Varun <varunraiko1803(a)gmail.com> > Date: Mon Nov 28 15:09:03 2016 +0530 > > LIMIT Clause without optimisation added to GROUP_CONCAT > Please mention MDEV-11297 in the commit comment. The idea is that it should be possible to search for MDEV number in 'git log' output and find all commits with the code for the issue. > diff --git a/mysql-test/t/group_concat_limit.test b/mysql-test/t/group_concat_limit.test > index 82baf54..7230e80 100644 Ok this is .test file. Where is the .result file, did you forget to add it? Please also add testcases where the GROUP_CONCAT's argument is NULL. > --- a/mysql-test/t/group_concat_limit.test > +++ b/mysql-test/t/group_concat_limit.test > @@ -18,30 +18,30 @@ insert into t1 values (3,7,"E","c"); > > # Test of MySQL simple request > > -select grp,group_concat(c limit 1 ) from t1 group by grp; > +select grp,group_concat(c limit 0 ) from t1 group by grp; > select grp,group_concat(c limit 1,1 ) from t1 group by grp; > > select grp,group_concat(c limit 1,10 ) from t1 group by grp; > select grp,group_concat(distinct c limit 1,10 ) from t1 group by grp; > > select grp,group_concat(c order by a) from t1 group by grp; > -elect grp,group_concat(c order by a limit 2 ) from t1 group by grp; > +select grp,group_concat(c order by a limit 2 ) from t1 group by grp; > > select grp,group_concat(c order by a limit 1,1 ) from t1 group by grp; > select grp,group_concat(c order by a limit 1,2 ) from t1 group by grp; > select grp,group_concat(c order by a limit 10 ) from t1 group by grp; > > -#select grp,group_concat(c order by c) from t1 group by grp; > -#select grp,group_concat(c order by c limit 2) from t1 group by grp; > +select grp,group_concat(c order by c) from t1 group by grp; > +select grp,group_concat(c order by c limit 2) from t1 group by grp; > > -#select grp,group_concat(c order by c desc) from t1 group by grp; > -#select grp,group_concat(c order by c desc limit 2) from t1 group by grp; > +select grp,group_concat(c order by c desc) from t1 group by grp; > +select grp,group_concat(c order by c desc limit 2) from t1 group by grp; > > -#select grp,group_concat(d order by a) from t1 group by grp; > -#select grp,group_concat(d order by a limit 2) from t1 group by grp; > +select grp,group_concat(d order by a) from t1 group by grp; > +select grp,group_concat(d order by a limit 2) from t1 group by grp; > > -#select grp,group_concat(d order by a desc) from t1 group by grp; > -#select grp,group_concat(d order by a desc limit 1) from t1 group by grp; > +select grp,group_concat(d order by a desc) from t1 group by grp; > +select grp,group_concat(d order by a desc limit 1) from t1 group by grp; > > What about this: create table t2 (a int, b varchar(10)); insert into t2 values (1,'a'),(1,'b'),(1,'c'), (2,'x'),(2,'y'); MariaDB [j10]> select group_concat(a,b) from t2; +-------------------+ | group_concat(a,b) | +-------------------+ | 1a,1b,1c,2x,2y | +-------------------+ 1 row in set (0.00 sec) MariaDB [j10]> select group_concat(a,b limit 3) from t2; +---------------------------+ | group_concat(a,b limit 3) | +---------------------------+ | 1a,1 | +---------------------------+ 1 row in set (0.01 sec) Do you think it's correct? We need to discuss this. > drop table t1; > diff --git a/sql/item_sum.cc b/sql/item_sum.cc > index 51a6c2b..681b800 100644 > --- a/sql/item_sum.cc > +++ b/sql/item_sum.cc > @@ -3114,6 +3114,11 @@ int dump_leaf_key(void* key_arg, element_count count __attribute__((unused)), > String *result= &item->result; > Item **arg= item->args, **arg_end= item->args + item->arg_count_field; > uint old_length= result->length(); > + ulonglong *offset_limit= &item->offset_limit; > + ulonglong *row_limit = &item->row_limit; > + > + if(item->limit_clause && !(*row_limit)) > + return 1; > > if (item->no_appended) > item->no_appended= FALSE; > @@ -3148,7 +3153,31 @@ int dump_leaf_key(void* key_arg, element_count count __attribute__((unused)), > res= (*arg)->val_str(&tmp); > } > if (res) > - result->append(*res); > + { > + // This can be further optimised if we calculate the values for the fields only when it is necessary that is afer #offset_limit > + if(item->limit_clause) > + { > + if(*offset_limit) > + { > + (*offset_limit)--; > + item->no_appended= TRUE; > + } > + else > + { > + if(*row_limit) > + { > + result->append(*res); > + (*row_limit)--; > + if(!(*row_limit)) > + item->no_appended= TRUE; I've commented out the above two lines and the testcase still produces the same result. I claim that they are not needed. If you think otherwise, please add test coverage. Please do the same for the assignment right below. > + } > + else > + item->no_appended= TRUE; > + } > + } > + else > + result->append(*res); > + } > } > > item->row_count++; > @@ -3199,7 +3228,8 @@ int dump_leaf_key(void* key_arg, element_count count __attribute__((unused)), > Item_func_group_concat(THD *thd, Name_resolution_context *context_arg, > bool distinct_arg, List<Item> *select_list, > const SQL_I_List<ORDER> &order_list, > - String *separator_arg) > + String *separator_arg, bool limit_clause, > + ulonglong row_limit, ulonglong offset_limit) > :Item_sum(thd), tmp_table_param(0), separator(separator_arg), tree(0), > unique_filter(NULL), table(0), > order(0), context(context_arg), > @@ -3208,7 +3238,9 @@ int dump_leaf_key(void* key_arg, element_count count __attribute__((unused)), > row_count(0), > distinct(distinct_arg), > warning_for_row(FALSE), > - force_copy_fields(0), original(0) > + force_copy_fields(0), original(0), > + row_limit(row_limit), offset_limit(offset_limit),limit_clause(limit_clause), > + copy_offset_limit(offset_limit), copy_row_limit(row_limit) > { > Item *item_select; > Item **arg_ptr; > @@ -3269,7 +3301,9 @@ int dump_leaf_key(void* key_arg, element_count count __attribute__((unused)), > warning_for_row(item->warning_for_row), > always_null(item->always_null), > force_copy_fields(item->force_copy_fields), > - original(item) > + original(item), row_limit(item->row_limit), > + offset_limit(item->offset_limit),limit_clause(item->limit_clause), > + copy_offset_limit(item->copy_offset_limit), copy_row_limit(item->row_limit) > { > quick_group= item->quick_group; > result.set_charset(collation.collation); > @@ -3382,6 +3416,8 @@ void Item_func_group_concat::clear() > null_value= TRUE; > warning_for_row= FALSE; > no_appended= TRUE; > + offset_limit= copy_offset_limit; > + row_limit= copy_row_limit; > if (tree) > reset_tree(tree); > if (unique_filter) > diff --git a/sql/item_sum.h b/sql/item_sum.h > index b9075db..a38c5ca 100644 > --- a/sql/item_sum.h > +++ b/sql/item_sum.h > @@ -1583,6 +1583,12 @@ class Item_func_group_concat : public Item_sum > bool always_null; > bool force_copy_fields; > bool no_appended; > + bool limit_clause; > + ulonglong row_limit; > + ulonglong offset_limit; > + ulonglong copy_offset_limit; > + ulonglong copy_row_limit; Please add (short) comments describing each new member variable. > + > /* > Following is 0 normal object and pointer to original one for copy > (to correctly free resources) > @@ -1602,7 +1608,8 @@ class Item_func_group_concat : public Item_sum > public: > Item_func_group_concat(THD *thd, Name_resolution_context *context_arg, > bool is_distinct, List<Item> *is_select, > - const SQL_I_List<ORDER> &is_order, String *is_separator); > + const SQL_I_List<ORDER> &is_order, String *is_separator, > + bool limit_clause, ulonglong row_limit, ulonglong offset_limit); > > Item_func_group_concat(THD *thd, Item_func_group_concat *item); > ~Item_func_group_concat(); > diff --git a/sql/sql_yacc.yy b/sql/sql_yacc.yy > index 163322d..d87e6fb 100644 > --- a/sql/sql_yacc.yy > +++ b/sql/sql_yacc.yy > @@ -1786,7 +1786,7 @@ bool my_yyoverflow(short **a, YYSTYPE **b, ulong *yystacksize); > %type <num> > order_dir lock_option > udf_type opt_local opt_no_write_to_binlog > - opt_temporary all_or_any opt_distinct > + opt_temporary all_or_any opt_distinct opt_glimit_clause > opt_ignore_leaves fulltext_options union_option > opt_not > select_derived_init transaction_access_mode_types > @@ -1844,7 +1844,7 @@ bool my_yyoverflow(short **a, YYSTYPE **b, ulong *yystacksize); > opt_escape > sp_opt_default > simple_ident_nospvar simple_ident_q > - field_or_var limit_option > + field_or_var limit_option glimit_option > part_func_expr > window_func_expr > window_func > @@ -10449,16 +10449,36 @@ sum_expr: > | GROUP_CONCAT_SYM '(' opt_distinct > { Select->in_sum_expr++; } > expr_list opt_gorder_clause > - opt_gconcat_separator > + opt_gconcat_separator opt_glimit_clause > ')' > { > SELECT_LEX *sel= Select; > sel->in_sum_expr--; > - $$= new (thd->mem_root) > - Item_func_group_concat(thd, Lex->current_context(), $3, $5, > - sel->gorder_list, $7); > - if ($$ == NULL) > - MYSQL_YYABORT; > + if(!$8) Coding style requires a space after "if": "if (...)". Please fix this everywhere in this patch. IIRC it also requires that if the statement inside if, it should be enclosed in {...}. > + $$= new (thd->mem_root) > + Item_func_group_concat(thd, Lex->current_context(), $3, $5, > + sel->gorder_list, $7, FALSE, 0, 0); > + else > + { > + if(sel->select_limit && sel->offset_limit) > + $$= new (thd->mem_root) > + Item_func_group_concat(thd, Lex->current_context(), $3, $5, > + sel->gorder_list, $7, TRUE, sel->select_limit->val_int(), > + sel->offset_limit->val_int()); There is at least one case where you can't call val_int from the parser: when LIMIT number is a prepared statement parameter. consider an example: create table t2 (a int, b varchar(10)); insert into t2 values (1,'a'),(1,'b'),(1,'c'), (2,'x'),(2,'y'); prepare STMT from 'select group_concat(b order by a limit ?) from t2' ; This crashes: Program received signal SIGABRT, Aborted. 0x00007ffff5d7a425 in __GI_raise (sig=<optimized out>) at ../nptl/sysdeps/unix/sysv/linux/raise.c:64 64 ../nptl/sysdeps/unix/sysv/linux/raise.c: No such file or directory. #2 0x00007ffff5d730ee in __assert_fail_base (fmt=<optimized out>, assertion=0x5555566d8bd7 "0", file=0x5555566d8f08 "/home/psergey/dev-git/10.2-varun-mdev11297/sql/item.cc", line=<optimized out>, function=<optimized out>) at assert.c:94 #3 0x00007ffff5d73192 in __GI___assert_fail (assertion=0x5555566d8bd7 "0", file=0x5555566d8f08 "/home/psergey/dev-git/10.2-varun-mdev11297/sql/item.cc", line=3689, function=0x5555566dbd60 "virtual longlong Item_param::val_int()") at assert.c:103 #4 0x0000555555de54a0 in Item_param::val_int (this=0x7fff40054160) at /home/psergey/dev-git/10.2-varun-mdev11297/sql/item.cc:3689 #5 0x0000555555d63277 in MYSQLparse (thd=0x7fff40000b00) at /home/psergey/dev-git/10.2-varun-mdev11297/sql/sql_yacc.yy:10471 #6 0x0000555555b6e696 in parse_sql (thd=0x7fff40000b00, parser_state=0x7fffdc0c8cb0, creation_ctx=0x0, do_pfs_digest=false) at /home/psergey/dev-git/10.2-varun-mdev11297/sql/sql_parse.cc:9815 #7 0x0000555555b848b2 in Prepared_statement::prepare (this=0x7fff4004bd80, packet=0x7fff400133a0 "select group_concat(b order by a limit ?) from t2", packet_len=49) at /home/psergey/dev-git/10.2-varun-mdev11297/sql/sql_prepare.cc:3835 ... > + else if(sel->select_limit) > + $$= new (thd->mem_root) > + Item_func_group_concat(thd, Lex->current_context(), $3, $5, > + sel->gorder_list, $7, TRUE, sel->select_limit->val_int(),0); > + else > + $$ = NULL; //need more thinking and maybe some error handling to inform the user the problem with the query First, the identation is wrong, this 'else' is actually attached to the "if(sel->select_limit)". One doesn't make such mistakes when they are using {...} for all multi-line if-elses. Second, can the "$$ = NULL" ever execute at all? It will be run when the LIMIT clause is present (as $8!=0), but neither LIMIT nor OFFSET clauses are. I think the grammar prevents that. You can add an assert if you still want the code here to cover all cases. > + } > + if ($$ == NULL) > + MYSQL_YYABORT; > + > + sel->select_limit= NULL; > + sel->offset_limit= NULL; > + sel->explicit_limit= 0; > + > $5->empty(); > sel->gorder_list.empty(); > } > @@ -10680,6 +10700,105 @@ gorder_list: > { if (add_gorder_to_list(thd, $1,(bool) $2)) MYSQL_YYABORT; } > ; > > +opt_glimit_clause: > + /* empty */ { $$ = 0; } > + | glimit_clause { $$ = 1; } > + ; > + > +glimit_clause_init: > + LIMIT{} > + ; > + > +glimit_clause: > + glimit_clause_init glimit_options{} > + | glimit_clause_init glimit_options > + ROWS_SYM EXAMINED_SYM glimit_rows_option{} > + | glimit_clause_init ROWS_SYM EXAMINED_SYM glimit_rows_option{} > + ; > + > +glimit_options: > + glimit_option > + { > + SELECT_LEX *sel= Select; > + sel->select_limit= $1; > + sel->offset_limit= 0; > + sel->explicit_limit= 1; > + } > + | glimit_option ',' glimit_option > + { > + SELECT_LEX *sel= Select; > + sel->select_limit= $3; > + sel->offset_limit= $1; > + sel->explicit_limit= 1; > + } > + | glimit_option OFFSET_SYM glimit_option > + { > + SELECT_LEX *sel= Select; > + sel->select_limit= $1; > + sel->offset_limit= $3; > + sel->explicit_limit= 1; > + } > + ; > + > +glimit_option: > + ident > + { > + Item_splocal *splocal; > + LEX *lex= thd->lex; > + Lex_input_stream *lip= & thd->m_parser_state->m_lip; > + sp_variable *spv; > + sp_pcontext *spc = lex->spcont; > + if (spc && (spv = spc->find_variable($1, false))) > + { > + splocal= new (thd->mem_root) > + Item_splocal(thd, $1, spv->offset, spv->sql_type(), > + lip->get_tok_start() - lex->sphead->m_tmp_query, > + lip->get_ptr() - lip->get_tok_start()); > + if (splocal == NULL) > + MYSQL_YYABORT; > +#ifndef DBUG_OFF > + splocal->m_sp= lex->sphead; > +#endif > + lex->safe_to_cache_query=0; > + } > + else > + my_yyabort_error((ER_SP_UNDECLARED_VAR, MYF(0), $1.str)); > + if (splocal->type() != Item::INT_ITEM) > + my_yyabort_error((ER_WRONG_SPVAR_TYPE_IN_LIMIT, MYF(0))); > + splocal->limit_clause_param= TRUE; > + $$= splocal; > + } > + | param_marker > + { > + $1->limit_clause_param= TRUE; > + } > + | ULONGLONG_NUM > + { > + $$= new (thd->mem_root) Item_uint(thd, $1.str, $1.length); > + if ($$ == NULL) > + MYSQL_YYABORT; > + } > + | LONG_NUM > + { > + $$= new (thd->mem_root) Item_uint(thd, $1.str, $1.length); > + if ($$ == NULL) > + MYSQL_YYABORT; > + } > + | NUM > + { > + $$= new (thd->mem_root) Item_uint(thd, $1.str, $1.length); > + if ($$ == NULL) > + MYSQL_YYABORT; > + } > + ; > + > +glimit_rows_option: > + glimit_option > + { > + LEX *lex=Lex; > + lex->limit_rows_examined= $1; > + } > + > in_sum_expr: > opt_all > { When compiling, I'm getting warnings like this: || [ 76%] Building CXX object sql/CMakeFiles/sql.dir/item_sum.cc.o || /home/psergey/dev-git/10.2-varun-mdev11297/sql/item_sum.h: In constructor ‘Item_func_group_concat::Item_func_group_concat(THD*, Name_resolution_context*, bool, List<Item>*, const SQL_I_List<st_order>&, String*, bool, ulonglong, ulonglong)’: /home/psergey/dev-git/10.2-varun-mdev11297/sql/item_sum.h|1596 col 27| warning: ‘Item_func_group_concat::original’ will be initialized after [-Wreorder] /home/psergey/dev-git/10.2-varun-mdev11297/sql/item_sum.h|1587 col 13| warning: ‘ulonglong Item_func_group_concat::row_limit’ [-Wreorder] item_sum.cc|3227 col 1| warning: when initialized here [-Wreorder] /home/psergey/dev-git/10.2-varun-mdev11297/sql/item_sum.h|1588 col 13| warning: ‘Item_func_group_concat::offset_limit’ will be initialized after [-Wreorder] /home/psergey/dev-git/10.2-varun-mdev11297/sql/item_sum.h|1586 col 8| warning: ‘bool Item_func_group_concat::limit_clause’ [-Wreorder] item_sum.cc|3227 col 1| warning: when initialized here [-Wreorder] || /home/psergey/dev-git/10.2-varun-mdev11297/sql/item_sum.h: In constructor ‘Item_func_group_concat::Item_func_group_concat(THD*, Item_func_group_concat*)’: /home/psergey/dev-git/10.2-varun-mdev11297/sql/item_sum.h|1596 col 27| warning: ‘Item_func_group_concat::original’ will be initialized after [-Wreorder] /home/psergey/dev-git/10.2-varun-mdev11297/sql/item_sum.h|1587 col 13| warning: ‘ulonglong Item_func_group_concat::row_limit’ [-Wreorder] item_sum.cc|3288 col 1| warning: when initialized here [-Wreorder] /home/psergey/dev-git/10.2-varun-mdev11297/sql/item_sum.h|1588 col 13| warning: ‘Item_func_group_concat::offset_limit’ will be initialized after [-Wreorder] /home/psergey/dev-git/10.2-varun-mdev11297/sql/item_sum.h|1586 col 8| warning: ‘bool Item_func_group_concat::limit_clause’ [-Wreorder] item_sum.cc|3288 col 1| warning: when initialized here [-Wreorder] || Linking CXX static library libsql.a BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://s.petrunia.net/blog

1 0

Re: [Maria-developers] Review for MDEV-9069
by Sergei Golubchik 30 Nov '16

30 Nov '16

Hi, Georg! On Nov 03, Georg Richter wrote: > Hello Serg, > > attached you will find the patch for MDEV-9069. > > /Georg > > -- > Georg Richter, Senior Software Engineer > MariaDB Corporation Ab > From bde4b9d0683f115fa55bb6d2e2164739f89d732c Mon Sep 17 00:00:00 2001 > From: Georg Richter <georg(a)mariadb.com> > Date: Thu, 3 Nov 2016 07:44:15 +0100 > Subject: [PATCH] Initial implementaion for MDEV-9069: "extend AES_ENCRYPT() > and AES_DECRYPT() to support IV and the algorithm" > > - Extended function syntax: > AES_ENCRYPT(plaintext, key, [[iv, [aad]] using block_encryption_mode); > AES_DECRYPT(plaintext, key, [[iv, [aad]] using block_encryption_mode); This doesn't look right. at least square brackets aren't paired, but also I'd expect [using block_encryption_mode] to be in brackets, and the comma after key should be too. > AES_ crypt functions will return an error, if > - length of IV is too small > - the number of parameter doesn't match (e.g. if an iv was specified for block cipher mode ECB) > AES_crypt functions will return a warning, if > - key_size is too small > > - Added new session variable block_encryption_mode > - Added a new status variable block_encryption_mode_list, which lists available block_encryption modes This is absolutely not necessary, the list of available values for block_encryption_mode can be retrieved as select enum_value_list from system_variables where variable_name='block_encryption_mode'; > > - Added new tests for AES_ crypt functions: AES_ECB and AES_CBC vectors from > NIST Cryptographic validation program (CAVP) > [http://csrc.nist.gov/groups/STM/cavp/] What license are these tests available under? > > - To prevent too many warnings all keys in tests now are hased by sha2() function What does that mean? That is, no need to explain, I'll see it from the code, but the commit comment is confusing, please reformulate this sentence. > > - This patch also includes bug fix for MDEV-11174 (my_aes_crypt crashes in GCM mode) > > Note: > - AES_GCM and AES_CTR are not well tested - CAVP test vectors couldn't be used for AES_GCM since the auth tag after encryption differs. why does it differ? > - A key which is longer than the requested key size will be truncated to the appropriate size. what do you mean, truncated, there is a key derivation function that creates the real key from the user specified long key in Item_aes_crypt::create_key > diff --git a/mysys_ssl/my_crypt.cc b/mysys_ssl/my_crypt.cc > index 49bd9af..5485576 100644 > --- a/mysys_ssl/my_crypt.cc > +++ b/mysys_ssl/my_crypt.cc > @@ -155,6 +155,7 @@ class MyCTX_gcm : public MyCTX > int real_ivlen= EVP_CIPHER_CTX_iv_length(&ctx); > aad= iv + real_ivlen; > aadlen= ivlen - real_ivlen; > + EVP_CIPHER_CTX_ctrl(&ctx, EVP_CTRL_GCM_SET_IVLEN, 12, NULL); Hmm, why is that needed? > return res; > } > > @@ -205,12 +208,56 @@ const EVP_CIPHER *(*ciphers[])(uint)= { > aes_ecb, aes_cbc > #ifdef HAVE_EncryptAes128Ctr > , aes_ctr > +#endif > #ifdef HAVE_EncryptAes128Gcm > , aes_gcm > #endif > +}; > + > +const char *my_aes_block_encryption_mode_str[]= { > + "aes-128-ecb", > + "aes-192-ecb", > + "aes-256-ecb", > + "aes-128-cbc", > + "aes-192-cbc", > + "aes-256-cbc" > +#ifdef HAVE_EncryptAes128Ctr > + , "aes-128-ctr", > + "aes-192-ctr", > + "aes-256-ctr" > #endif > +#ifdef HAVE_EncryptAes128Gcm > + , "aes-128-gcm", > + "aes-192-gcm", > + "aes-256-gcm" > +#endif > + , NULL > }; > > +struct st_my_aes_info my_aes_info[]= { > + { my_aes_128_ecb, MY_AES_ECB, 128, 0, 2}, > + { my_aes_192_ecb, MY_AES_ECB, 192, 0, 2}, > + { my_aes_256_ecb, MY_AES_ECB, 256, 0, 2}, > + { my_aes_128_cbc, MY_AES_CBC, 128, MY_AES_BLOCK_SIZE, 3}, > + { my_aes_192_cbc, MY_AES_CBC, 192, MY_AES_BLOCK_SIZE, 3}, > + { my_aes_256_cbc, MY_AES_CBC, 256, MY_AES_BLOCK_SIZE, 3}, > +#ifdef HAVE_EncryptAes128Ctr > + { my_aes_128_ctr, MY_AES_CTR, 128, MY_AES_BLOCK_SIZE, 3}, > + { my_aes_192_ctr, MY_AES_CTR, 192, MY_AES_BLOCK_SIZE, 3}, > + { my_aes_256_ctr, MY_AES_CTR, 256, MY_AES_BLOCK_SIZE, 3}, > +#endif > +#ifdef HAVE_EncryptAes128Gcm > + { my_aes_128_gcm, MY_AES_GCM, 128, 12, 4 }, > + { my_aes_192_gcm, MY_AES_GCM, 192, 12, 4 }, > + { my_aes_256_gcm, MY_AES_GCM, 256, 12, 4 }, > +#endif > +}; > + > +TYPELIB block_encryption_mode_typelib= > + { my_aes_block_encryption_end, > + "block_encryption_modes", > + my_aes_block_encryption_mode_str, NULL}; > + I don't think all that belongs to my_crypt.cc and my_crypt.h. This is part of the SQL interface to aes, it should be in item_strfunc.cc > extern "C" { > > int my_aes_crypt_init(void *ctx, enum my_aes_mode mode, int flags, > diff --git a/sql/item_strfunc.cc b/sql/item_strfunc.cc > index acd3d74..9a4108c 100644 > --- a/sql/item_strfunc.cc > +++ b/sql/item_strfunc.cc > @@ -359,47 +359,116 @@ void Item_func_sha2::fix_length_and_dec() > } > > /* Implementation of AES encryption routines */ > -void Item_aes_crypt::create_key(String *user_key, uchar *real_key) > +void Item_aes_crypt::create_item(String *user_item, uchar *real_item, uint item_size) > { > - uchar *real_key_end= real_key + AES_KEY_LENGTH / 8; > + uchar *real_item_end= real_item + item_size; > uchar *ptr; > - const char *sptr= user_key->ptr(); > - const char *key_end= sptr + user_key->length(); > + const char *sptr= user_item->ptr(); > + const char *item_end= sptr + user_item->length(); > > - bzero(real_key, AES_KEY_LENGTH / 8); > + bzero(real_item, item_size); > > - for (ptr= real_key; sptr < key_end; ptr++, sptr++) > + for (ptr= real_item; sptr < item_end; ptr++, sptr++) > { > - if (ptr == real_key_end) > - ptr= real_key; > + if (ptr == real_item_end) > + return; > *ptr ^= (uchar) *sptr; > } > } > > - > String *Item_aes_crypt::val_str(String *str) > { > DBUG_ASSERT(fixed == 1); > - StringBuffer<80> user_key_buf; > + StringBuffer<80> user_buf; > String *sptr= args[0]->val_str(str); > - String *user_key= args[1]->val_str(&user_key_buf); > - uint32 aes_length; > + enum my_aes_mode block_cipher; > + > + if (block_encryption_mode == (ulong)-1) > + block_encryption_mode= current_thd->variables.block_encryption_mode; > + > + block_cipher= my_aes_info[block_encryption_mode].aes_mode; > + > + if (arg_count > my_aes_info[block_encryption_mode].param_count) > + { > + null_value= 1; > + my_error(ER_WRONG_PARAMCOUNT_TO_NATIVE_FCT, MYF(0), what ? "AES_ENCRYPT" : "AES_DECRYPT"); > + return 0; > + } > > - if (sptr && user_key) // we need both arguments to be not NULL > + if (sptr) > { > + uchar riv[128]; // MY_AES_BLOCK_SIZE + MAX_AADLEN (=720 bits) > + uint32 digest_size; // size of digest in bytes > + String *user_iv; > + uint iv_size; > + > + > + digest_size= my_aes_get_size(block_cipher, sptr->length()); > + iv_size= my_aes_info[block_encryption_mode].iv_size; > + > + if (iv_size) // check if block encryption mode requires IV > + { I thought you can have an assert here: DBUG_ASSERT(arg_count > 2) because you do ER_WRONG_PARAMCOUNT_TO_NATIVE_FCT earlier. > + if (arg_count > 2) > + { > + user_iv= args[2]->val_str(&user_buf); > + // throw error if iv is too short > + if (!user_iv || user_iv->length() < iv_size) > + { > + null_value= 1; > + my_error(ER_AES_INVALID_IV, MYF(0), iv_size); This needs to be documented carefully, because it's inconsistent with the behavior for key argument. > + return 0; > + } > + create_item(user_iv, riv, iv_size); > + } else { > + null_value= 1; > + my_error(ER_AES_INVALID_IV, MYF(0), iv_size); > + return 0; > + } > + } > +#ifdef HAVE_EncryptAes128Gcm > + if (arg_count > 3 && > + block_cipher == MY_AES_GCM) why do you check for MY_AES_GCM, isn't arg_count > 3 enough? > + { > + String *user_aad= args[3]->val_str(&user_buf); > + if (user_aad) > + { > + create_item(user_aad, riv + iv_size, user_aad->length()); > + iv_size+= user_aad->length(); > + } > + } > +#endif > null_value=0; > - aes_length=my_aes_get_size(MY_AES_ECB, sptr->length()); > > - if (!str_value.alloc(aes_length)) // Ensure that memory is free > + if (!str_value.alloc(digest_size)) // Ensure that memory is free > { > - uchar rkey[AES_KEY_LENGTH / 8]; > - create_key(user_key, rkey); > + String *user_key= args[1]->val_str(&user_buf); > + uint32 key_size= my_aes_info[block_encryption_mode].key_size; > + uchar rkey[MY_AES_MAX_KEY_LENGTH]; > > - if (!my_aes_crypt(MY_AES_ECB, what, (uchar*)sptr->ptr(), sptr->length(), > - (uchar*)str_value.ptr(), &aes_length, > - rkey, AES_KEY_LENGTH / 8, 0, 0)) > + if (!user_key || !user_key->length()) > { > - str_value.length((uint) aes_length); > + null_value= 1; > + return 0; > + } better to check the key before allocating any memory, just as the old code did. > + > + // if key is too short, throw a warning > + if (user_key->length() < (key_size / 8)) > + { > + push_warning_printf(current_thd, Sql_condition::WARN_LEVEL_WARN, > + ER_WARN_AES_KEY_TOO_SHORT, > + ER_THD(current_thd, ER_WARN_AES_KEY_TOO_SHORT), > + key_size / 8); > + } > + > + create_item(user_key, rkey, key_size / 8); > + > + if (!my_aes_crypt(block_cipher, > + what, (uchar*)sptr->ptr(), sptr->length(), > + (uchar*)str_value.ptr(), &digest_size, > + rkey, key_size / 8, (arg_count > 2) ? riv : 0, > + (arg_count > 2) ? iv_size : 0)) > + { > + str_value.length((uint) digest_size); > return &str_value; > } > } > diff --git a/sql/item_strfunc.h b/sql/item_strfunc.h > index 25b63eb..153ca7b 100644 > --- a/sql/item_strfunc.h > +++ b/sql/item_strfunc.h > @@ -184,8 +188,9 @@ class Item_func_aes_encrypt :public Item_aes_crypt > class Item_func_aes_decrypt :public Item_aes_crypt > { > public: > - Item_func_aes_decrypt(THD *thd, Item *a, Item *b): > - Item_aes_crypt(thd, a, b) {} > + Item_func_aes_decrypt(THD *thd, ulong mode, List<Item> &list_item): > + Item_aes_crypt(thd, list_item) > + { block_encryption_mode= mode; } you also need to implement check_vcol_func_processor() and check_valid_arguments_processor() to make sure that AES_ENCRYPT/AES_DECRYPT without the USING clause are not used in partitioning expression and in stored generated columns. And add tests for that, please. after I push MDEV-5800 (or now in bb-10.2-vcols branch) see Item_func_week as an example (it also has an optional argument that by default takes a value from a session variable). > void fix_length_and_dec(); > const char *func_name() const { return "aes_decrypt"; } > Item *get_copy(THD *thd, MEM_ROOT *mem_root) > diff --git a/sql/mysqld.cc b/sql/mysqld.cc > index 310ccb0..b2f677e 100644 > --- a/sql/mysqld.cc > +++ b/sql/mysqld.cc > @@ -8317,6 +8317,27 @@ static int show_memory_used(THD *thd, SHOW_VAR *var, char *buff, > return 0; > } > > +static int show_block_encrypt_mode_list(THD *thd, SHOW_VAR *var, char *buff, > + struct system_status_var *status_var, > + enum enum_var_type scope) > +{ > + uint i; > + char *end= buff + SHOW_VAR_FUNC_BUFF_SIZE; > + var->type= SHOW_CHAR; > + var->value= buff; > + > + for (i=0; my_aes_block_encryption_mode_str[i] && > + buff + strlen(my_aes_block_encryption_mode_str[i]) < end && > + i < my_aes_block_encryption_end; i++) > + { > + buff= strnmov(buff, my_aes_block_encryption_mode_str[i], end-buff-1); > + *buff++= ','; > + } > + if (i) > + buff--; > + *buff= 0; > + return 0; > +} No need to, because block_encryption_mode_list status variable is not needed either. > > #ifndef DBUG_OFF > static int debug_status_func(THD *thd, SHOW_VAR *var, char *buff, > diff --git a/sql/share/errmsg-utf8.txt b/sql/share/errmsg-utf8.txt > index d42611b..5a4c8bf 100644 > --- a/sql/share/errmsg-utf8.txt > +++ b/sql/share/errmsg-utf8.txt > @@ -7231,4 +7231,16 @@ ER_PARTITION_DEFAULT_ERROR > eng "Only one DEFAULT partition allowed" > ukr "Припустимо мати тільки один DEFAULT розділ" > ER_REFERENCED_TRG_DOES_NOT_EXIST > - eng "Referenced trigger '%s' for the given action time and event type does not exist" > + eng "Referenced trigger '%s' for the given action time and event type does not exist" > +ER_AES_INVALID_IV > + eng "The initialization vector supplied is too short. Must be at least %d bytes long" > + ger "Der angegebene Initalisierungsvektor ist zu kurz. Die Mindestlänge beträgt %d Bytes" > +ER_AES_INVALID_MODE > + eng "Unknown block encryption mode '%s'" > + ger "Der angegebene Blockcodierungsmodus '%s' exstiert nicht" > +ER_AES_INVALID_KEY > + eng "Invalid key" > + ger "Ungültiger Schlüssel" This error message seems to be unused. > +ER_WARN_AES_KEY_TOO_SHORT > + eng "The provided key is too short. It is more secure to use a key with %d bytes" s/with %d bytes/of at least %d bytes long/ > + ger "Der angegebene Schlüssel ist zu kurz. Es ist empfehlenswert einen Schlüssel mit %d Bytes zu verwenden" > diff --git a/sql/sql_yacc.yy b/sql/sql_yacc.yy > index e17a514..12e79d3 100644 > --- a/sql/sql_yacc.yy > +++ b/sql/sql_yacc.yy > @@ -6670,7 +6673,6 @@ charset: > CHAR_SYM SET {} > | CHARSET {} > ; > - Huh? > charset_name: > ident_or_text > { > @@ -7102,6 +7104,16 @@ opt_component: > | '.' ident { $$= $2; } > ; > > +block_encryption_mode: > + ident_or_text > + { > + if (!($$=find_type($1.str, &block_encryption_mode_typelib, MYF(0)))) > + my_yyabort_error((ER_AES_INVALID_MODE, MYF(0), $1.str)); > + $$-= 1; > + } > + | DEFAULT { $$= (ulong)-1;} why do you want to support USING DEFAULT ? > + ; > + > string_list: > text_string > { Lex->last_field->interval_list.push_back($1, thd->mem_root); } > diff --git a/sql/sys_vars.cc b/sql/sys_vars.cc > index b6359ff..8b39462 100644 > --- a/sql/sys_vars.cc > +++ b/sql/sys_vars.cc > @@ -554,6 +554,10 @@ static Sys_var_mybool Sys_explicit_defaults_for_timestamp( > READ_ONLY GLOBAL_VAR(opt_explicit_defaults_for_timestamp), > CMD_LINE(OPT_ARG), DEFAULT(FALSE), NO_MUTEX_GUARD, NOT_IN_BINLOG); > > +static Sys_var_enum Sys_block_encryption_mode( > + "block_encryption_mode", "mode for AES_ENCRYPT/AES_DECRYPT", "for AES_ENCRYPT and AES_DECRYPT" > + SESSION_VAR(block_encryption_mode), CMD_LINE(REQUIRED_ARG), > + my_aes_block_encryption_mode_str, DEFAULT(my_aes_128_ecb)); > > static Sys_var_ulonglong Sys_bulk_insert_buff_size( > "bulk_insert_buffer_size", "Size of tree cache used in bulk " > diff --git a/mysql-test/r/aes_cbc_crypt.result b/mysql-test/r/aes_cbc_crypt.result > new file mode 100644 > index 0000000..87ed84b > --- /dev/null > +++ b/mysql-test/r/aes_cbc_crypt.result > @@ -0,0 +1,19300 @@ > +# > +# Error checking > +# > +SELECT HEX(AES_ENCRYPT("Die Katze tritt die Treppe krumm", "foo", "1234567890123456" USING "aes-128-cbc")); I think it's better to use underscores aes_128_cbc, then the mode can be used as an identifier: AES_ENCRYPT("Die Katze", "foo", "1234567890123456" USING aes_128_cbc); (like charset names) otherwise it's not at all intuitive that SELECT AES_ENCRYPT("Die Katze", "foo", "1234567890123456" USING "aes-128-cbc"); works, but SELECT AES_ENCRYPT("Die Katze", "foo", "1234567890123456" USING concat_ws("-", "aes", "128", "cbc")); doesn't. So, either exclude the possibility of this confusion by not allowing a string literal as a mode. Or (actually better) allow any arbitrary non-constant expression as the mode. > +# > +# Session variable tests > +# > +SET session block_encryption_mode="aes-111-cbc"; > +ERROR 42000: Variable 'block_encryption_mode' can't be set to the value of 'aes-111-cbc' > +SET session block_encryption_mode="aes-128-cbc"; > +SELECT LEFT(AES_ENCRYPT(X'53696E676C6520626C6F636B206D7367', X'06A9214036B8A15B512E03D534120006', X'3DAFBA429D9EB430B422DA802C9FAC41'), 16) = X'E353779C1079AEB82708942DBE77181A'; > +LEFT(AES_ENCRYPT(X'53696E676C6520626C6F636B206D7367', X'06A9214036B8A15B512E03D534120006', X'3DAFBA429D9EB430B422DA802C9FAC41'), 16) = X'E353779C1079AEB82708942DBE77181A' > +1 > +SELECT AES_DECRYPT(AES_ENCRYPT(X'53696E676C6520626C6F636B206D7367', X'06A9214036B8A15B512E03D534120006', X'3DAFBA429D9EB430B422DA802C9FAC41'), X'06A9214036B8A15B512E03D534120006', X'3DAFBA429D9EB430B422DA802C9FAC41') = X'53696E676C6520626C6F636B206D7367'; > +AES_DECRYPT(AES_ENCRYPT(X'53696E676C6520626C6F636B206D7367', X'06A9214036B8A15B512E03D534120006', X'3DAFBA429D9EB430B422DA802C9FAC41'), X'06A9214036B8A15B512E03D534120006', X'3DAFBA429D9EB430B422DA802C9FAC41') = X'53696E676C6520626C6F636B206D7367' please add tests for views, default clause, virtual columns, partitioning. > +1 > ... I don't think it's necessary to commit megabytes of CAVP tests into the server repository. May be just a few K of them will be enough? Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

1 0

[Maria-developers] Working on spider patches, MDEV-7698
by Michael Widenius 30 Nov '16

30 Nov '16

Hi! I have now started to work on the spider patches for MariaDB 10.2, MDEV-7698. I have moved all code from maria-10.1-spider to a new branch maria-10.2-spider and added some more patches. I have closed all related MDEV's in MDEV-7698 that is now included in 10.2-spider. While doing this, I noticed that spider/handler test was not included in the test suite. I added the missing suite.pm and suite.opt files and got the tests to work. However, when I tried to run test to verify my changes, I noticed that a lot of test in spider/handler where failing: mysql-test-run --suite=spider/handler produces these failures: spider/handler.spider3_fixes spider/handler.direct_aggregate spider/handler.direct_update spider/handler.spider_fixes spider/handler.function spider/handler.ha spider/handler.vp_fixes All failures are because .test and .result file doesn't match. I checked the patch file: http://spiderformysql.com/downloads/spider-3.2/patch_mariadb-10.1.8.tgz but this doesn't include any updates to the handler test files: grep mysql_test * returns nothing. However the .tar file: http://spiderformysql.com/downloads/spider-3.2/mariadb-10.1.8-spider-3.2-vp… Contains a lot of updated .test and .result files. Kentoku, do you have patches for the test files, or should I just take them from the above spider branch or from somewhere else ? Another question: After applying the patches: 013_mariadb-10.0.15.vp_handler.diff 034_mariadb-10.0.15.vp_handler2.diff 005_mariadb-10.0.15.hs.diff 041_mariadb-10.0.15.vp_handler2.diff I get the following change in spider/handler/basic_sql.result: --- a/storage/spider/mysql-test/spider/handler/r/basic_sql.result +++ b/storage/spider/mysql-test/spider/handler/r/basic_sql.result @@ -70,6 +70,12 @@ CREATE TABLE ta_l ( PRIMARY KEY(a) ) MASTER_1_ENGINE MASTER_1_CHARSET MASTER_1_COMMENT_2_1 IGNORE SELECT a, b, c FROM tb_l +Warnings: +Warning 1062 Duplicate entry '1' for key 'PRIMARY' +Warning 1062 Duplicate entry '2' for key 'PRIMARY' +Warning 1062 Duplicate entry '3' for key 'PRIMARY' +Warning 1062 Duplicate entry '4' for key 'PRIMARY' +Warning 1062 Duplicate entry '5' for key 'PRIMARY' I can't figure out,why we get the above warnings. This is from a patch we discussed at booking.com one year ago. Any explanation for the above warnings would be appreciated. You can branch 10.2-spider and check the current state. Regards, Monty

2 5

Re: [Maria-developers] 1579140: MDEV-11005: Incorrect error message when using ONLINE alter table with GIS
by Sergei Golubchik 30 Nov '16

30 Nov '16

Hi, Jan! On Oct 10, Jan Lindström wrote: > parent(s): a6f032af5778018051d41fc8ba7e9c983b4b7fbf > author: Jan Lindström > committer: Jan Lindström > timestamp: 2016-10-10 12:34:06 +0300 > message: > > MDEV-11005: Incorrect error message when using ONLINE alter table with GIS > > Fix incorrect error message on ONLINE alter table with GIS indexes and > add rtree page type to page type check. > > diff --git a/sql/share/errmsg-utf8.txt b/sql/share/errmsg-utf8.txt > index d42611b..2892342 100644 > --- a/sql/share/errmsg-utf8.txt > +++ b/sql/share/errmsg-utf8.txt > @@ -6861,6 +6861,9 @@ ER_ALTER_OPERATION_NOT_SUPPORTED_REASON_CHANGE_FTS > ER_ALTER_OPERATION_NOT_SUPPORTED_REASON_FTS > eng "Fulltext index creation requires a lock" > > +ER_ALTER_OPERATION_NOT_SUPPORTED_REASON_GIS > + eng "Do not support online operation on table with GIS index" > + No, this is absolutely not possible in 10.2. ER_SQL_SLAVE_SKIP_COUNTER_NOT_SETTABLE_IN_GTID_MODE, the next error message, was added in 10.0. If you add a new error message here, then the error number for ER_SQL_SLAVE_SKIP_COUNTER_NOT_SETTABLE_IN_GTID_MODE will change. But 10.0 is GA, and error numbers cannot be changed in GA versions. You can add your new ER_ALTER_OPERATION_NOT_SUPPORTED_REASON_GIS after ER_CANNOT_DISCARD_TEMPORARY_TABLE, or, preferrably, copy all MySQL errors from 3000 and up to ER_ALTER_OPERATION_NOT_SUPPORTED_REASON_GIS. This will ensure that ER_ALTER_OPERATION_NOT_SUPPORTED_REASON_GIS will have the same number in MariaDB and in MySQL. > ER_SQL_SLAVE_SKIP_COUNTER_NOT_SETTABLE_IN_GTID_MODE > eng "sql_slave_skip_counter can not be set when the server is running with GTID_MODE = ON. Instead, for each transaction that you want to skip, generate an empty transaction with the same GTID as the transaction" Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

2 1

Re: [Maria-developers] [Commits] dabf6ca: MDEV-10340: support COM_RESET_CONNECTION
by Sergei Golubchik 30 Nov '16

30 Nov '16

Hi, Oleksandr! On Oct 17, Oleksandr Byelkin wrote: > revision-id: dabf6cac60987e88266396a28e40b341899704e6 (mariadb-10.2.2-49-gdabf6ca) > parent(s): 8303aded294ce905bbc513e7ee42623d5f1fdb50 > committer: Oleksandr Byelkin > timestamp: 2016-10-17 16:59:36 +0200 > message: > > MDEV-10340: support COM_RESET_CONNECTION > > draft to check with client > > --- > include/mysql.h.pp | 2 + > include/mysql_com.h | 2 + > mysql-test/r/mysqld--help.result | 2 +- > .../sys_vars/r/sysvars_server_embedded.result | 4 +- > .../sys_vars/r/sysvars_server_notembedded.result | 4 +- > sql/sql_class.cc | 65 ++++++++++++++++++++++ > sql/sql_class.h | 1 + > sql/sql_parse.cc | 13 ++++- > 8 files changed, 86 insertions(+), 7 deletions(-) > > diff --git a/include/mysql_com.h b/include/mysql_com.h > index 461800f..e1b129a 100644 > --- a/include/mysql_com.h > +++ b/include/mysql_com.h > @@ -111,6 +111,8 @@ enum enum_server_command > COM_TABLE_DUMP, COM_CONNECT_OUT, COM_REGISTER_SLAVE, > COM_STMT_PREPARE, COM_STMT_EXECUTE, COM_STMT_SEND_LONG_DATA, COM_STMT_CLOSE, > COM_STMT_RESET, COM_SET_OPTION, COM_STMT_FETCH, COM_DAEMON, > + COM_UNIMPLEMENTED, > + COM_RESET_CONNECTION, What's COM_UNIMPLEMENTED? > /* don't forget to update const char *command_name[] in sql_parse.cc */ > COM_MDB_GAP_BEG, > COM_MDB_GAP_END=250, > diff --git a/sql/sql_class.cc b/sql/sql_class.cc > index 1af3b9a..0cb58d4 100644 > --- a/sql/sql_class.cc > +++ b/sql/sql_class.cc > @@ -1575,6 +1575,71 @@ void THD::change_user(void) > } > > > +/* > + Do what's needed when one invokes change user > + > + SYNOPSIS > + cleanup_connection() > + > + IMPLEMENTATION > + Reset all resources that are connection specific > +*/ > + > +void THD::cleanup_connection(void) > +{ Why do you need a dedicated method for that, instead of simply invoking THD::change_user() for COM_RESET_CONNECTION? > +} Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

1 0