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 committer: Aleksey Midenkov 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