revision-id: d8b28f78535064c6505a8e355a04d2cef90f81f0 (mariadb-10.4.7-7-gd8b28f78535) parent(s): 13f36fffeaecf316435fc497b0f3ae2a5d58d749 author: Sergei Petrunia committer: Sergei Petrunia timestamp: 2019-09-11 20:22:08 +0300 message: MDEV-20371: Invalid reads at plan refinement stage: join->positions... (re-committing in 10.4) best_access_path() is called from two optimization phases: 1. Plan choice phase, in choose_plan(). Here, the join prefix being considered is in join->positions[] 2. Plan refinement stage, in fix_semijoin_strategies_for_picked_join_order Here, the join prefix is in join->best_positions[] It used to access join->positions[] from stage #2. This didnt cause any valgrind or asan failures (as join->positions[] has been written-to before) but the effect was similar to that of reading the random data: The join prefix we've picked (in join->best_positions) could have nothing in common with the join prefix that was last to be considered (in join->positions). --- sql/opt_subselect.cc | 27 +++++++++++++++++---------- sql/opt_subselect.h | 8 ++++++-- sql/sql_select.cc | 37 ++++++++++++++++++++++--------------- sql/sql_select.h | 9 ++++++++- 4 files changed, 53 insertions(+), 28 deletions(-) diff --git a/sql/opt_subselect.cc b/sql/opt_subselect.cc index 599642b3a26..d9172d7f13f 100644 --- a/sql/opt_subselect.cc +++ b/sql/opt_subselect.cc @@ -453,10 +453,6 @@ bool find_eq_ref_candidate(TABLE *table, table_map sj_inner_tables); static SJ_MATERIALIZATION_INFO * at_sjmat_pos(const JOIN *join, table_map remaining_tables, const JOIN_TAB *tab, uint idx, bool *loose_scan); -void best_access_path(JOIN *join, JOIN_TAB *s, - table_map remaining_tables, uint idx, - bool disable_jbuf, double record_count, - POSITION *pos, POSITION *loose_scan_pos); void trace_plan_prefix(JOIN *join, uint idx, table_map remaining_tables); static Item *create_subq_in_equalities(THD *thd, SJ_MATERIALIZATION_INFO *sjm, @@ -2787,6 +2783,13 @@ void advance_sj_state(JOIN *join, table_map remaining_tables, uint idx, NULL, }; +#ifdef HAVE_valgrind + new (&pos->firstmatch_picker) Firstmatch_picker; + new (&pos->loosescan_picker) LooseScan_picker; + new (&pos->sjmat_picker) Sj_materialization_picker; + new (&pos->dups_weedout_picker) Duplicate_weedout_picker; +#endif + if (join->emb_sjm_nest) { /* @@ -3078,7 +3081,8 @@ bool Sj_materialization_picker::check_qep(JOIN *join, Json_writer_temp_disable trace_semijoin_mat_scan(thd); for (i= first_tab + mat_info->tables; i <= idx; i++) { - best_access_path(join, join->positions[i].table, rem_tables, i, + best_access_path(join, join->positions[i].table, rem_tables, + join->positions, i, disable_jbuf, prefix_rec_count, &curpos, &dummy); prefix_rec_count= COST_MULT(prefix_rec_count, curpos.records_read); prefix_cost= COST_ADD(prefix_cost, curpos.read_time); @@ -3718,7 +3722,8 @@ void fix_semijoin_strategies_for_picked_join_order(JOIN *join) Json_writer_object trace_one_table(thd); trace_one_table.add_table_name(join->best_positions[i].table); } - best_access_path(join, join->best_positions[i].table, rem_tables, i, + best_access_path(join, join->best_positions[i].table, rem_tables, + join->best_positions, i, FALSE, prefix_rec_count, join->best_positions + i, &dummy); prefix_rec_count *= join->best_positions[i].records_read; @@ -3758,8 +3763,9 @@ void fix_semijoin_strategies_for_picked_join_order(JOIN *join) } if (join->best_positions[idx].use_join_buffer) { - best_access_path(join, join->best_positions[idx].table, - rem_tables, idx, TRUE /* no jbuf */, + best_access_path(join, join->best_positions[idx].table, + rem_tables, join->best_positions, idx, + TRUE /* no jbuf */, record_count, join->best_positions + idx, &dummy); } record_count *= join->best_positions[idx].records_read; @@ -3785,7 +3791,7 @@ void fix_semijoin_strategies_for_picked_join_order(JOIN *join) */ join->cur_sj_inner_tables= 0; Json_writer_object semijoin_strategy(thd); - semijoin_strategy.add("semi_join_strategy","sj_materialize"); + semijoin_strategy.add("semi_join_strategy","loose-scan"); Json_writer_array semijoin_plan(thd, "join_order"); for (idx= first; idx <= tablenr; idx++) { @@ -3797,7 +3803,8 @@ void fix_semijoin_strategies_for_picked_join_order(JOIN *join) if (join->best_positions[idx].use_join_buffer || (idx == first)) { best_access_path(join, join->best_positions[idx].table, - rem_tables, idx, TRUE /* no jbuf */, + rem_tables, join->best_positions, idx, + TRUE /* no jbuf */, record_count, join->best_positions + idx, &loose_scan_pos); if (idx==first) diff --git a/sql/opt_subselect.h b/sql/opt_subselect.h index 65131f6bc89..0799402f146 100644 --- a/sql/opt_subselect.h +++ b/sql/opt_subselect.h @@ -91,6 +91,7 @@ class Loose_scan_opt KEYUSE *best_loose_scan_start_key; uint best_max_loose_keypart; + table_map best_ref_depend_map; public: Loose_scan_opt(): @@ -253,13 +254,14 @@ class Loose_scan_opt best_loose_scan_records= records; best_max_loose_keypart= max_loose_keypart; best_loose_scan_start_key= start_key; + best_ref_depend_map= 0; } } } } void check_ref_access_part2(uint key, KEYUSE *start_key, double records, - double read_time) + double read_time, table_map ref_depend_map_arg) { if (part1_conds_met && read_time < best_loose_scan_cost) { @@ -269,6 +271,7 @@ class Loose_scan_opt best_loose_scan_records= records; best_max_loose_keypart= max_loose_keypart; best_loose_scan_start_key= start_key; + best_ref_depend_map= ref_depend_map_arg; } } @@ -284,6 +287,7 @@ class Loose_scan_opt best_loose_scan_records= rows2double(quick->records); best_max_loose_keypart= quick_max_loose_keypart; best_loose_scan_start_key= NULL; + best_ref_depend_map= 0; } } @@ -300,7 +304,7 @@ class Loose_scan_opt pos->use_join_buffer= FALSE; pos->table= tab; pos->range_rowid_filter_info= tab->range_rowid_filter_info; - // todo need ref_depend_map ? + pos->ref_depend_map= best_ref_depend_map; DBUG_PRINT("info", ("Produced a LooseScan plan, key %s, %s", tab->table->key_info[best_loose_scan_key].name.str, best_loose_scan_start_key? "(ref access)": diff --git a/sql/sql_select.cc b/sql/sql_select.cc index ff07a7aea89..1b09449779e 100644 --- a/sql/sql_select.cc +++ b/sql/sql_select.cc @@ -107,10 +107,6 @@ static bool create_ref_for_key(JOIN *join, JOIN_TAB *j, KEYUSE *org_keyuse, static ha_rows get_quick_record_count(THD *thd, SQL_SELECT *select, TABLE *table, const key_map *keys,ha_rows limit); -void best_access_path(JOIN *join, JOIN_TAB *s, - table_map remaining_tables, uint idx, - bool disable_jbuf, double record_count, - POSITION *pos, POSITION *loose_scan_pos); static void optimize_straight_join(JOIN *join, table_map join_tables); static bool greedy_search(JOIN *join, table_map remaining_tables, uint depth, uint prune_level, @@ -5481,6 +5477,13 @@ make_join_statistics(JOIN *join, List<TABLE_LIST> &tables_list, { if (choose_plan(join, all_table_map & ~join->const_table_map)) goto error; + +#ifdef HAVE_valgrind + // JOIN::positions holds the current query plan. We've already + // made the plan choice, so we should only use JOIN::best_positions + for (uint k=join->const_tables; k < join->table_count; k++) + MEM_UNDEFINED(&join->positions[k], sizeof(join->positions[k])); +#endif } else { @@ -7180,6 +7183,7 @@ void best_access_path(JOIN *join, JOIN_TAB *s, table_map remaining_tables, + const POSITION *join_positions, uint idx, bool disable_jbuf, double record_count, @@ -7299,7 +7303,7 @@ best_access_path(JOIN *join, if (!keyuse->val->maybe_null || keyuse->null_rejecting) notnull_part|=keyuse->keypart_map; - double tmp2= prev_record_reads(join->positions, idx, + double tmp2= prev_record_reads(join_positions, idx, (found_ref | keyuse->used_tables)); if (tmp2 < best_prev_record_reads) { @@ -7341,7 +7345,7 @@ best_access_path(JOIN *join, Really, there should be records=0.0 (yes!) but 1.0 would be probably safer */ - tmp= prev_record_reads(join->positions, idx, found_ref); + tmp= prev_record_reads(join_positions, idx, found_ref); records= 1.0; trace_access_idx.add("access_type", "fulltext") .add("index", keyinfo->name); @@ -7368,7 +7372,7 @@ best_access_path(JOIN *join, { trace_access_idx.add("access_type", "eq_ref") .add("index", keyinfo->name); - tmp = prev_record_reads(join->positions, idx, found_ref); + tmp = prev_record_reads(join_positions, idx, found_ref); records=1.0; } else @@ -7655,7 +7659,8 @@ best_access_path(JOIN *join, } tmp= COST_ADD(tmp, s->startup_cost); - loose_scan_opt.check_ref_access_part2(key, start_key, records, tmp); + loose_scan_opt.check_ref_access_part2(key, start_key, records, tmp, + found_ref); } /* not ft_key */ if (records < DBL_MAX) @@ -8432,7 +8437,8 @@ optimize_straight_join(JOIN *join, table_map join_tables) trace_one_table.add_table_name(s); } /* Find the best access method from 's' to the current partial plan */ - best_access_path(join, s, join_tables, idx, disable_jbuf, record_count, + best_access_path(join, s, join_tables, join->positions, idx, + disable_jbuf, record_count, position, &loose_scan_pos); /* compute the cost of the new plan extended with 's' */ @@ -9369,8 +9375,8 @@ best_extension_by_limited_search(JOIN *join, /* Find the best access method from 's' to the current partial plan */ POSITION loose_scan_pos; - best_access_path(join, s, remaining_tables, idx, disable_jbuf, - record_count, position, &loose_scan_pos); + best_access_path(join, s, remaining_tables, join->positions, idx, + disable_jbuf, record_count, position, &loose_scan_pos); /* Compute the cost of extending the plan with 's' */ current_record_count= COST_MULT(record_count, position->records_read); @@ -9763,11 +9769,11 @@ cache_record_length(JOIN *join,uint idx) */ double -prev_record_reads(POSITION *positions, uint idx, table_map found_ref) +prev_record_reads(const POSITION *positions, uint idx, table_map found_ref) { double found=1.0; - POSITION *pos_end= positions - 1; - for (POSITION *pos= positions + idx - 1; pos != pos_end; pos--) + const POSITION *pos_end= positions - 1; + for (const POSITION *pos= positions + idx - 1; pos != pos_end; pos--) { if (pos->table->table->map & found_ref) { @@ -16654,7 +16660,8 @@ void optimize_wo_join_buffering(JOIN *join, uint first_tab, uint last_tab, if ((i == first_tab && first_alt) || join->positions[i].use_join_buffer) { /* Find the best access method that would not use join buffering */ - best_access_path(join, rs, reopt_remaining_tables, i, + best_access_path(join, rs, reopt_remaining_tables, + join->positions, i, TRUE, rec_count, &pos, &loose_scan_pos); } diff --git a/sql/sql_select.h b/sql/sql_select.h index b7f870bf797..416c2d35c07 100644 --- a/sql/sql_select.h +++ b/sql/sql_select.h @@ -854,6 +854,7 @@ class LooseScan_picker : public Semi_join_strategy_picker friend void best_access_path(JOIN *join, JOIN_TAB *s, table_map remaining_tables, + const struct st_position *join_positions, uint idx, bool disable_jbuf, double record_count, @@ -2071,6 +2072,12 @@ class store_key_const_item :public store_key_item } }; +void best_access_path(JOIN *join, JOIN_TAB *s, + table_map remaining_tables, + const POSITION *join_positions, uint idx, + bool disable_jbuf, double record_count, + POSITION *pos, POSITION *loose_scan_pos); + bool cp_buffer_from_ref(THD *thd, TABLE *table, TABLE_REF *ref); bool error_if_full_join(JOIN *join); int report_error(TABLE *table, int error); @@ -2435,7 +2442,7 @@ bool instantiate_tmp_table(TABLE *table, KEY *keyinfo, ulonglong options); bool open_tmp_table(TABLE *table); void setup_tmp_table_column_bitmaps(TABLE *table, uchar *bitmaps); -double prev_record_reads(POSITION *positions, uint idx, table_map found_ref); +double prev_record_reads(const POSITION *positions, uint idx, table_map found_ref); void fix_list_after_tbl_changes(SELECT_LEX *new_parent, List<TABLE_LIST> *tlist); double get_tmp_table_lookup_cost(THD *thd, double row_count, uint row_size); double get_tmp_table_write_cost(THD *thd, double row_count, uint row_size);