maria-developers team mailing list archive

[Jan Lindström <jan.lindstrom@xxxxxxxxxxx>]

On Tue, Sep 6, 2016 at 8:37 PM, Sergei Petrunia <psergey@xxxxxxxxxxxx>
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.


>
> @@ -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