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