- developers - lists.mariadb.org

[Maria-developers] Please review MDEV-10717 Assertion `!null_value' failed in virtual bool Item::send(Protocol*, String*)
by Alexander Barkov 05 Dec '16

05 Dec '16

Hello Sergei, Please review a patch for MDEV-10717. Thanks!

2 1

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= … [View More]

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; + … [View More]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 [View Less]

2 2

Re: [Maria-developers] MDEV-11330 Split Item_func_hybrid_field_type::val_xxx() into methods in Type_handler
by Vicențiu Ciorbaru 05 Dec '16

05 Dec '16

Hi Alexander! I find this version much cleaner! Looks good to me. What I'd really like to see later is if we can write some unit tests for these conversions. Not mtr linked tests but actual unit tests specifically using these Items. I am up for chipping in and writing some myself. One thing I'd like to discuss later in the context of reentrant functions are these side effects such as "return (null_value= true);" when fetching data from an Item. I think they should go from base items and only … [View More]

1 0

[Maria-developers] MariaRocks port: why handler::init_with_fields() call is needed + call for comments
by Sergey Petrunia 02 Dec '16

02 Dec '16

Hello, I think me and Serg have tried having this discussion before on several occasions but were not successful due to me not having the details back then. Now I'm trying with the details. == Description of the issue == Look at MariaDB, the code in TABLE_SHARE::init_from_binary_frm_image (or open_binary_frm() in mysql-5.6 codebase) The code goes like this: ... /* Allocate handler */ if (!(handler_file= get_new_handler(share, thd->mem_root, … [View More]

1 0

Re: [Maria-developers] [Commits] cf04c06: MDEV-10435 crash with bad stat tables.
by Sergei Golubchik 01 Dec '16

01 Dec '16

Hi, Alexey! On Nov 02, Alexey Botchkov wrote: > revision-id: cf04c06a58e3d0e700491f8f9167e9323cf1de1d (mariadb-10.1.18-28-gcf04c06) > parent(s): c18054deb2b5cfcf1f13aa71574406f2bafb87c6 > committer: Alexey Botchkov > timestamp: 2016-11-02 13:02:32 +0400 > message: > > MDEV-10435 crash with bad stat tables. > > Functions from sql/statistics.cc don't seem to expect > stat tables to fail or to have inadequate structure. > Table open … [View More]

1 0

[Maria-developers] 10.2 tree, innodb_gis.alter_spatial_index test broken after your push
by Sergey Petrunia 01 Dec '16

01 Dec '16

Hi Jan, Your push https://github.com/MariaDB/server/commit/dc9f919f27fccfeb0de3ab392f33bc5efd… broke the innodb_gis.alter_spatial_index test in 10.2 tree. BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://s.petrunia.net/blog

1 0

Re: [Maria-developers] [Commits] 2c9bb42: MDEV-11432 Change the informational redo log format tag to "MariaDB 10.2.3"
by Jan Lindström 01 Dec '16

01 Dec '16

Hi, ok to push. R: Jan On Thu, Dec 1, 2016 at 8:36 AM, <marko.makela(a)mariadb.com> wrote: > revision-id: 2c9bb42d901fc4f48f4884e4a85af74eae6d0929 > (mariadb-10.2.2-91-g2c9bb42) > parent(s): dc9f919f27fccfeb0de3ab392f33bc5efdfd59a0 > author: Marko Mäkelä > committer: Marko Mäkelä > timestamp: 2016-12-01 08:28:59 +0200 > message: > > MDEV-11432 Change the informational redo log format tag to "MariaDB 10.2.3" > > MariaDB 10.2 incorporates MySQL 5.7. … [View More]MySQL 5.7.9 (the first GA release > of the series) introduced an informational field to the InnoDB redo log > header, which identifies the server version where the redo log files > were created (initialized, resized or updated), in > WL#8845: InnoDB: Redo log format version identifier. > > The informational message would be displayed to the user, for example > if someone tries to start up MySQL 8.0 after killing a MariaDB 10.2 server. > In the current MariaDB 10.2 source code, the identifier string would > misleadingly say "MySQL 5.7.14" (using the hard-coded version number in > univ.i) instead of "MariaDB 10.2.3" (using the contents of the VERSION > file, the build system copies to config.h and my_config.h). > > This is only a cosmetic change. The compatibility check is based on a > numeric identifier. > > We should probably also change the numeric identifier and some logic > around it. MariaDB 10.2 should refuse to recover from a crashed MySQL 5.7 > instance, because the redo log might contain references to shared > tablespaces, > which are not supported by MariaDB 10.2. Also, when MariaDB 10.2 creates > an encrypted redo log, there should be a redo log format version tag that > will prevent MySQL 5.7 or 8.0 from starting up. > > --- > storage/innobase/include/log0log.h | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/storage/innobase/include/log0log.h > b/storage/innobase/include/log0log.h > index c1f2570..caa067c 100644 > --- a/storage/innobase/include/log0log.h > +++ b/storage/innobase/include/log0log.h > @@ -555,7 +555,11 @@ or the MySQL version that created the redo log file. > */ > /** End of the log file creator field. */ > #define LOG_HEADER_CREATOR_END (LOG_HEADER_CREATOR + 32) > /** Contents of the LOG_HEADER_CREATOR field */ > -#define LOG_HEADER_CREATOR_CURRENT "MySQL " INNODB_VERSION_STR > +#define LOG_HEADER_CREATOR_CURRENT \ > + "MariaDB " \ > + IB_TO_STR(MYSQL_VERSION_MAJOR) "." \ > + IB_TO_STR(MYSQL_VERSION_MINOR) "." \ > + IB_TO_STR(MYSQL_VERSION_PATCH) > > /** The redo log format identifier corresponding to the current format > version. > Stored in LOG_HEADER_FORMAT. */ > _______________________________________________ > commits mailing list > commits(a)mariadb.org > https://lists.askmonty.org/cgi-bin/mailman/listinfo/commits [View Less]

1 0

Re: [Maria-developers] Please review MDEV-11298 Split Item_func_hex::val_str_ascii() into virtual methods in Type_handler
by Alexander Barkov 01 Dec '16

01 Dec '16

Hello Alexey, Thanks for review! I addressed your suggestions. Also, Vicentiu expressed a concern (when reviewing another patch with the same approach) that this kind of cast that I used in sql_type.cc might be unsafe, because it's non-standard. We're discussion other possible solutions now. So I thought that in the meanwhile for the built-in data types we can create methods: String *val_str_ascii_from_val_int(String *str); String *val_str_ascii_from_val_real(String *str); String *… [View More]val_str_ascii_from_val_str(String *str); and use them from Type_handler_xxx. A new patch is attached. Please see comments below. On 11/30/2016 05:46 PM, Alexey Botchkov wrote: > Hi, Alexander! > > Basically ok with the patch. > Few comments though: > > > +String *Item_func_hex::val_str_ascii_from_int(String *str, ulonglong > num) > > { > ... > > + char ans[65], *ptr; > > + if (!(ptr= longlong2str(num, ans, 16)) || > > + str->copy(ans, (uint32) (ptr - ans), &my_charset_numeric)) > > + return make_empty_result(); // End of memory > > + return str; > > } > > > We can avoid extra copying here like this: > > { > char *n_end; > str->set_charset(&my_charset_numeric); > if (str->alloc(65) || > !(n_end= longlong2str(num, (char *) str->ptr(), 16))) > return make_empty_result(); > str->length((uint32) (n_end - str->ptr()); > return str; > } Thanks for the idea. It's nice to fix it at once. I moved this to String rather than Item_func_hex though: - this helps to avoid (char*) str->ptr(). - it can be useful for other purposes I tried to move octex2hex() from passport.c to int2str.c and reuse it in sql_string.cc, but failed because of strange compilation failures in mysqlbinlog.cc which includes both password.c and sql_string.cc using #include. From my understanding there is a bug in password.c: instead of #include <mysql.h> it should be: #include "mysql.h" I would like to avoid making changes in here for now, so I gave up trying to reuse octet2str(). Instead of that I added APPEND_HEX() and reused it in 3 places in sql_string.cc. > > > class Item_func_hex :public Item_str_ascii_func > > { > .... > > Shouldn't we implement this too: ? > const Type_handler *Item_func_hex::type_handler() > { return m_handler; } Not really. Type handler for Item_func_hex is type_handler_varchar, and it does not depend on the data type of the argument. To avoid confusion, I renamed m_handler to m_arg0_type_handler. > Personally i wouldn't add the m_handler member at all and just do > const Type_handler *Item_func_hex::type_handler() > { return args[0]->m_handler; } > Are you sure the m_handler member works faster? Well you can just forget > that > comment by now, but i'd like to test this someday. I think we should cache it, because it can be expensive to get the handler of args[0] on every row, as it involves virtual methods, which can call more virtual methods recursively. I added a comment about this. > > > +class Item_func_hex_str: public Item_func_hex > > +{ > > +public: > > + String *val_str_ascii(String *str) > > + { > > + /* Convert given string to a hex string, character by character */ > > + String *res= args[0]->val_str(str); > > + if ((null_value= (!res || tmp_value.alloc(res->length() * 2 + 1)))) > > + return 0; > > + tmp_value.length(res->length() * 2); > > + tmp_value.set_charset(&my_charset_latin1); > > + octet2hex((char*) tmp_value.ptr(), res->ptr(), res->length()); > > + return &tmp_value; > > + } > > +}; > > I think it makes more sence to switch the *str and tmp_value > usage here. So the caller gets the 'str' value he sends to the > function as a result. Not that it changes a lot, still i think it's > nicer. Good idea! Done. > > Best regards. > HF > [View Less]

1 0

[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 … [View More]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 [View Less]

1 0