maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Michael!

On Apr 16, Michael Widenius wrote:
> Hi!
> 
> On Thu, Apr 16, 2020 at 1:17 PM Sergei Golubchik <serg@xxxxxxxxxxx> wrote:
> 
> <cut>
> > where's a test case for MDEV-22077?
> 
> One can't use the test case in MDEV-22077 as it involves a table
> created by a 32 bit MariaDB version. Doing another test case it's very
> hard so I ignored it and just did run the test to verify the patch.

Alice posted a simple test case in the comments.
It has a very similar stack trace.
Could you check it out? Is it a same bug or a different one?

> > > diff --git a/sql/sql_delete.cc b/sql/sql_delete.cc
> > > index 19eabbb053c..2fc0de4345f 100644
> > > --- a/sql/sql_delete.cc
> > > +++ b/sql/sql_delete.cc
> > > @@ -547,10 +547,12 @@ bool mysql_delete(THD *thd, TABLE_LIST *table_list, COND *conds,
> > >      else
> > >      {
> > >        ha_rows scanned_limit= query_plan.scanned_rows;
> > > +      table->no_keyread= 1;
> > >        query_plan.index= get_index_for_order(order, table, select, limit,
> > >                                              &scanned_limit,
> > >                                              &query_plan.using_filesort,
> > >                                              &reverse);
> > > +      table->no_keyread= 0;
> >
> > why?
> 
> get_index_for_order is not allowed to change to use key_reads as there
> is no guarantee
> at this point that the suggested index will be used. That was the
> reason we got crashes.

Why does it enable keyread then? Where?

> > > @@ -28568,8 +28568,6 @@ test_if_cheaper_ordering(const JOIN_TAB *tab, ORDER *order, TABLE *table,
> > >    if (new_used_key_parts != NULL)
> > >      *new_used_key_parts= best_key_parts;
> > >    table->file->ha_end_keyread();
> > > -  if (is_best_covering && !table->no_keyread)
> > > -    table->file->ha_start_keyread(best_key);
> >
> > I find it very confusing that test_if_cheaper_ordering has a side effect
> > of ending keyread. It was ok, when it ended previous keyread and started
> > a new one. But just disabling - it's looks very strange.
> 
> It has to be disabled as we may in the future use another index than
> the one we originally planned to use (with keyread).
> This is something that should be re-factored in the future so that we
> only enable key-read's when we have done the final
> decision of which index we will really use.

Yes, but it can be disabled in the caller.
Or not even enabled that early.
Either approach is better than disabling keyread as a side effect of
test_if_cheaper_ordering.

Where is it enabled now?
 
Regards,
Sergei
VP of MariaDB Server Engineering
and security@xxxxxxxxxxx