- developers - lists.mariadb.org

Re: [Maria-developers] [Commits] 2950feb5c09: MDEV-9959: A serious MariaDB server performance bug
by Sergey Petrunia 26 Mar '18

26 Mar '18

Hi Varun, Please find my feedback below. As disucssed before: please also run the MDEV's test case with this patch and post a comment with the result. On Fri, Mar 23, 2018 at 05:17:57PM +0530, Varun wrote: > revision-id: 2950feb5c09033843a6a32c7431421b45d5812dd (mariadb-10.3.0-646-g2950feb5c09) > parent(s): 9ef7ab221d70630155206910de8a6053db097164 > author: Varun Gupta > committer: Varun Gupta > timestamp: 2018-03-23 17:17:09 +0530 > message: > > MDEV-9959: A serious MariaDB server performance bug > > step#1: if a derived table has SELECT DISTINCT, provide index statistics for it so that the join optimizer in the > upper select knows that ref access to the table will produce one row. > > Added handling for multiple selects in the derived table > > --- > mysql-test/r/mdev9959.result | 64 ++++++++++++++++++++++++++++++++++++++++++++ > mysql-test/t/mdev9959.test | 28 +++++++++++++++++++ > sql/sql_lex.h | 2 ++ > sql/sql_union.cc | 36 +++++++++++++++++++++++++ > sql/table.cc | 26 +++++++++++++++--- > 5 files changed, 153 insertions(+), 3 deletions(-) > > diff --git a/mysql-test/r/mdev9959.result b/mysql-test/r/mdev9959.result > new file mode 100644 > index 00000000000..5269ba9b734 > --- /dev/null > +++ b/mysql-test/r/mdev9959.result > @@ -0,0 +1,64 @@ > +create table t1(a int); > +insert into t1 values (1),(2),(3),(4),(5),(6); > +create table t2(a int, b int,c int); > +insert into t2(a,b,c) values (1,1,2),(2,2,3),(3,1,4),(4,2,2),(5,1,1),(6,2,5); > +create table t3(a int, b int); > +insert into t3(a,b) values (1,1),(2,2),(2,1),(1,2),(5,1),(9,2); > +analyze select * from t1 , ( (select distinct t2.a from t2 group by c))q where t1.a=q.a; > +id select_type table type possible_keys key key_len ref rows r_rows filtered r_filtered Extra > +1 PRIMARY t1 ALL NULL NULL NULL NULL 6 6.00 100.00 100.00 Using where > +1 PRIMARY <derived2> ref key0 key0 5 test.t1.a 1 0.83 100.00 100.00 > +2 DERIVED t2 ALL NULL NULL NULL NULL 6 6.00 100.00 100.00 Using temporary; Using filesort It is not at all clear which parts of the above output matter and which don't, for example, does #rows matter for table t1? Please add a comment (into .test file with --echo # ) describing that table "<derived2>" should have type=ref and rows=1 > +select * from t1 , ( (select distinct t2.a from t2 group by c))q where t1.a=q.a; > +a a > +1 1 > +2 2 > +3 3 > +5 5 > +6 6 > +analyze select * from t1 , ( (select t2.a from t2 group by c))q where t1.a=q.a; > +id select_type table type possible_keys key key_len ref rows r_rows filtered r_filtered Extra > +1 PRIMARY t1 ALL NULL NULL NULL NULL 6 6.00 100.00 100.00 Using where > +1 PRIMARY <derived2> ref key0 key0 5 test.t1.a 2 0.83 100.00 100.00 > +2 DERIVED t2 ALL NULL NULL NULL NULL 6 6.00 100.00 100.00 Using temporary; Using filesort > +select * from t1 , ( (select t2.a from t2 group by c))q where t1.a=q.a; > +a a > +1 1 > +2 2 > +3 3 > +5 5 > +6 6 > +analyze select * from t1 , ( (select distinct t2.a from t2 group by c) union all (select distinct t2.a from t2 group by c))q where t1.a=q.a; > +id select_type table type possible_keys key key_len ref rows r_rows filtered r_filtered Extra > +1 PRIMARY t1 ALL NULL NULL NULL NULL 6 6.00 100.00 100.00 Using where > +1 PRIMARY <derived2> ref key0 key0 5 test.t1.a 2 1.67 100.00 100.00 > +2 DERIVED t2 ALL NULL NULL NULL NULL 6 6.00 100.00 100.00 Using temporary; Using filesort > +3 UNION t2 ALL NULL NULL NULL NULL 6 6.00 100.00 100.00 Using temporary; Using filesort What does the above test show? We've got type=ref, #rows = 2 for the derived table. The estimate is correct but where does it come from? As far as I understand this patch, it only sets the estimate rows=1 for derived tables. How do we arrive at the value of 2? (If we would have gotten rows=2 without this patch, what's the point of having the testcase as part of this patch?) > +select * from t1 , ( (select distinct t2.a from t2 group by c) union all (select distinct t2.a from t2 group by c))q where t1.a=q.a; > +a a > +1 1 > +1 1 > +2 2 > +2 2 > +3 3 > +3 3 > +5 5 > +5 5 > +6 6 > +6 6 > +analyze select * from t1 , ( (select distinct t2.a from t2 group by c) union (select distinct t2.a from t2 group by c) union (select t3.a from t3 group by b))q where t1.a=q.a; > +id select_type table type possible_keys key key_len ref rows r_rows filtered r_filtered Extra > +1 PRIMARY t1 ALL NULL NULL NULL NULL 6 6.00 100.00 100.00 Using where > +1 PRIMARY <derived2> ref key0 key0 5 test.t1.a 2 0.83 100.00 100.00 > +2 DERIVED t2 ALL NULL NULL NULL NULL 6 6.00 100.00 100.00 Using temporary; Using filesort > +3 UNION t2 ALL NULL NULL NULL NULL 6 6.00 100.00 100.00 Using temporary; Using filesort > +4 UNION t3 ALL NULL NULL NULL NULL 6 6.00 100.00 100.00 Using temporary; Using filesort > +NULL UNION RESULT <union2,3,4> ALL NULL NULL NULL NULL NULL 5.00 NULL NULL > +select * from t1 , ( (select distinct t2.a from t2 group by c) union (select distinct t2.a from t2 group by c) union (select t3.a from t3 group by b))q where t1.a=q.a; > +a a > +1 1 > +2 2 > +3 3 > +5 5 > +6 6 > +drop table t1,t2,t3; > diff --git a/mysql-test/t/mdev9959.test b/mysql-test/t/mdev9959.test > new file mode 100644 > index 00000000000..77970f363ac > --- /dev/null > +++ b/mysql-test/t/mdev9959.test > @@ -0,0 +1,28 @@ > +create table t1(a int); > +insert into t1 values (1),(2),(3),(4),(5),(6); > +create table t2(a int, b int,c int); > +insert into t2(a,b,c) values (1,1,2),(2,2,3),(3,1,4),(4,2,2),(5,1,1),(6,2,5); > +create table t3(a int, b int); > +insert into t3(a,b) values (1,1),(2,2),(2,1),(1,2),(5,1),(9,2); > + > +# one select in derived table > + > +#with distinct > +analyze select * from t1 , ( (select distinct t2.a from t2 group by c))q where t1.a=q.a; > +select * from t1 , ( (select distinct t2.a from t2 group by c))q where t1.a=q.a; > +# without distinct > +analyze select * from t1 , ( (select t2.a from t2 group by c))q where t1.a=q.a; > +select * from t1 , ( (select t2.a from t2 group by c))q where t1.a=q.a; > + > +# multiple selects in derived table > + > +#UNION ALL > +analyze select * from t1 , ( (select distinct t2.a from t2 group by c) union all (select distinct t2.a from t2 group by c))q where t1.a=q.a; > +select * from t1 , ( (select distinct t2.a from t2 group by c) union all (select distinct t2.a from t2 group by c))q where t1.a=q.a; > + > +#UNION > +analyze select * from t1 , ( (select distinct t2.a from t2 group by c) union (select distinct t2.a from t2 group by c) union (select t3.a from t3 group by b))q where t1.a=q.a; > +select * from t1 , ( (select distinct t2.a from t2 group by c) union (select distinct t2.a from t2 group by c) union (select t3.a from t3 group by b))q where t1.a=q.a; > + > +drop table t1,t2,t3; > + > diff --git a/sql/sql_lex.h b/sql/sql_lex.h > index f0241a32acf..4128c1dd300 100644 > --- a/sql/sql_lex.h > +++ b/sql/sql_lex.h > @@ -761,6 +761,7 @@ class st_select_lex_unit: public st_select_lex_node { > Procedure *last_procedure; /* Pointer to procedure, if such exists */ > > bool columns_are_renamed; > + bool union_all; /* TRUE if we have UNION ALL operation */ This variable is checked but it is never set? > > void init_query(); > st_select_lex* outer_select(); > @@ -800,6 +801,7 @@ class st_select_lex_unit: public st_select_lex_node { > bool union_needs_tmp_table(); > > void set_unique_exclude(); > + bool check_distinct_in_union(); > > friend struct LEX; > friend int subselect_union_engine::exec(); > diff --git a/sql/sql_union.cc b/sql/sql_union.cc > index 857c9a117f5..43ce0418fe5 100644 > --- a/sql/sql_union.cc > +++ b/sql/sql_union.cc > @@ -1960,3 +1960,39 @@ void st_select_lex_unit::set_unique_exclude() > } > } > } > + > +/* > + Check if the selects in the derived table can give distinct rows I think "can" is the wrong word here. UNION/INTERSECT/EXCEPT output "can" be distinct if the underlying selects produce distinct sets of rows. What we are checking is whether the output will be distinct regardless of what data is there in the queried tables. > + > + for example: > + select * from > + ((select t1.a from t1) op (select t2.a from t2) op (select t3.a from t3)); > + the op here being UNION/INTERSECT/EXCEPT > + > + so this function would check if the derived table like the case above > + can give distinct rows or not. Please describe the criteria and provide a couple of examples (either here or in the comments inside the function). > + > + @retval false Distinct rows are not guarenteed > + @retval true Distinct rows are guanrenteed Typos in the above lines. > + > +*/ > + > +bool st_select_lex_unit::check_distinct_in_union() > +{ > + bool is_intersect_present=FALSE; > + st_select_lex* first= first_select(); > + for(st_select_lex *sl=first; sl ; sl=sl->next_select()) > + is_intersect_present|= (sl->linkage == INTERSECT_TYPE); > + > + if (!union_all) > + return true; > + else > + { > + if (union_select) > + { > + if (!is_intersect_present && !union_select->next_select()) > + return true; > + } > + } > + return false; > +} > diff --git a/sql/table.cc b/sql/table.cc > index f42455a8413..c474d2ed4d7 100644 > --- a/sql/table.cc > +++ b/sql/table.cc > @@ -7113,9 +7113,29 @@ bool TABLE::add_tmp_key(uint key, uint key_parts, > if (!keyinfo->rec_per_key) > return TRUE; > bzero(keyinfo->rec_per_key, sizeof(ulong)*key_parts); > - st_select_lex* lex= pos_in_table_list ? pos_in_table_list->derived->first_select(): NULL; > - if (lex && (lex->options & SELECT_DISTINCT)) > - keyinfo->rec_per_key[key_parts-1]=1; > + > + /* > + For the case when there is a derived table that would give distinct rows, > + the index statistics are passed to the join optimizer to tell that > + a ref access to the derived table will produce only one row. > + */ > + > + st_select_lex_unit* derived= pos_in_table_list ? pos_in_table_list->derived: NULL; > + if (derived) > + { > + /* > + This handles the case when we have a single select in the derived table > + */ > + st_select_lex* first= derived->first_select(); > + if (first && !first->is_part_of_union() && > + first->options & SELECT_DISTINCT) > + keyinfo->rec_per_key[key_parts-1]=1; > + else > + { > + if (check_distinct_in_union()) > + keyinfo->rec_per_key[key_parts-1]=1; > + } I think it would be better if there was a single "if", like so: if ((this is a non-union select with SELECT_DISTINCT || check_distinct_in_union()) { keyinfo->rec_per_key[key_parts-1]=1; } > + } > keyinfo->read_stats= NULL; > keyinfo->collected_stats= NULL; > BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://s.petrunia.net/blog

