maria-developers team mailing list archive

[Varun Gupta <varungupta1803@xxxxxxxxx>]

On Fri, May 1, 2020 at 2:15 AM Sergey Petrunia <sergey@xxxxxxxxxxx> wrote:

> Hi Varun,
>
>
> On Sun, Apr 12, 2020 at 09:08:37PM +0530, Varun wrote:
> > revision-id: 3ead2cea95c32b7ceaf6e6ec81f7afbd9137cfe9
> (mariadb-10.2.31-103-g3ead2cea95c)
> > parent(s): d565895bbd8e2a163be48b9bac51fccbf3949c80
> > author: Varun Gupta
> > committer: Varun Gupta
> > timestamp: 2020-04-12 21:05:36 +0530
> > message:
> >
> > MDEV-13266: Race condition in ANALYZE TABLE / statistics collection
> >
> > Fixing a race condition while collecting the engine independent
> statistics.
> > The issue here was when the statistics was collected on specific indexes
> > then because of some race condition the statistics for indexes was not
> > collected. The TABLE::keys_in_use_for_query was set to 0 in such cases.
> >
> > This happens when the table is opened from TABLE_SHARE instead of the
> > table share stored in table_cache.
>
> It would be nice to write down the race condition that we've caught (in
> case we
> need this info later)
>
> Is my understanding correct: the error scenario is as follows:
>
> Thread1> start running "ANALYZE TABLE t PERISTENT FOR COLUMNS (..) INDEXES
> ($list)"
> Thread1> Walk through $list and save it in TABLE::keys_in_use_for_query
>
> Thread1> Close/re-open tables
> Thread2> Make some use of table t. This involves taking table t from
> Thread2> the table cache, and putting it back (with
>          TABLE::keys_in_use_for_query reset to 0)
>
> Thread1> continue collecting EITS stats. Since
> TABLE::keys_in_use_for_query we
> will not collect statistics for indexes in $list.
>
> Please confirm (and if not, describe the race condition).
>
> Yes this is correct, I will add the above description to the commit message


> The patch also introduces this behaviour:
>
> analyze table ... persistent for ... indexes(no_such_index);
>
> will now cause engine statistics to be still collected. Before the patch
> it exited with an error.
>
> But then, this is consistent with what happens for
>
> analyze table ... persistent for columns(no_such_column) ...
>
> So I guess this is ok.
>

I am not able to understand what you mean here, for no_such_index I think
you mean an empty list and for such case I think collecting statistics fro
all the indexes is fine.
If this means something else, then lets discuss it


Regards,
Varun


> > ---
> >  sql/sql_admin.cc | 46 +++++++++++++++++++++-------------------------
> >  1 file changed, 21 insertions(+), 25 deletions(-)
> >
> > diff --git a/sql/sql_admin.cc b/sql/sql_admin.cc
> > index ab95fdc340c..0613495f202 100644
> > --- a/sql/sql_admin.cc
> > +++ b/sql/sql_admin.cc
> > @@ -769,31 +769,6 @@ static bool mysql_admin_table(THD* thd, TABLE_LIST*
> tables,
> >          (table->table->s->table_category == TABLE_CATEGORY_USER &&
> >           (get_use_stat_tables_mode(thd) > NEVER ||
> >            lex->with_persistent_for_clause));
> > -
> > -
> > -      if (!lex->index_list)
> > -      {
> > -        tab->keys_in_use_for_query.init(tab->s->keys);
> > -      }
> > -      else
> > -      {
> > -        int pos;
> > -        LEX_STRING *index_name;
> > -        List_iterator_fast<LEX_STRING> it(*lex->index_list);
> > -
> > -        tab->keys_in_use_for_query.clear_all();
> > -        while ((index_name= it++))
> > -     {
> > -          if (tab->s->keynames.type_names == 0 ||
> > -              (pos= find_type(&tab->s->keynames, index_name->str,
> > -                              index_name->length, 1)) <= 0)
> > -          {
> > -            compl_result_code= result_code= HA_ADMIN_INVALID;
> > -            break;
> > -          }
> > -          tab->keys_in_use_for_query.set_bit(--pos);
> > -        }
> > -      }
> >      }
> >
> >      if (result_code == HA_ADMIN_OK)
> > @@ -878,6 +853,27 @@ static bool mysql_admin_table(THD* thd, TABLE_LIST*
> tables,
> >            }
> >            tab->file->column_bitmaps_signal();
> >          }
> > +        if (!lex->index_list)
> > +          tab->keys_in_use_for_query.init(tab->s->keys);
> > +        else
> > +        {
> > +          int pos;
> > +          LEX_STRING *index_name;
> > +          List_iterator_fast<LEX_STRING> it(*lex->index_list);
> > +
> > +          tab->keys_in_use_for_query.clear_all();
> > +          while ((index_name= it++))
> > +          {
> > +            if (tab->s->keynames.type_names == 0 ||
> > +                (pos= find_type(&tab->s->keynames, index_name->str,
> > +                                index_name->length, 1)) <= 0)
> > +            {
> > +              compl_result_code= result_code= HA_ADMIN_INVALID;
> > +              break;
> > +            }
> > +            tab->keys_in_use_for_query.set_bit(--pos);
> > +          }
> > +        }
> >          if (!(compl_result_code=
> >                alloc_statistics_for_table(thd, table->table)) &&
> >              !(compl_result_code=
> > _______________________________________________
> > commits mailing list
> > commits@xxxxxxxxxxx
> > https://lists.askmonty.org/cgi-bin/mailman/listinfo/commits
>
> BR
>  Sergei
> --
> Sergei Petrunia, Software Developer
> MariaDB Corporation | Skype: sergefp | Blog: http://s.petrunia.net/blog
>
>
>