On Fri, May 1, 2020 at 2:15 AM Sergey Petrunia 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 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 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@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