Re: [Maria-developers] [Commits] 3ead2cea95c: MDEV-13266: Race condition in ANALYZE TABLE / statistics collection
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). 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.
--- 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@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
On Fri, May 1, 2020 at 2:15 AM Sergey Petrunia <sergey@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 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
On Sun, May 03, 2020 at 04:47:46PM +0530, Varun Gupta wrote:
On Fri, May 1, 2020 at 2:15 AM Sergey Petrunia <sergey@mariadb.com> wrote:
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
I mean, before the patch MariaDB> analyze table t1 persistent for columns () indexes (no_such_index); +-------+---------+----------+------------------+ | Table | Op | Msg_type | Msg_text | +-------+---------+----------+------------------+ | j1.t1 | analyze | error | Invalid argument | +-------+---------+----------+------------------+ 1 row in set (0.00 sec) After the patch: MariaDB> analyze table t1 persistent for columns () indexes (no_such_index); +-------+---------+----------+-----------------------------------------+ | Table | Op | Msg_type | Msg_text | +-------+---------+----------+-----------------------------------------+ | j1.t1 | analyze | status | Engine-independent statistics collected | | j1.t1 | analyze | error | Invalid argument | +-------+---------+----------+-----------------------------------------+ 2 rows in set (0.03 sec) But the new behavior is actually consistent with other similar errors: MariaDB> analyze table t1 persistent for columns (no_such_column) indexes (); +-------+---------+----------+-----------------------------------------+ | Table | Op | Msg_type | Msg_text | +-------+---------+----------+-----------------------------------------+ | j1.t1 | analyze | status | Engine-independent statistics collected | | j1.t1 | analyze | error | Invalid argument | +-------+---------+----------+-----------------------------------------+ Perhaps we need a separate MDEV to fix error reporting. BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://s.petrunia.net/blog
participants (2)
-
Sergey Petrunia -
Varun Gupta