maria-developers team mailing list archive

[Aleksey Midenkov <midenok@xxxxxxxxx>]

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@xxxxxxxxxxx> 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@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



-- 
All the best,

Aleksey Midenkov
@midenok