Re: [Maria-developers] faf8de3aa98: Fixed some assert crashes related to keyread.
Hi, Michael! On Apr 13, Michael Widenius wrote:
revision-id: faf8de3aa98 (mariadb-10.5.2-121-gfaf8de3aa98) parent(s): 3cbe15bd78c author: Michael Widenius <monty@mariadb.com> committer: Michael Widenius <monty@mariadb.com> timestamp: 2020-04-09 01:37:02 +0300 message:
Fixed some assert crashes related to keyread.
- MDEV-22062 Assertion `!table->file->keyread_enabled()' failed in close_thread_table() - MDEV-22077 table->no_keyread .. failed in join_read_first()
diff --git a/mysql-test/main/keyread.test b/mysql-test/main/keyread.test index d9d3002d392..76d0f5674dd 100644 --- a/mysql-test/main/keyread.test +++ b/mysql-test/main/keyread.test @@ -8,3 +8,14 @@ create view v1 as select * from t1 where f2 = 1; select distinct f1 from v1; drop view v1; drop table t1; + +# +# MDEV-22062 Assertion `!table->file->keyread_enabled()' failed in +# close_thread_table +# + +CREATE TABLE t1 (a INT NOT NULL, UNIQUE(a)) ENGINE=InnoDB; +INSERT INTO t1 VALUES (1),(2); +DELETE FROM t1 ORDER BY a LIMIT 1; +SELECT * FROM t1; +DROP TABLE t1;
where's a test case for MDEV-22077?
diff --git a/sql/sql_delete.cc b/sql/sql_delete.cc index 19eabbb053c..2fc0de4345f 100644 --- a/sql/sql_delete.cc +++ b/sql/sql_delete.cc @@ -547,10 +547,12 @@ bool mysql_delete(THD *thd, TABLE_LIST *table_list, COND *conds, else { ha_rows scanned_limit= query_plan.scanned_rows; + table->no_keyread= 1; query_plan.index= get_index_for_order(order, table, select, limit, &scanned_limit, &query_plan.using_filesort, &reverse); + table->no_keyread= 0;
why?
if (!query_plan.using_filesort) query_plan.scanned_rows= scanned_limit; } diff --git a/sql/sql_select.cc b/sql/sql_select.cc index 129dae9eedb..1f05156b96f 100644 --- a/sql/sql_select.cc +++ b/sql/sql_select.cc @@ -23584,7 +23584,7 @@ test_if_skip_sort_order(JOIN_TAB *tab,ORDER *order,ha_rows select_limit, If ref_key used index tree reading only ('Using index' in EXPLAIN), and best_key doesn't, then revert the decision. */ - if (table->covering_keys.is_set(best_key)) + if (table->covering_keys.is_set(best_key) && !table->no_keyread) table->file->ha_start_keyread(best_key); else table->file->ha_end_keyread(); @@ -28568,8 +28568,6 @@ test_if_cheaper_ordering(const JOIN_TAB *tab, ORDER *order, TABLE *table, if (new_used_key_parts != NULL) *new_used_key_parts= best_key_parts; table->file->ha_end_keyread(); - if (is_best_covering && !table->no_keyread) - table->file->ha_start_keyread(best_key);
I find it very confusing that test_if_cheaper_ordering has a side effect of ending keyread. It was ok, when it ended previous keyread and started a new one. But just disabling - it's looks very strange.
DBUG_RETURN(TRUE); }
Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
Hi! On Thu, Apr 16, 2020 at 1:17 PM Sergei Golubchik <serg@mariadb.org> wrote: <cut>
where's a test case for MDEV-22077?
One can't use the test case in MDEV-22077 as it involves a table created by a 32 bit MariaDB version. Doing another test case it's very hard so I ignored it and just did run the test to verify the patch.
diff --git a/sql/sql_delete.cc b/sql/sql_delete.cc index 19eabbb053c..2fc0de4345f 100644 --- a/sql/sql_delete.cc +++ b/sql/sql_delete.cc @@ -547,10 +547,12 @@ bool mysql_delete(THD *thd, TABLE_LIST *table_list, COND *conds, else { ha_rows scanned_limit= query_plan.scanned_rows; + table->no_keyread= 1; query_plan.index= get_index_for_order(order, table, select, limit, &scanned_limit, &query_plan.using_filesort, &reverse); + table->no_keyread= 0;
why?
get_index_for_order is not allowed to change to use key_reads as there is no guarantee at this point that the suggested index will be used. That was the reason we got crashes.
@@ -28568,8 +28568,6 @@ test_if_cheaper_ordering(const JOIN_TAB *tab, ORDER *order, TABLE *table, if (new_used_key_parts != NULL) *new_used_key_parts= best_key_parts; table->file->ha_end_keyread(); - if (is_best_covering && !table->no_keyread) - table->file->ha_start_keyread(best_key);
I find it very confusing that test_if_cheaper_ordering has a side effect of ending keyread. It was ok, when it ended previous keyread and started a new one. But just disabling - it's looks very strange.
It has to be disabled as we may in the future use another index than the one we originally planned to use (with keyread). This is something that should be re-factored in the future so that we only enable key-read's when we have done the final decision of which index we will really use. Regards, Monty
Hi, Michael! On Apr 16, Michael Widenius wrote:
Hi!
On Thu, Apr 16, 2020 at 1:17 PM Sergei Golubchik <serg@mariadb.org> wrote:
<cut>
where's a test case for MDEV-22077?
One can't use the test case in MDEV-22077 as it involves a table created by a 32 bit MariaDB version. Doing another test case it's very hard so I ignored it and just did run the test to verify the patch.
Alice posted a simple test case in the comments. It has a very similar stack trace. Could you check it out? Is it a same bug or a different one?
diff --git a/sql/sql_delete.cc b/sql/sql_delete.cc index 19eabbb053c..2fc0de4345f 100644 --- a/sql/sql_delete.cc +++ b/sql/sql_delete.cc @@ -547,10 +547,12 @@ bool mysql_delete(THD *thd, TABLE_LIST *table_list, COND *conds, else { ha_rows scanned_limit= query_plan.scanned_rows; + table->no_keyread= 1; query_plan.index= get_index_for_order(order, table, select, limit, &scanned_limit, &query_plan.using_filesort, &reverse); + table->no_keyread= 0;
why?
get_index_for_order is not allowed to change to use key_reads as there is no guarantee at this point that the suggested index will be used. That was the reason we got crashes.
Why does it enable keyread then? Where?
@@ -28568,8 +28568,6 @@ test_if_cheaper_ordering(const JOIN_TAB *tab, ORDER *order, TABLE *table, if (new_used_key_parts != NULL) *new_used_key_parts= best_key_parts; table->file->ha_end_keyread(); - if (is_best_covering && !table->no_keyread) - table->file->ha_start_keyread(best_key);
I find it very confusing that test_if_cheaper_ordering has a side effect of ending keyread. It was ok, when it ended previous keyread and started a new one. But just disabling - it's looks very strange.
It has to be disabled as we may in the future use another index than the one we originally planned to use (with keyread). This is something that should be re-factored in the future so that we only enable key-read's when we have done the final decision of which index we will really use.
Yes, but it can be disabled in the caller. Or not even enabled that early. Either approach is better than disabling keyread as a side effect of test_if_cheaper_ordering. Where is it enabled now? Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
Hi! On Fri, 17 Apr 2020, 15:21 Sergei Golubchik, <serg@mariadb.org> wrote:
Hi, Michael!
On Apr 16, Michael Widenius wrote:
Hi!
On Thu, Apr 16, 2020 at 1:17 PM Sergei Golubchik <serg@mariadb.org> wrote:
<cut>
where's a test case for MDEV-22077?
One can't use the test case in MDEV-22077 as it involves a table created by a 32 bit MariaDB version. Doing another test case it's very hard so I ignored it and just did run the test to verify the patch.
Alice posted a simple test case in the comments. It has a very similar stack trace. Could you check it out? Is it a same bug or a different one?
diff --git a/sql/sql_delete.cc b/sql/sql_delete.cc index 19eabbb053c..2fc0de4345f 100644 --- a/sql/sql_delete.cc +++ b/sql/sql_delete.cc @@ -547,10 +547,12 @@ bool mysql_delete(THD *thd, TABLE_LIST *table_list, COND *conds, else { ha_rows scanned_limit= query_plan.scanned_rows; + table->no_keyread= 1; query_plan.index= get_index_for_order(order, table, select, limit, &scanned_limit,
&query_plan.using_filesort,
&reverse); + table->no_keyread= 0;
why?
get_index_for_order is not allowed to change to use key_reads as there is no guarantee at this point that the suggested index will be used. That was the reason we got crashes.
Why does it enable keyread then? Where?
Don't know exactly and don't care for the moment. The problem is that we try to do a last desperate attempt to use a better index after all optimization decissions are already done and this is an effect of this. Changing this is too time consuming at this point (been in discussions for years and not done already as it's not trivial) so I don't have time to do it now. Regards, Monty
participants (2)
-
Michael Widenius -
Sergei Golubchik