Re: [Maria-developers] 4670c85a48c: MDEV-16222 Assertion `0' failed in row_purge_remove_sec_if_poss_leaf on table with virtual columns and indexes
Hi, Aleksey! Summary: looks ok, but please move clear_error() to the caller (ha_innodb.cc) and remove the assert. On Jul 04, Aleksey Midenkov wrote:
revision-id: 4670c85a48c (mariadb-10.4.5-47-g4670c85a48c) parent(s): 0f55a9eb73b author: Aleksey Midenkov <midenok@gmail.com> committer: Aleksey Midenkov <midenok@gmail.com> timestamp: 2019-06-27 18:05:25 +0300 message:
MDEV-16222 Assertion `0' failed in row_purge_remove_sec_if_poss_leaf on table with virtual columns and indexes
Cause Stale thd->m_stmt_da->m_sql_errno which is from different invocation.
Fix Reset error state before attempt to open table.
diff --git a/mysql-test/suite/innodb/r/purge_secondary_mdev-16222.result b/mysql-test/suite/innodb/r/purge_secondary_mdev-16222.result new file mode 100644 index 00000000000..30e8f9800fb --- /dev/null +++ b/mysql-test/suite/innodb/r/purge_secondary_mdev-16222.result @@ -0,0 +1,34 @@ ... +select * from t1 into outfile 'load.data'; +Warnings: +Warning 1287 '<select expression> INTO <destination>;' is deprecated and will be removed in a future release. Please use 'SELECT <select list> INTO <destination> FROM...' instead
See the warning, better not to use the deprecated syntax, unless you specifically want to test it,
+load data infile 'load.data' replace into table t1; +set debug_sync= "now WAIT_FOR latch_released"; +set global debug_dbug= "-d,ib_purge_virtual_mdev_16222_1"; diff --git a/sql/sql_class.cc b/sql/sql_class.cc index d65b6b8ed8d..cb73b7db2de 100644 --- a/sql/sql_class.cc +++ b/sql/sql_class.cc @@ -4762,7 +4762,10 @@ TABLE *open_purge_table(THD *thd, const char *db, size_t dblen, DBUG_ASSERT(!error || !ot_ctx.can_recover_from_failed_open());
if (unlikely(error)) + { close_thread_tables(thd); + thd->clear_error();
We use thd->clear_error() in many places, but over years there were quite a few problems with it, and now I'd prefer to avoid it whenever possible. The thing is, you want to remove a specific error on a specific code path, but thd->clear_error() removes all errors, and no matter how you got here. Generally intercepting errors with Internal_error_handler is a much safer approach. In this case, though, thd->clear_error() might be okay, but better move it up the stack, do it in the caller when there can be only one way to reach this code. Not in a kind-of-generically-looking open table helper.
+ }
DBUG_RETURN(error ? NULL : tl->table); } diff --git a/sql/table.cc b/sql/table.cc index 9ac167b6adb..93c2c5e88a8 100644 --- a/sql/table.cc +++ b/sql/table.cc @@ -8240,6 +8240,7 @@ int TABLE::update_virtual_fields(handler *h, enum_vcol_update_mode update_mode)
int TABLE::update_virtual_field(Field *vf) { + DBUG_ASSERT(!in_use->is_error());
Yes, that's what I mean above. There were cases when update_virtual_field() was called when is_error() was true and is_error() introduced a bug. I don't quite remember details, but it was something like, after the error has happened somewhere, on the way to returning an error, it was calling update_virtual_field() and some code in a function down the stack was, like some_function_which_returns_void(....); if (thd->is_error()) // the error inside a function { ... handle it ... } and the error in this case was not caused by anything in the function, but existed from before. The point is, is_error() and clear_error() are global, and you're generally interested in local state. Whether _this block_ caused an error. Clear the error caused by _this function call_. And so on, not clear an error that might've happened some time somewhere at the undefined point in the past before this code line was reached.
Query_arena backup_arena; DBUG_ENTER("TABLE::update_virtual_field"); in_use->set_n_backup_active_arena(expr_arena, &backup_arena);
Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
Hello Sergei! Actually I was aware of that, but open_purge_table() looks so non-generic, so I decided to push that down. Well, I agree, it's "kind of" generic and it will be safer to move a bit higher. But why to remove DEBUG_ASSERT? update_virtual_field() relies on is_error() from inside. If error comes from outside it will return TRUE illegally. Like it happened with current bug. If there were assert in the first place it didn't took so long to debug. On Thu, Jul 4, 2019 at 2:18 PM Sergei Golubchik <serg@mariadb.org> wrote:
Hi, Aleksey!
Summary: looks ok, but please move clear_error() to the caller (ha_innodb.cc) and remove the assert.
On Jul 04, Aleksey Midenkov wrote:
revision-id: 4670c85a48c (mariadb-10.4.5-47-g4670c85a48c) parent(s): 0f55a9eb73b author: Aleksey Midenkov <midenok@gmail.com> committer: Aleksey Midenkov <midenok@gmail.com> timestamp: 2019-06-27 18:05:25 +0300 message:
MDEV-16222 Assertion `0' failed in row_purge_remove_sec_if_poss_leaf on table with virtual columns and indexes
Cause Stale thd->m_stmt_da->m_sql_errno which is from different invocation.
Fix Reset error state before attempt to open table.
diff --git a/mysql-test/suite/innodb/r/purge_secondary_mdev-16222.result b/mysql-test/suite/innodb/r/purge_secondary_mdev-16222.result new file mode 100644 index 00000000000..30e8f9800fb --- /dev/null +++ b/mysql-test/suite/innodb/r/purge_secondary_mdev-16222.result @@ -0,0 +1,34 @@ ... +select * from t1 into outfile 'load.data'; +Warnings: +Warning 1287 '<select expression> INTO <destination>;' is deprecated and will be removed in a future release. Please use 'SELECT <select list> INTO <destination> FROM...' instead
See the warning, better not to use the deprecated syntax, unless you specifically want to test it,
+load data infile 'load.data' replace into table t1; +set debug_sync= "now WAIT_FOR latch_released"; +set global debug_dbug= "-d,ib_purge_virtual_mdev_16222_1"; diff --git a/sql/sql_class.cc b/sql/sql_class.cc index d65b6b8ed8d..cb73b7db2de 100644 --- a/sql/sql_class.cc +++ b/sql/sql_class.cc @@ -4762,7 +4762,10 @@ TABLE *open_purge_table(THD *thd, const char *db, size_t dblen, DBUG_ASSERT(!error || !ot_ctx.can_recover_from_failed_open());
if (unlikely(error)) + { close_thread_tables(thd); + thd->clear_error();
We use thd->clear_error() in many places, but over years there were quite a few problems with it, and now I'd prefer to avoid it whenever possible.
The thing is, you want to remove a specific error on a specific code path, but thd->clear_error() removes all errors, and no matter how you got here.
Generally intercepting errors with Internal_error_handler is a much safer approach.
In this case, though, thd->clear_error() might be okay, but better move it up the stack, do it in the caller when there can be only one way to reach this code. Not in a kind-of-generically-looking open table helper.
+ }
DBUG_RETURN(error ? NULL : tl->table); } diff --git a/sql/table.cc b/sql/table.cc index 9ac167b6adb..93c2c5e88a8 100644 --- a/sql/table.cc +++ b/sql/table.cc @@ -8240,6 +8240,7 @@ int TABLE::update_virtual_fields(handler *h, enum_vcol_update_mode update_mode)
int TABLE::update_virtual_field(Field *vf) { + DBUG_ASSERT(!in_use->is_error());
Yes, that's what I mean above. There were cases when update_virtual_field() was called when is_error() was true and is_error() introduced a bug.
I don't quite remember details, but it was something like, after the error has happened somewhere, on the way to returning an error, it was calling update_virtual_field() and some code in a function down the stack was, like
some_function_which_returns_void(....); if (thd->is_error()) // the error inside a function { ... handle it ... }
and the error in this case was not caused by anything in the function, but existed from before.
The point is, is_error() and clear_error() are global, and you're generally interested in local state. Whether _this block_ caused an error. Clear the error caused by _this function call_. And so on, not clear an error that might've happened some time somewhere at the undefined point in the past before this code line was reached.
Query_arena backup_arena; DBUG_ENTER("TABLE::update_virtual_field"); in_use->set_n_backup_active_arena(expr_arena, &backup_arena);
Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
-- All the best, Aleksey Midenkov @midenok
Hi, Aleksey! On Jul 05, Aleksey Midenkov wrote:
Hello Sergei!
Actually I was aware of that, but open_purge_table() looks so non-generic, so I decided to push that down. Well, I agree, it's "kind of" generic and it will be safer to move a bit higher. But why to remove DEBUG_ASSERT? update_virtual_field() relies on is_error() from inside. If error comes from outside it will return TRUE illegally. Like it happened with current bug. If there were assert in the first place it didn't took so long to debug.
Ideally I'd prefer update_virtual_field() not to rely on thd->is_error(). But changing this is clearly out of the scope of this bug. So okay, let's keep the assert. Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
participants (2)
-
Aleksey Midenkov -
Sergei Golubchik