maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

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@xxxxxxxxx>
> committer: Aleksey Midenkov <midenok@xxxxxxxxx>
> 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@xxxxxxxxxxx