[Commits] 66349551bf3: MDEV-16214: Incorrect plan taken by the optimizer , uses INDEX instead of ref access with ORDER BY
revision-id: 66349551bf3a266fbefaebfe044abd05108723c2 (mariadb-10.3.12-205-g66349551bf3) parent(s): 3d56adbfac394b2b3ffd22a89fe7c2978ed9a505 author: Varun Gupta committer: Varun Gupta timestamp: 2019-05-17 13:44:05 +0530 message: MDEV-16214: Incorrect plan taken by the optimizer , uses INDEX instead of ref access with ORDER BY The issue in this case is that we take in account the estimates from quick keys instead of rec_per_key. The estimates for quick keys are better than rec_per_key only if we have ref(const), so we need to check that all keyparts in the ref key are of the type ref(const). --- mysql-test/main/order_by.result | 57 +++++++++++++++++++++++++++++++++++++++++ mysql-test/main/order_by.test | 37 ++++++++++++++++++++++++++ sql/sql_select.cc | 42 ++++++++++++++++++++---------- 3 files changed, 122 insertions(+), 14 deletions(-) diff --git a/mysql-test/main/order_by.result b/mysql-test/main/order_by.result index db096acb162..8d1e471f618 100644 --- a/mysql-test/main/order_by.result +++ b/mysql-test/main/order_by.result @@ -3266,3 +3266,60 @@ NULLIF(GROUP_CONCAT(v1), null) C B DROP TABLE t1; +# +# MDEV-16214: Incorrect plan taken by the optimizer , uses INDEX instead of ref access with ORDER BY +# +create table t1(a int); +insert into t1 values (0),(1),(2),(3),(4),(5),(6),(7),(8),(9); +create table t2( +id int primary key, +key1 int,key2 int, +col1 int, +key(key1), key(key2) +); +insert into t2 +select +A.a + B.a*10 + C.a*100, +A.a + 10*B.a, A.a + 10*B.a, +123456 +from t1 A, t1 B, t1 C; +# here type should show ref not index +explain select +(SELECT concat(id, '-', key1, '-', col1) +FROM t2 +WHERE +t2.key1 = t1.a and t2.key1 IS NOT NULL +ORDER BY +t2.key2 ASC +LIMIT 1) +from t1; +id select_type table type possible_keys key key_len ref rows Extra +1 PRIMARY t1 ALL NULL NULL NULL NULL 10 +2 DEPENDENT SUBQUERY t2 ref key1 key1 5 test.t1.a 10 Using index condition; Using where; Using filesort +select +(SELECT concat(id, '-', key1, '-', col1) +FROM t2 +WHERE +t2.key1 = t1.a and t2.key1 IS NOT NULL +ORDER BY +t2.key2 ASC +LIMIT 1) +from t1; +(SELECT concat(id, '-', key1, '-', col1) +FROM t2 +WHERE +t2.key1 = t1.a and t2.key1 IS NOT NULL +ORDER BY +t2.key2 ASC +LIMIT 1) +900-0-123456 +901-1-123456 +902-2-123456 +903-3-123456 +904-4-123456 +905-5-123456 +906-6-123456 +907-7-123456 +908-8-123456 +909-9-123456 +drop table t1,t2; diff --git a/mysql-test/main/order_by.test b/mysql-test/main/order_by.test index d67c67de89c..58b91fbda91 100644 --- a/mysql-test/main/order_by.test +++ b/mysql-test/main/order_by.test @@ -2201,3 +2201,40 @@ GROUP BY id ORDER BY id+1 DESC; DROP TABLE t1; + +--echo # +--echo # MDEV-16214: Incorrect plan taken by the optimizer , uses INDEX instead of ref access with ORDER BY +--echo # + +create table t1(a int); +insert into t1 values (0),(1),(2),(3),(4),(5),(6),(7),(8),(9); + +create table t2( + id int primary key, + key1 int,key2 int, + col1 int, + key(key1), key(key2) +); + +insert into t2 + select + A.a + B.a*10 + C.a*100, + A.a + 10*B.a, A.a + 10*B.a, + 123456 +from t1 A, t1 B, t1 C; + +let $query= select + (SELECT concat(id, '-', key1, '-', col1) + FROM t2 + WHERE + t2.key1 = t1.a and t2.key1 IS NOT NULL + ORDER BY + t2.key2 ASC + LIMIT 1) + from t1; + +--echo # here type should show ref not index +eval explain $query; +eval $query; + +drop table t1,t2; diff --git a/sql/sql_select.cc b/sql/sql_select.cc index f36a68bc7ae..8cdcf3afc8b 100644 --- a/sql/sql_select.cc +++ b/sql/sql_select.cc @@ -9986,31 +9986,35 @@ static bool create_ref_for_key(JOIN *join, JOIN_TAB *j, j->ref.null_rejecting|= (key_part_map)1 << i; keyuse_uses_no_tables= keyuse_uses_no_tables && !keyuse->used_tables; /* - Todo: we should remove this check for thd->lex->describe on the next - line. With SHOW EXPLAIN code, EXPLAIN printout code no longer depends - on it. However, removing the check caused change in lots of query - plans! Does the optimizer depend on the contents of - table_ref->key_copy ? If yes, do we produce incorrect EXPLAINs? + We don't want to compute heavy expressions in EXPLAIN, an example would + select * from t1 where t1.key=(select thats very heavy); + + (select thats very heavy) => is a constant here + eg: (select avg(order_cost) from orders) => constant but expensive */ if (!keyuse->val->used_tables() && !thd->lex->describe) { // Compare against constant - store_key_item tmp(thd, + store_key_item tmp(thd, keyinfo->key_part[i].field, key_buff + maybe_null, maybe_null ? key_buff : 0, keyinfo->key_part[i].length, keyuse->val, FALSE); - if (unlikely(thd->is_fatal_error)) - DBUG_RETURN(TRUE); - tmp.copy(); + if (unlikely(thd->is_fatal_error)) + DBUG_RETURN(TRUE); + tmp.copy(); j->ref.const_ref_part_map |= key_part_map(1) << i ; } else - *ref_key++= get_store_key(thd, - keyuse,join->const_table_map, - &keyinfo->key_part[i], - key_buff, maybe_null); + { + *ref_key++= get_store_key(thd, + keyuse,join->const_table_map, + &keyinfo->key_part[i], + key_buff, maybe_null); + if (!keyuse->val->used_tables()) + j->ref.const_ref_part_map |= key_part_map(1) << i ; + } /* Remember if we are going to use REF_OR_NULL But only if field _really_ can be null i.e. we force JT_REF @@ -25256,6 +25260,8 @@ bool JOIN_TAB::save_explain_data(Explain_table_access *eta, { if (!(eta->ref_list.append_str(thd->mem_root, "const"))) return 1; + if (thd->lex->describe) + key_ref++; } else { @@ -26921,7 +26927,15 @@ test_if_cheaper_ordering(const JOIN_TAB *tab, ORDER *order, TABLE *table, */ if (ref_key >= 0 && ref_key != MAX_KEY && tab->type == JT_REF) { - if (table->quick_keys.is_set(ref_key)) + /* + For all the parts of the ref key we check if all of them belong + to the type ref(const). This is done because if all parts of the ref + key are of type ref(const), then we are sure that the estimates + provides by quick keys is better than that provide by rec_per_key. + */ + if (tab->ref.const_ref_part_map == make_prev_keypart_map(tab->ref.key_parts) && + table->quick_keys.is_set(ref_key) && + table->quick_key_parts[ref_key] == tab->ref.key_parts) refkey_rows_estimate= table->quick_rows[ref_key]; else {
Hi Varun, The patch is mostly fine. Please find some input on the comments / coding style below. Ok to push after it is addressed. On Fri, May 17, 2019 at 01:44:36PM +0530, Varun wrote:
revision-id: 66349551bf3a266fbefaebfe044abd05108723c2 (mariadb-10.3.12-205-g66349551bf3) parent(s): 3d56adbfac394b2b3ffd22a89fe7c2978ed9a505 author: Varun Gupta committer: Varun Gupta timestamp: 2019-05-17 13:44:05 +0530 message:
MDEV-16214: Incorrect plan taken by the optimizer , uses INDEX instead of ref access with ORDER BY
The issue in this case is that we take in account the estimates from quick keys instead of rec_per_key. The estimates for quick keys are better than rec_per_key only if we have ref(const), so we need to check that all keyparts in the ref key are of the type ref(const).
--- mysql-test/main/order_by.result | 57 +++++++++++++++++++++++++++++++++++++++++ mysql-test/main/order_by.test | 37 ++++++++++++++++++++++++++ sql/sql_select.cc | 42 ++++++++++++++++++++---------- 3 files changed, 122 insertions(+), 14 deletions(-) ... diff --git a/sql/sql_select.cc b/sql/sql_select.cc index f36a68bc7ae..8cdcf3afc8b 100644 --- a/sql/sql_select.cc +++ b/sql/sql_select.cc @@ -9986,31 +9986,35 @@ static bool create_ref_for_key(JOIN *join, JOIN_TAB *j, j->ref.null_rejecting|= (key_part_map)1 << i; keyuse_uses_no_tables= keyuse_uses_no_tables && !keyuse->used_tables; /* - Todo: we should remove this check for thd->lex->describe on the next - line. With SHOW EXPLAIN code, EXPLAIN printout code no longer depends - on it. However, removing the check caused change in lots of query - plans! Does the optimizer depend on the contents of - table_ref->key_copy ? If yes, do we produce incorrect EXPLAINs? + We don't want to compute heavy expressions in EXPLAIN, an example would + select * from t1 where t1.key=(select thats very heavy); + + (select thats very heavy) => is a constant here + eg: (select avg(order_cost) from orders) => constant but expensive */ if (!keyuse->val->used_tables() && !thd->lex->describe) { // Compare against constant - store_key_item tmp(thd, + store_key_item tmp(thd, keyinfo->key_part[i].field, key_buff + maybe_null, maybe_null ? key_buff : 0, keyinfo->key_part[i].length, keyuse->val, FALSE); - if (unlikely(thd->is_fatal_error)) - DBUG_RETURN(TRUE); - tmp.copy(); + if (unlikely(thd->is_fatal_error)) + DBUG_RETURN(TRUE); + tmp.copy(); j->ref.const_ref_part_map |= key_part_map(1) << i ; } else - *ref_key++= get_store_key(thd, - keyuse,join->const_table_map, - &keyinfo->key_part[i], - key_buff, maybe_null); + { + *ref_key++= get_store_key(thd, + keyuse,join->const_table_map, + &keyinfo->key_part[i], + key_buff, maybe_null); + if (!keyuse->val->used_tables()) + j->ref.const_ref_part_map |= key_part_map(1) << i ; + } /* Remember if we are going to use REF_OR_NULL But only if field _really_ can be null i.e. we force JT_REF @@ -25256,6 +25260,8 @@ bool JOIN_TAB::save_explain_data(Explain_table_access *eta, { if (!(eta->ref_list.append_str(thd->mem_root, "const"))) return 1; + if (thd->lex->describe) + key_ref++; This is very cryptic.
Please provide an explanation, something like: create_ref_for_key() handles keypart=const equalities as follows: - non-EXPLAIN execution will copy the "const" to lookup tuple immediately and will not add an element to ref.key_copy - EXPLAIN will put an element into ref.key_copy. Since we've just printed "const" for it, we should skip it here.
} else { @@ -26921,7 +26927,15 @@ test_if_cheaper_ordering(const JOIN_TAB *tab, ORDER *order, TABLE *table, */ if (ref_key >= 0 && ref_key != MAX_KEY && tab->type == JT_REF) { - if (table->quick_keys.is_set(ref_key)) + /* + For all the parts of the ref key we check if all of them belong + to the type ref(const). This is done because if all parts of the ref + key are of type ref(const), then we are sure that the estimates + provides by quick keys is better than that provide by rec_per_key.
The above is hard to read and has typos. Here's a shorter variant: If ref access uses keypart=const for all its key parts, and quick select uses the same # of key parts, then they are equivalent. Reuse #rows estimate from quick select as it is more precise.
+ */ + if (tab->ref.const_ref_part_map == make_prev_keypart_map(tab->ref.key_parts) && + table->quick_keys.is_set(ref_key) && + table->quick_key_parts[ref_key] == tab->ref.key_parts) Please fix indetation; One of the lines is too long.
refkey_rows_estimate= table->quick_rows[ref_key]; else {
BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://s.petrunia.net/blog
participants (2)
-
Sergey Petrunia -
Varun