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