On Fri, May 1, 2020 at 2:15 AM Sergey Petrunia <ser...@mariadb.com> 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
> > comm...@mariadb.org
> > 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
>
>
>
_______________________________________________
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help : https://help.launchpad.net/ListHelp
