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! 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 secur...@mariadb.org ___ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] 4670c85a48c: MDEV-16222 Assertion `0' failed in row_purge_remove_sec_if_poss_leaf on table with virtual columns and indexes
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 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
> > 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 000..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' INTO ;' is
> > deprecated and will be removed in a future release. Please use 'SELECT
> > INTO 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, _arena);
>
> Regards,
> Sergei
> VP of MariaDB Server Engineering
> and secur...@mariadb.org
--
All the best,
Aleksey Midenkov
@midenok
___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help : https://help.launchpad.net/ListHelp
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 000..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' INTO ;' is deprecated
> and will be removed in a future release. Please use 'SELECT
> INTO 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, _arena);
Regards,
Sergei
VP of MariaDB Server Engineering
and secur...@mariadb.org
___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help : https://help.launchpad.net/ListHelp
