On Wed, Sep 07, 2016 at 10:36:32AM +0300, Jan Lindström wrote: > On Tue, Sep 6, 2016 at 8:37 PM, Sergei Petrunia <[email protected]> > wrote: > > > revision-id: f2aea435df7e92fcf8f09f8f6c160161168c5bed > > parent(s): a14f61ef749ad9f9ab2b0f5badf6754ba7443c9e > > committer: Sergei Petrunia > > branch nick: 10.0 > > timestamp: 2016-09-06 20:37:21 +0300 > > message: > > > > MDEV-10649: Optimizer sometimes use "index" instead of "range" access for > > UPDATE > > > > (XtraDB variant only, for now) > > > > Re-opening a TABLE object (after e.g. FLUSH TABLES or open table cache > > eviction) causes ha_innobase to call > > dict_stats_update(DICT_STATS_FETCH_ONLY_IF_NOT_IN_MEMORY). > > > > Inside this call, the following is done: > > dict_stats_empty_table(table); > > dict_stats_copy(table, t); > > > > On the other hand, commands like UPDATE make this call to get the "rows in > > table" statistics in table->stats.records: > > > > ha_innobase->info(HA_STATUS_VARIABLE|HA_STATUS_NO_LOCK) > > > > note the HA_STATUS_NO_LOCK parameter. It means, no locks are taken by > > ::info() If the ::info() call happens between dict_stats_empty_table > > and dict_stats_copy calls, the UPDATE's optimizer will get an estimate > > of table->stats.records=1, which causes it to pick a full table scan, > > which in turn will take a lot of row locks and cause other bad > > consequences. > > > > --- > > storage/xtradb/dict/dict0stats.cc | 29 +++++++++++++++++++---------- > > 1 file changed, 19 insertions(+), 10 deletions(-) > > > > diff --git a/storage/xtradb/dict/dict0stats.cc b/storage/xtradb/dict/ > > dict0stats.cc > > index b073398..a4aa436 100644 > > --- a/storage/xtradb/dict/dict0stats.cc > > +++ b/storage/xtradb/dict/dict0stats.cc > > @@ -673,7 +673,10 @@ Write all zeros (or 1 where it makes sense) into a > > table and its indexes' > > dict_stats_copy( > > /*============*/ > > dict_table_t* dst, /*!< in/out: destination table */ > > - const dict_table_t* src) /*!< in: source table */ > > + const dict_table_t* src, /*!< in: source table */ > > + bool reset_ignored_indexes) /*!< in: if true, set ignored > > indexes > > + to have the same statistics > > as if > > + the table was empty */ > > { > > dst->stats_last_recalc = src->stats_last_recalc; > > dst->stat_n_rows = src->stat_n_rows; > > @@ -692,7 +695,16 @@ Write all zeros (or 1 where it makes sense) into a > > table and its indexes' > > && (src_idx = dict_table_get_next_index(src_idx)))) { > > > > if (dict_stats_should_ignore_index(dst_idx)) { > > - continue; > > + if (reset_ignored_indexes) { > > + /* Reset index statistics for all ignored > > indexes, > > + unless they are FT indexes (these have no > > statistics)*/ > > + if (dst_idx->type & DICT_FTS) { > > + continue; > > + } > > + dict_stats_empty_index(dst_idx); > > > > Does this really help? Yes, we hold dict_sys mutex here so > dict_stats_empty_index here is safe for all readers using same mutex. > However, as you pointed up above, info() uses no locking method. > Thus, we really do not know what it will return, all values before > dict_stats_copy(), some of the indexes with new stats, all indexes with new > stats.
I think there two issues here: issue#1 is the above chunk of code. It is executed only for indexes that have dict_stats_should_ignore_index()= true. These are fulltext indexes, indexes that are corrupted or are being dropped. As far I understand, the optimizer will not (or should not) attempt to use index statistics on such indexes, anyway. issue#2 is with updating individual index statistics members. It does exist, there are race conditions also near ANALYZE code: https://jira.mariadb.org/browse/MDEV-10790 I would like to limit the scope of this fix to address the race condition with reading/updating dict_table_t::stat_n_rows. (I believe this is done here, to a full extent). As for race conditions with dict_index_t members, I want to address them in MDEV-10790. > > > > > @@ -3240,13 +3252,10 @@ N*AVG(Ui). In each call it searches for the > > currently fetched index into > > > > dict_table_stats_lock(table, RW_X_LATCH); > > > > - /* Initialize all stats to dummy values before > > - copying because dict_stats_table_clone_create() > > does > > - skip corrupted indexes so our dummy object 't' may > > - have less indexes than the real object 'table'. */ > > - dict_stats_empty_table(table); > > - > > - dict_stats_copy(table, t); > > + /* Pass reset_ignored_indexes=true as parameter > > + to dict_stats_copy. This will cause statictics > > + for corrupted indexes to be set to empty values */ > > + dict_stats_copy(table, t, true); > > > > This is better solution than the original but as noted above, is this > enough ? > > R: Jan -- BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://s.petrunia.net/blog _______________________________________________ Mailing list: https://launchpad.net/~maria-developers Post to : [email protected] Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
