- developers - lists.mariadb.org

[Maria-developers] Review for version#2 of MDEV-11297: Add support for LIMIT clause in GROUP_CONCAT
by Sergey Petrunia 11 Dec '16

11 Dec '16

Hi Varun, Nice to see that nearly all comments from the previous review were addressed. Please find my further input below. General questions: How is DISTINCT handled? Syntax like GROUP_CONCAT(DISTINCT col ORDER BY ... LIMIT ...) is allowed by the parser, however I don't see any handling for DISTINCT in your patch. Did you forget to update group_concat_limit.result file? Please check. > diff -urpN '--exclude=.*' 10.2-varun-mdev11297-orig/sql/item_sum.cc 10.2-varun-mdev11297-noc/sql/item_sum.cc > --- 10.2-varun-mdev11297-orig/sql/item_sum.cc 2016-12-11 12:17:04.023961449 +0300 > +++ 10.2-varun-mdev11297-noc/sql/item_sum.cc 2016-12-11 12:18:40.411957365 +0300 > @@ -3114,6 +3114,11 @@ int dump_leaf_key(void* key_arg, element > 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->copy_offset_limit; > + ulonglong *row_limit = &item->copy_row_limit; > + > + if (item->limit_clause && !(*row_limit)) > + return 1; > > if (item->no_appended) > item->no_appended= FALSE; > @@ -3122,20 +3127,23 @@ int dump_leaf_key(void* key_arg, element > > tmp.length(0); > > - for (; arg < arg_end; arg++) > + if (!item->limit_clause || !(*offset_limit)) > { > - String *res; > - /* > - We have to use get_tmp_table_field() instead of > - real_item()->get_tmp_table_field() because we want the field in > - the temporary table, not the original field > - We also can't use table->field array to access the fields > - because it contains both order and arg list fields. > - */ > - if ((*arg)->const_item()) > - res= (*arg)->val_str(&tmp); > - else > + > + for (; arg < arg_end; arg++) > { > + String *res; > + /* > + We have to use get_tmp_table_field() instead of > + real_item()->get_tmp_table_field() because we want the field in > + the temporary table, not the original field > + We also can't use table->field array to access the fields > + because it contains both order and arg list fields. > + */ > + if ((*arg)->const_item()) > + res= (*arg)->val_str(&tmp); > + else > + { > Field *field= (*arg)->get_tmp_table_field(); Please make the code to use the right indentation. > if (field) > { > @@ -3146,9 +3154,19 @@ int dump_leaf_key(void* key_arg, element > } > else > res= (*arg)->val_str(&tmp); > + } > + if (res) > + { > + result->append(*res); > + } > } > - if (res) > - result->append(*res); > + if ((*row_limit)) > + (*row_limit)--; > + } > + else > + { > + item->no_appended= TRUE; If I comment out the above line, the test still passes. Please add test coverage that shows it is needed, or remove it. > + (*offset_limit)--; > } > > item->row_count++; > @@ -3199,7 +3217,8 @@ Item_func_group_concat:: > 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, > + Item *rowLimit, Item *offsetLimit) camelCase or javaStyleVariableNames are against the coding style. Please use something like row_limit_arg if there is no better option. (In general, please try to make your new code fit in with the rest of the code. Did you see camelCase anywhere? If not, it's a sign not to use it). > :Item_sum(thd), tmp_table_param(0), separator(separator_arg), tree(0), > unique_filter(NULL), table(0), > order(0), context(context_arg), > @@ -3208,7 +3227,9 @@ Item_func_group_concat(THD *thd, Name_re > row_count(0), > distinct(distinct_arg), > warning_for_row(FALSE), > - force_copy_fields(0), original(0) > + force_copy_fields(0),row_limit(0), > + offset_limit(0),limit_clause(limit_clause), > + copy_offset_limit(0), copy_row_limit(0),original(0) Please use 'NULL' for pointers, not '0'. > { > Item *item_select; > Item **arg_ptr; > @@ -3250,6 +3271,16 @@ Item_func_group_concat(THD *thd, Name_re > /* orig_args is only used for print() */ > orig_args= (Item**) (order + arg_count_order); > memcpy(orig_args, args, sizeof(Item*) * arg_count); > + > + if (limit_clause) > + { > + if (!(row_limit=(Item*) thd->alloc(sizeof(Item)))) > + return; I'm not sure what the above is supposed to mean? Why allocate sizeof(Item) bytes? > + row_limit= rowLimit; ... especially when the next thing you do is the above assignment, so you lose the value that you've got from the thd->alloc() call? > + if (!(offset_limit= (Item*)thd->alloc(sizeof(Item)))) > + return; > + offset_limit= offsetLimit; The same question as above. > + } > } > > > @@ -3269,7 +3300,9 @@ Item_func_group_concat::Item_func_group_ > warning_for_row(item->warning_for_row), > always_null(item->always_null), > force_copy_fields(item->force_copy_fields), > - 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->copy_row_limit), original(item->original) > { > quick_group= item->quick_group; > result.set_charset(collation.collation); > @@ -3382,6 +3415,10 @@ void Item_func_group_concat::clear() > null_value= TRUE; > warning_for_row= FALSE; > no_appended= TRUE; > + if (offset_limit) > + copy_offset_limit= offset_limit->val_int(); > + if (row_limit) > + copy_row_limit= row_limit->val_int(); > if (tree) > reset_tree(tree); > if (unique_filter) > diff -urpN '--exclude=.*' 10.2-varun-mdev11297-orig/sql/item_sum.h 10.2-varun-mdev11297-noc/sql/item_sum.h > --- 10.2-varun-mdev11297-orig/sql/item_sum.h 2016-12-11 12:17:04.043961448 +0300 > +++ 10.2-varun-mdev11297-noc/sql/item_sum.h 2016-12-11 12:18:40.415957365 +0300 > @@ -1583,6 +1583,12 @@ class Item_func_group_concat : public It > bool always_null; > bool force_copy_fields; > bool no_appended; > + Item *row_limit; > + Item *offset_limit; > + bool limit_clause; > + ulonglong copy_offset_limit; > + ulonglong copy_row_limit; > + In previous review I have requested: >> Please add (short) comments describing each new member variable. And you still haven't done that. > /* > Following is 0 normal object and pointer to original one for copy > (to correctly free resources) > @@ -1602,7 +1608,8 @@ protected: > 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, Item *row_limit, Item *offset_limit); > > Item_func_group_concat(THD *thd, Item_func_group_concat *item); > ~Item_func_group_concat(); > diff -urpN '--exclude=.*' 10.2-varun-mdev11297-orig/sql/sql_yacc.yy 10.2-varun-mdev11297-noc/sql/sql_yacc.yy > --- 10.2-varun-mdev11297-orig/sql/sql_yacc.yy 2016-12-11 12:17:04.107961446 +0300 > +++ 10.2-varun-mdev11297-noc/sql/sql_yacc.yy 2016-12-11 12:18:40.887957345 +0300 > @@ -1786,7 +1786,7 @@ bool my_yyoverflow(short **a, YYSTYPE ** > %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 ** > 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,34 @@ 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) > + $$= 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, > + sel->offset_limit); > + 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, 0); > + } > + if ($$ == NULL) > + MYSQL_YYABORT; > + > + sel->select_limit= NULL; > + sel->offset_limit= NULL; > + sel->explicit_limit= 0; > + > $5->empty(); > sel->gorder_list.empty(); > } > @@ -10680,6 +10698,95 @@ gorder_list: > { if (add_gorder_to_list(thd, $1,(bool) $2)) MYSQL_YYABORT; } > ; What is the difference between glimit_option and limit_option? Looking at the grammar, they look the same. Please either remove glimit_option or explain why do you need it. I can see that limit_clause and glimit_clause have to be different because limit_clause also supports LIMIT ROWS EXAMINED. Actually, there is one thing in limit_clause that I think should be in glimit_clause also. It's this call: Lex->set_stmt_unsafe(LEX::BINLOG_STMT_UNSAFE_LIMIT); Did you omit it intentionally? Let's meet and discuss it. > +opt_glimit_clause: > + /* empty */ { $$ = 0; } > + | glimit_clause { $$ = 1; } > + ; > + > +glimit_clause_init: > + LIMIT{} > + ; > + > +glimit_clause: > + glimit_clause_init glimit_options{} > + ; > + > +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; > + } > + ; > + > in_sum_expr: > opt_all > { BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://s.petrunia.net/blog

1 0

[Maria-developers] Please review MDEV-11528 Split Item_func_min_max::val_xxx() and Item_func_min_max::get_date() into methods in Type_handler
by Alexander Barkov 10 Dec '16

10 Dec '16

Hello Alexey, Can you please review a patch for MDEV-11528. Thanks!

1 0

[Maria-developers] MDEV-11503 Introduce Type_handler::make_in_vector() and Item_func_in_fix_comparator_compatible_types()
by Alexander Barkov 09 Dec '16

09 Dec '16

Hello Sanja, Please review a patch for MDEV-11503. Thanks!

1 1

[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] [Commits] 00f5065: MDEV-11422 rpl.rpl_parallel_optimistic_nobinlog failed
by Jan Lindström 08 Dec '16

08 Dec '16

Hi, All other looks correct, I'm concerned only following change that is not needed to fix this bug: On Thu, Dec 8, 2016 at 2:35 PM, <marko.makela(a)mariadb.com> wrote: > > > - err = lock_trx_handle_wait(trx, > - (lock_mutex_taken || already_have_lock_mutex), > - (trx_mutex_taken || already_have_trx_mutex)); > +#ifdef UNIV_DEBUG > + dberr_t err = > +#endif > + lock_trx_handle_wait(trx, true, true); > + > + ut_ad(err == DB_SUCCESS || err == DB_LOCK_WAIT > + || err == DB_DEADLOCK); > > > In above is true, true always correct? we conditionally take lock and trx mutexes on some cases if we already do not have them, and below function will take them if they are not already taken. There is complex rules as Galera can call this from different situations and we already have had cases where some mutex was taken twice or no mutex was taken. R: Jan

1 0

[Maria-developers] MDEV-10787 Assertion `ltime->neg == 0' failed in void date_to_datetime(MYSQL_TIME*)
by Alexander Barkov 07 Dec '16

07 Dec '16

Hello Sergei, please review a patch for MDEV-10787. Thanks!

2 2

[Maria-developers] Temporary push of multi_range_read support to 10.2
by Michael Widenius 07 Dec '16

07 Dec '16

Hi! Kentoku, I have now pushed into 10.2 the code for spider patches: 014,015,023,033,035,037,040,042,044,045,049,050,051,053,059 The push is done to make it easier for Jacob and you to make test cases for the new code. I will work on reviewing and cleaning up this patch during the next few days and then do a push a final version when I have got some more test cases that covers most of the new code. Regards, Monty

1 0

[Maria-developers] OSS-Fuzz
by Daniel Black 07 Dec '16

07 Dec '16

In case anyone is interested. Code needs to be written to take a random bit of data and push this into the server somehow. Apart from that it appears reasonably automated. https://github.com/google/oss-fuzz

1 0

Re: [Maria-developers] [Commits] 895309f: MDEV-10663: Use of Inline table columns in HAVING clause throws 1463 Error
by Sergei Golubchik 06 Dec '16

06 Dec '16

Hi, Oleksandr! On Dec 06, Oleksandr Byelkin wrote: > revision-id: 895309f441e0262229671dcec5d49c4ff8f8ca2c (mariadb-5.5.53-16-g895309f) > parent(s): 18cdff6765b954431934f6e6d0b1e281c8f0e0b8 > committer: Oleksandr Byelkin > timestamp: 2016-12-06 14:18:24 +0100 > message: > > MDEV-10663: Use of Inline table columns in HAVING clause throws 1463 Error > > check for VIEW/DERIVED fields ok to push! Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

1 0

Re: [Maria-developers] [Commits] f79d9d4: [MDEV]-11162 Assertion failing because cleanup does not get called
by Nirbhay Choubey 06 Dec '16

06 Dec '16

Hi Varun, On Tue, Dec 6, 2016 at 9:34 AM, Varun <varunraiko1803(a)gmail.com> wrote: > revision-id: f79d9d454bc3fa6fecb34fec850752a688c0ccc7 > (mariadb-10.0.28-17-gf79d9d4) > parent(s): e99990c631905cd923257331ef124a2bc9276e0d > author: Varun > committer: Varun > timestamp: 2016-12-06 19:48:58 +0530 > message: > > [MDEV]-11162 Assertion failing because cleanup does not get called > Looks like you forgot to add the result file. Besides, since the test is simple, you could instead add it to an existing _debug_ select/optimizer test. Best, Nirbhay > --- > mysql-test/t/bug-11162.test | 6 ++++++ > sql/sql_select.cc | 1 + > 2 files changed, 7 insertions(+) > > diff --git a/mysql-test/t/bug-11162.test b/mysql-test/t/bug-11162.test > new file mode 100644 > index 0000000..5924851 > --- /dev/null > +++ b/mysql-test/t/bug-11162.test > @@ -0,0 +1,6 @@ > +--source include/have_innodb.inc > + > +CREATE TABLE t1 (i INT) ENGINE=InnoDB; > +SELECT ( SELECT DISTINCT GROUP_CONCAT(SLEEP(0)) FROM t1 GROUP BY i ); > +SELECT i FROM t1 order by i LIMIT 1; > +drop table t1; > diff --git a/sql/sql_select.cc b/sql/sql_select.cc > index 2db9a2b..f345d3c 100644 > --- a/sql/sql_select.cc > +++ b/sql/sql_select.cc > @@ -3144,6 +3144,7 @@ JOIN::destroy() > */ > tmp_table_param.cleanup(); > tmp_join->tmp_table_param.copy_field= 0; > + cleanup(1); > DBUG_RETURN(tmp_join->destroy()); > } > cond_equal= 0; > _______________________________________________ > commits mailing list > commits(a)mariadb.org > https://lists.askmonty.org/cgi-bin/mailman/listinfo/commits

1 0