1 0

Re: [Maria-developers] [Commits] b718e87caaf: MDEV-11274: Executing EXPLAIN of complex query over join limit causes server to crash
by Sergey Petrunia 26 Mar '18

26 Mar '18

Hi Varun, On Sat, Mar 24, 2018 at 03:31:59AM +0530, Varun wrote: > revision-id: b718e87caaf2d0a60c19cd042dfc04868f4bb0b4 (mariadb-5.5.56-187-gb718e87caaf) > parent(s): dfc79ae949f6d831ff65a08a5d32d14b59ba06e6 > author: Varun Gupta > committer: Varun Gupta > timestamp: 2018-03-24 03:31:18 +0530 > message: > > MDEV-11274: Executing EXPLAIN of complex query over join limit causes server to crash > > For this case we have a view that is mergeable but we are not able to merge it in the > parent select because that would exceed the maximum tables allowed in the join list, so we > materialise this view > TABLE_LIST::dervied is NULL for such views, it is only set for views which have ALGORITHM=TEMPTABLE > Fixed by making sure TABLE_LIST::derived is set for views that could not be merged > > --- > mysql-test/r/view.result | 197 +++++++++++++++++++++++++++++++++++++++++++++++ > mysql-test/t/view.test | 196 ++++++++++++++++++++++++++++++++++++++++++++++ > sql/table.h | 2 + > 3 files changed, 395 insertions(+) > > diff --git a/sql/table.h b/sql/table.h > index 98f8c7ad73f..7b58f62e659 100644 > --- a/sql/table.h > +++ b/sql/table.h > @@ -46,6 +46,7 @@ struct TABLE_LIST; > class ACL_internal_schema_access; > class ACL_internal_table_access; > class Field; > +struct LEX; > > /* > Used to identify NESTED_JOIN structures within a join (applicable only to What is the above needed for? Please remove this change. Ok to push after this is addressed. > @@ -2125,6 +2126,7 @@ struct TABLE_LIST > DBUG_PRINT("enter", ("Alias: '%s' Unit: %p", > (alias ? alias : "<NULL>"), > get_unit())); > + derived= get_unit(); > derived_type= ((derived_type & (derived ? DTYPE_MASK : DTYPE_VIEW)) | > DTYPE_TABLE | DTYPE_MATERIALIZE); > set_check_materialized(); BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://s.petrunia.net/blog

1 0

Re: [Maria-developers] 108cdbf: MDEV-10871 Add logging capability to pam_user_map.c
by Sergei Golubchik 24 Mar '18

24 Mar '18

Hi, Alexey! On Mar 23, Alexey Botchkov wrote: > > Technically yes, there can be many arguments. > BTW i think some other arguments make sence. > We can specify the configuration file name in arguments (now it's hardcoded > as "/etc/security/user_map.conf"), > then we can specify some 'user: user_map' pairs as arguments, > to make an addition to what is in the user_map.conf or we can go by > withouth the .conf > file at all. > Is it worth adding the todo-mdev? Agree. File name — makes sense, user:map pairs probably not, I don't remember any other pam module doing that. Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

1 0

[Maria-developers] MDEV-15620 Crash when using "SET @@NEW.a=expr" inside a trigger
by Alexander Barkov 23 Mar '18

23 Mar '18

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

1 0

Re: [Maria-developers] 108cdbf: MDEV-10871 Add logging capability to pam_user_map.c
by Sergei Golubchik 22 Mar '18

22 Mar '18

Hi, Alexey! Looks good, of course. See few minor comments below. On Mar 18, Alexey Botchkov wrote: > revision-id: 108cdbf854df6af1ed2c0fc3a63e1786f6b4b7b4 (mariadb-10.1.31-61-g108cdbf) > parent(s): a0c722d853071e605ea025168da1b01bfe21092c > committer: Alexey Botchkov > timestamp: 2018-03-18 12:03:59 +0400 > message: > > MDEV-10871 Add logging capability to pam_user_map.c. > > 'debug' option added to the pam_user_map.so. > It writes the verbose report to the syslog. don't indent the commit comment, please > > --- > plugin/auth_pam/mapper/pam_user_map.c | 74 +++++++++++++++++++++++++++++++++-- > 1 file changed, 71 insertions(+), 3 deletions(-) > > diff --git a/plugin/auth_pam/mapper/pam_user_map.c b/plugin/auth_pam/mapper/pam_user_map.c > index e62be94..1706630 100644 > --- a/plugin/auth_pam/mapper/pam_user_map.c > +++ b/plugin/auth_pam/mapper/pam_user_map.c > @@ -26,10 +26,13 @@ top: accounting > > #include <stdlib.h> > #include <stdio.h> > +#include <ctype.h> > +#include <string.h> > #include <syslog.h> > #include <grp.h> > #include <pwd.h> > > +#include <security/pam_ext.h> > #include <security/pam_modules.h> > > #define FILENAME "/etc/security/user_map.conf" > @@ -90,9 +93,40 @@ static int user_in_group(const gid_t *user_groups, int ng,const char *group) > } > > > +static void print_groups(pam_handle_t *pamh, const gid_t *user_groups, int ng) > +{ > + char buf[256]; > + char *c_buf= buf, *buf_end= buf+sizeof(buf)-1; > + struct group *gr; > + > + for (; ng > 0; user_groups++, ng--) > + { > + char *c; > + if (!(gr= getgrgid(*user_groups)) || > + !(c= gr->gr_name)) > + continue; > + while (*c) > + { > + if (c_buf == buf_end) > + break; > + *(c_buf++)= *c; > + } > + if (c_buf == buf_end) > + break; > + *(c_buf++)= ','; > + } > + *c_buf= 0; > + pam_syslog(pamh, LOG_DEBUG, "User belongs to groups [%s].\n", buf); s/pam_syslog/SYSLOG_DEBUG/ ? > +} > + > + > +static const char debug_keyword[]= "debug"; > +#define SYSLOG_DEBUG if (mode_debug) pam_syslog > + > int pam_sm_authenticate(pam_handle_t *pamh, int flags, > int argc, const char *argv[]) > { > + int mode_debug= 0; > int pam_err, line= 0; > const char *username; > char buf[256]; > @@ -101,6 +135,14 @@ int pam_sm_authenticate(pam_handle_t *pamh, int flags, > gid_t *groups= group_buffer; > int n_groups= -1; > > + for (; argc > 0; argv++) > + { > + if (strcasecmp(*argv, debug_keyword) == 0) > + mode_debug= 1; > + } can there be many arguments? > + > + SYSLOG_DEBUG(pamh, LOG_DEBUG, "Opening file '%s'.\n", FILENAME); > > + > f= fopen(FILENAME, "r"); > if (f == NULL) > { > @@ -110,12 +152,18 @@ int pam_sm_authenticate(pam_handle_t *pamh, int flags, > > pam_err = pam_get_item(pamh, PAM_USER, (const void**)&username); > if (pam_err != PAM_SUCCESS) > + { > + pam_syslog(pamh, LOG_ERR, "Cannot get username.\n"); > goto ret; > + } > + > + SYSLOG_DEBUG(pamh, LOG_DEBUG, "Incoming username '%s'.\n", username); > > while (fgets(buf, sizeof(buf), f) != NULL) > { > char *s= buf, *from, *to, *end_from, *end_to; > int check_group; > + int cmp_result; > > line++; > > @@ -124,7 +172,11 @@ int pam_sm_authenticate(pam_handle_t *pamh, int flags, > if ((check_group= *s == '@')) > { > if (n_groups < 0) > + { > n_groups= populate_user_groups(username, &groups); > + if (mode_debug) > + print_groups(pamh, groups, n_groups); > + } > s++; > } > from= s; > @@ -139,14 +191,29 @@ int pam_sm_authenticate(pam_handle_t *pamh, int flags, > if (end_to == to) goto syntax_error; > > *end_from= *end_to= 0; > - if (check_group ? > - user_in_group(groups, n_groups, from) : > - (strcmp(username, from) == 0)) > + > + cmp_result= 0; looks redundant ^^^ > + if (check_group) > + { > + cmp_result= user_in_group(groups, n_groups, from); > + SYSLOG_DEBUG(pamh, LOG_DEBUG, "Check if user is in gourp '%s': %s\n", s/gourp/group/ > + from, cmp_result ? "YES":"NO"); > + } > + else > + { > + cmp_result= (strcmp(username, from) == 0); > + SYSLOG_DEBUG(pamh, LOG_DEBUG, "Check if username '%s': %s\n", > + from, cmp_result ? "YES":"NO"); > + } > + if (cmp_result) > { > pam_err= pam_set_item(pamh, PAM_USER, to); > + SYSLOG_DEBUG(pamh, LOG_DEBUG, "User authenticated as '%s'\n", to); may be "user mapped to %s" ? > goto ret; > } > } > + > + SYSLOG_DEBUG(pamh, LOG_DEBUG, "User not found in the list.\n"); > pam_err= PAM_AUTH_ERR; > goto ret; > > @@ -162,6 +229,7 @@ int pam_sm_authenticate(pam_handle_t *pamh, int flags, > return pam_err; > } > > + > int pam_sm_setcred(pam_handle_t *pamh, int flags, > int argc, const char *argv[]) > { Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

1 0

[Maria-developers] GSoC 2018 - Histograms with equal-width bins
by Teodor Niculescu 22 Mar '18

22 Mar '18

Greetings, My name is Teodor Niculescu and I am a second year undergraduate student at the The Faculty of Automatic Control and Computers, University Politehnica of Bucharest, Romania. I am interested in contributing to MariaDB as part of this years Google Summer of Code. I am interested in the project: MDEV-12313 Histograms with equal-width bins. I would like to ask you about a good place to start reading the codebase, in order to help me get an idea about what I need to do for this project. I look forward to hearing from you. Sincerely, Teodor.

2 1

Re: [Maria-developers] MDEV-15492: Subquery crash similar to MDEV-10050
by Sergei Golubchik 21 Mar '18

21 Mar '18

Hi, Oleksandr! On Mar 21, Oleksandr Byelkin wrote: > revision-id: 49a443c53a7f01812299f20e94c7209579efd881 (mariadb-5.5.59-45-g49a443c53a7) > parent(s): 0943b33de3daa0fcbf58803be8e991941de63218 > author: Oleksandr Byelkin > committer: Oleksandr Byelkin > timestamp: 2018-03-21 12:13:37 +0100 > message: > > MDEV-15492: Subquery crash similar to MDEV-10050 > > Detection of first execution of PS fixed. > More debug info. > > diff --git a/sql/sql_prepare.cc b/sql/sql_prepare.cc > index a3bf9d6c93c..369a9f3a804 100644 > --- a/sql/sql_prepare.cc > +++ b/sql/sql_prepare.cc > @@ -3819,6 +3819,7 @@ bool Prepared_statement::execute(String *expanded_query, bool open_cursor) > Statement stmt_backup; > Query_arena *old_stmt_arena; > bool error= TRUE; > + bool qc_executed; > > char saved_cur_db_name_buf[SAFE_NAME_LEN+1]; > LEX_STRING saved_cur_db_name= > @@ -3931,12 +3932,14 @@ bool Prepared_statement::execute(String *expanded_query, bool open_cursor) > 1); > error= mysql_execute_command(thd); > MYSQL_QUERY_EXEC_DONE(error); > + qc_executed= FALSE; > } > else > { > thd->lex->sql_command= SQLCOM_SELECT; > status_var_increment(thd->status_var.com_stat[SQLCOM_SELECT]); > thd->update_stats(); > + qc_executed= TRUE; > } > } mostly ok, but this leaves one code path where qc_executed stays uninitialized. Better assign a default value of FALSE when declaring a variable, and set it to TRUE when really executed. ok to push after that. > > @@ -3960,7 +3963,7 @@ bool Prepared_statement::execute(String *expanded_query, bool open_cursor) > thd->set_statement(&stmt_backup); > thd->stmt_arena= old_stmt_arena; > > - if (state == Query_arena::STMT_PREPARED) > + if (state == Query_arena::STMT_PREPARED && !qc_executed) > state= Query_arena::STMT_EXECUTED; > > if (error == 0 && this->lex->sql_command == SQLCOM_CALL) Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

1 0

Re: [Maria-developers] [Commits] b960f68b102: MDEV-11274: Executing EXPLAIN of complex query over join limit causes server to crash
by Sergey Petrunia 21 Mar '18

21 Mar '18

Hi Varun, On Tue, Jan 23, 2018 at 10:49:37AM +0530, Varun wrote: > revision-id: b960f68b102ec72cf75252b357760e99c5dffc18 (mariadb-5.5.56-144-gb960f68b102) > parent(s): a7a4519a40c58947796c6d9b2e4e58acc18aeef8 > author: Varun Gupta > committer: Varun Gupta > timestamp: 2018-01-23 10:31:02 +0530 > message: > > MDEV-11274: Executing EXPLAIN of complex query over join limit causes server to crash > > For this case we have a view that is mergeable but we are not able to merge it in the > parent select because that would exceed the maximum tables allowed in the join list, so we > materialise this view > TABLE_LIST::dervied is NULL for such views, it is only set for views which have ALGORITHM=TEMPTABLE > Fixed by using SELECT_LEX_UNIT representing the result of the query within the view definition ... > diff --git a/sql/table.cc b/sql/table.cc > index 9cade76cb78..8f377d5bda8 100644 > --- a/sql/table.cc > +++ b/sql/table.cc > @@ -6817,9 +6817,8 @@ int TABLE_LIST::fetch_number_of_rows() > if (jtbm_subselect) > return 0; > if (is_materialized_derived() && !fill_me) > - > { > - table->file->stats.records= ((select_union*)derived->result)->records; > + table->file->stats.records= ((select_union*)get_unit()->result)->records; > set_if_bigger(table->file->stats.records, 2); So, regular VIEWs with algorithm=TEMPTABLE have derived == view->unit This is set in mysql_make_view(): ... table->derived_type= VIEW_ALGORITHM_TMPTABLE; DBUG_PRINT("info", ("algorithm: TEMPORARY TABLE")); view_select->linkage= DERIVED_TABLE_TYPE; ... However, the view in the example has algorithm=MERGE so the execution doesn't reach that point. The decision to materialized is made in mysql_derived_merge. It calls derived->set_materialized_derived(); which calls set_check_materialized(); I have discussed this with Sanja, and the outcome is that a better solution would be to set TABLE_LIST::derived when we decide that this view is going to be handled with temp.table strategy. set_check_materialized() seems to be the right place to do it (although its name looks confusing. What does one "check"? Probably the name is a leftover from some feature backport from MySQL). Could you change the patch to set TABLE_LIST::derived and see if that works? BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://s.petrunia.net/blog

1 0

Re: [Maria-developers] [Commits] 0f35480f3d6: MDEV-6707: Wrong result (extra row) with group by, multi-part key
by Sergey Petrunia 20 Mar '18

20 Mar '18

Hi Varun, Nice to see feedback for the first review fixed, but there's more to do. Please find more input below. On Fri, Jan 19, 2018 at 01:07:20AM +0530, Varun wrote: > revision-id: 0f35480f3d60fac91bf1f91f8140ac5f55724139 (mariadb-5.5.56-142-g0f35480f3d6) > parent(s): fafdac3365f4943e73bcefd0e0d07d69997a9724 > author: Varun Gupta > committer: Varun Gupta > timestamp: 2018-01-18 23:53:35 +0530 > message: > > MDEV-6707: Wrong result (extra row) with group by, multi-part key > > This case involves using a composite key,few parts of which are involved in GROUP BY and few in the MIN/MAX > functions in the select list. > Ranges are created in accordance with the where condition, so during the execution of such queries, > we try to find a prefix using the fields involved in GROUP BY > and we then check if this newly returned prefix lies within the range we had calculated earlier. > > For queries which use composite key, few parts of which are involved in GROUP BY and few in the MIN/MAX functtions > in the select list, we try to find the prefix of the ranges by using the fields involved in the group by clause. > We get extra rows in the output when we have same partial ranges created for the fields in the GROUP BY clause > > This issue can be fixed if we compare such partial ranges and don't lookup if we see the same prefix again. > > --- > mysql-test/r/range.result | 14 +++++++++++++ > mysql-test/r/range_mrr_icp.result | 14 +++++++++++++ > mysql-test/t/range.test | 13 ++++++++++++ > sql/opt_range.cc | 44 ++++++++++++++++++++++++++++++++++++--- > sql/opt_range.h | 5 +++-- > 5 files changed, 85 insertions(+), 5 deletions(-) > > diff --git a/mysql-test/r/range.result b/mysql-test/r/range.result > index 630a692cef6..72b31de4df4 100644 > --- a/mysql-test/r/range.result > +++ b/mysql-test/r/range.result > @@ -2144,3 +2144,17 @@ value1 1000685 12345 > value1 1003560 12345 > value1 1004807 12345 > drop table t1; > +# > +# MDEV-6707: Wrong result (extra row) with group by, multi-part key > +# > +CREATE TABLE t1 (f1 INT, f2 VARCHAR(1), KEY(f2,f1)) ENGINE=InnoDB; > +INSERT INTO t1 VALUES > +(7,'v'),(0,'s'),(9,'l'),(4,'c'); > +explain > +SELECT MAX(f1), f2 FROM t1 WHERE f2 LIKE 'c%' AND f1 <> 9 GROUP BY f2; > +id select_type table type possible_keys key key_len ref rows Extra > +1 SIMPLE t1 range f2 f2 9 NULL 2 Using where; Using index for group-by > +SELECT MAX(f1), f2 FROM t1 WHERE f2 LIKE 'c%' AND f1 <> 9 GROUP BY f2; > +MAX(f1) f2 > +4 c > +DROP TABLE t1; > diff --git a/mysql-test/r/range_mrr_icp.result b/mysql-test/r/range_mrr_icp.result > index 3f5de5b0189..1050bfcd887 100644 > --- a/mysql-test/r/range_mrr_icp.result > +++ b/mysql-test/r/range_mrr_icp.result > @@ -2146,4 +2146,18 @@ value1 1000685 12345 > value1 1003560 12345 > value1 1004807 12345 > drop table t1; > +# > +# MDEV-6707: Wrong result (extra row) with group by, multi-part key > +# > +CREATE TABLE t1 (f1 INT, f2 VARCHAR(1), KEY(f2,f1)) ENGINE=InnoDB; > +INSERT INTO t1 VALUES > +(7,'v'),(0,'s'),(9,'l'),(4,'c'); > +explain > +SELECT MAX(f1), f2 FROM t1 WHERE f2 LIKE 'c%' AND f1 <> 9 GROUP BY f2; > +id select_type table type possible_keys key key_len ref rows Extra > +1 SIMPLE t1 range f2 f2 9 NULL 2 Using where; Using index for group-by > +SELECT MAX(f1), f2 FROM t1 WHERE f2 LIKE 'c%' AND f1 <> 9 GROUP BY f2; > +MAX(f1) f2 > +4 c > +DROP TABLE t1; > set optimizer_switch=@mrr_icp_extra_tmp; > diff --git a/mysql-test/t/range.test b/mysql-test/t/range.test > index 393ca68e945..62e907b7b4a 100644 > --- a/mysql-test/t/range.test > +++ b/mysql-test/t/range.test > @@ -1718,3 +1718,16 @@ where (key1varchar='value1' AND (key2int <=1 OR key2int > 1)); > --echo # The following must show col1=12345 for all rows: > select * from t1; > drop table t1; > + > +--echo # > +--echo # MDEV-6707: Wrong result (extra row) with group by, multi-part key > +--echo # > + > +CREATE TABLE t1 (f1 INT, f2 VARCHAR(1), KEY(f2,f1)) ENGINE=InnoDB; > +INSERT INTO t1 VALUES > +(7,'v'),(0,'s'),(9,'l'),(4,'c'); > + > +explain > +SELECT MAX(f1), f2 FROM t1 WHERE f2 LIKE 'c%' AND f1 <> 9 GROUP BY f2; > +SELECT MAX(f1), f2 FROM t1 WHERE f2 LIKE 'c%' AND f1 <> 9 GROUP BY f2; > +DROP TABLE t1; > diff --git a/sql/opt_range.cc b/sql/opt_range.cc > index 25a9e729a8b..be959764b16 100644 > --- a/sql/opt_range.cc > +++ b/sql/opt_range.cc > @@ -11249,6 +11249,7 @@ int QUICK_RANGE_SELECT::get_next() > @param prefix_length length of cur_prefix > @param group_key_parts The number of key parts in the group prefix > @param cur_prefix prefix of a key to be searched for > + @param save_last_range INOUT Saving the last range we encountered > > Each subsequent call to the method retrieves the first record that has a > prefix with length prefix_length and which is different from cur_prefix, > @@ -11271,7 +11272,8 @@ int QUICK_RANGE_SELECT::get_next() > > int QUICK_RANGE_SELECT::get_next_prefix(uint prefix_length, > uint group_key_parts, > - uchar *cur_prefix) > + uchar *cur_prefix, > + QUICK_RANGE **save_last_range) > { > DBUG_ENTER("QUICK_RANGE_SELECT::get_next_prefix"); > const key_part_map keypart_map= make_prev_keypart_map(group_key_parts); > @@ -11301,8 +11303,43 @@ int QUICK_RANGE_SELECT::get_next_prefix(uint prefix_length, > last_range= 0; > DBUG_RETURN(HA_ERR_END_OF_FILE); > } > + > last_range= *(cur_range++); > > + /* > + While calculating these prefixes we might encounter a case where there > + woud be same partial ranges for multiple ranges. Typo. > + An example would be > + select max(f1), f2 from t1 where f2 ='c' AND f1 <> 9 group by f2; > + so the ranges would be > + (NULL,c <= f1,f2 <= c,9) > + (c,9 <= f1,f2 <= c,+infinity) This looks as if index was defined on (f1, f2) ? Then the comment doesn't make sense because loose index scan does not support queries in form: select max(key_part1) ... group by key_part2 It only can handle them when the index is on (f2,f1), but this is counterintuitive for the reader that is not familiar with the testcase for this bug. Please rephrase the comment. Please use key_part1 and key_part2 as column names. Tuple comparisons should look like this (note the braces): (a,b) < (c,d) . > + > + In this case for calculating prefixes with the group by field we take up the > + partial ranges involving field f2 those would be > + c<= f2 <=c > + c<= f2 <=c > + > + So we lookup rows with the same prefix in all such ranges and > + then we check for the other part(in this case f1) in ALL the ranges. > + So if a record lies in a range, then it would satisfy both the partial > + ranges in this case and therefore there would be multiple outputs for > + the same row. > + For such cases we should calculate the prefix only when we have the next > + partial range different from the previous one. > + */ > + > + > + if (*save_last_range) > + { > + if (!key_tuple_cmp(key_part_info, (*save_last_range)->min_key, > + last_range->min_key, prefix_length)) > + { > + last_range=NULL; > + continue; > + } > + } > + *save_last_range= last_range; > key_range start_key, end_key; > last_range->make_min_endpoint(&start_key, prefix_length, keypart_map); > last_range->make_max_endpoint(&end_key, prefix_length, keypart_map); > @@ -13315,7 +13352,7 @@ QUICK_GROUP_MIN_MAX_SELECT(TABLE *table, JOIN *join_arg, bool have_min_arg, > seen_first_key(FALSE), doing_key_read(FALSE), min_max_arg_part(min_max_arg_part_arg), > key_infix(key_infix_arg), key_infix_len(key_infix_len_arg), > min_functions_it(NULL), max_functions_it(NULL), > - is_index_scan(is_index_scan_arg) > + is_index_scan(is_index_scan_arg), save_last_range(NULL) This is not the right place for this initialization. It needs to be in QUICK_GROUP_MIN_MAX_SELECT::reset(), as the quick select may be executed multiple times. Testcase: create table ten(a int); insert into ten values (0),(1),(2),(3),(4),(5),(6),(7),(8),(9); MariaDB [test]> select (SELECT MAX(f1) as MAXVAL FROM t1 WHERE f2 LIKE 'c%' AND f1 <> 9 GROUP BY f2) AS SUBQ from ten; +------+ | SUBQ | +------+ | 4 | | 4 | | 4 | | 4 | | 4 | | 4 | | 4 | | 4 | | 4 | | 4 | +------+ 10 rows in set (0.00 sec) Now, let's add a HAVING clause (which always evaluates to TRUE but forces the subquery to be recomputed: MariaDB [test]> select (SELECT MAX(f1) as MAXVAL FROM t1 WHERE f2 LIKE 'c%' AND f1 <> 9 GROUP BY f2 having maxval<100+ten.a) AS SUBQ from ten; +------+ | SUBQ | +------+ | 4 | | NULL | | NULL | | NULL | | NULL | | NULL | | NULL | | NULL | | NULL | | NULL | +------+ 10 rows in set (0.00 sec) To make sure this is loose index, try with IGNORE INDEX: MariaDB [test]> select (SELECT MAX(f1) as MAXVAL FROM t1 ignore index(f2) WHERE f2 LIKE 'c%' AND f1 <> 9 GROUP BY f2 having maxval<100+ten.a) AS SUBQ from ten; +------+ | SUBQ | +------+ | 4 | | 4 | | 4 | | 4 | | 4 | | 4 | | 4 | | 4 | | 4 | | 4 | +------+ 10 rows in set (0.00 sec) Please fix this and add the above into the testcase. > { > head= table; > index= use_index; > @@ -13968,7 +14005,8 @@ int QUICK_GROUP_MIN_MAX_SELECT::next_prefix() > uchar *cur_prefix= seen_first_key ? group_prefix : NULL; > if ((result= quick_prefix_select->get_next_prefix(group_prefix_len, > group_key_parts, > - cur_prefix))) > + cur_prefix, > + &save_last_range))) > DBUG_RETURN(result); > seen_first_key= TRUE; > } > diff --git a/sql/opt_range.h b/sql/opt_range.h > index b8b46ae5ab1..206d65f6da4 100644 > --- a/sql/opt_range.h > +++ b/sql/opt_range.h > @@ -477,7 +477,7 @@ class QUICK_RANGE_SELECT : public QUICK_SELECT_I > int get_next(); > void range_end(); > int get_next_prefix(uint prefix_length, uint group_key_parts, > - uchar *cur_prefix); > + uchar *cur_prefix, QUICK_RANGE **save_last_range); > bool reverse_sorted() { return 0; } > bool unique_key_range(); > int init_ror_merged_scan(bool reuse_handler, MEM_ROOT *alloc); > @@ -916,7 +916,8 @@ class QUICK_GROUP_MIN_MAX_SELECT : public QUICK_SELECT_I > Use index scan to get the next different key instead of jumping into it > through index read > */ > - bool is_index_scan; > + bool is_index_scan; > + QUICK_RANGE *save_last_range; Please add a comment describing the new member variable. > public: > /* > The following two members are public to allow easy access from BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://s.petrunia.net/blog

1 0

[Maria-developers] GSoC 2018 Student Self Introduction & Question
by zhzhzoo zhang 20 Mar '18

20 Mar '18

Hello everybody, I am a first year graduate in Peking University major in math. I started to take part in algorithm contests at grade 7 so I have a lot of programming experience and I am very well trained in algorithms and data structures. In my childhood memory MySQL was the most popular DBMS choice for personal website (LAMP stack). However it was acquired by Oracle as I grew up. In this year's GSoC projects, `optimizer trace' interested me particularly. It would be great for MariaDB to have this functionality. This functionality is already implemented in MySQL. So my question is, can I read MySQL's source code implementing this function? Would that cause legal problems? Thanks very much! Regards, Zhang Shuai.

3 2