maria-developers team mailing list archive

[Oleksandr Byelkin <sanja@xxxxxxxxxxxxxxxx>]

On 08.06.15 18:49, Sergey Petrunia wrote:
> On Thu, Apr 23, 2015 at 07:11:09PM +0200, sanja@xxxxxxxxxxx wrote:
> > revision-id: 5ccbfa02d4da91c3f9080dc1b1627d19cf40eccd
> > parent(s): c7efc0315b51f214b6423ae4dcb4ddeb1b14e91a
> > committer: Oleksandr Byelkin
> > branch nick: server
> > timestamp: 2015-04-23 19:11:06 +0200
> > message:
> >
> > MDEV-7445: Server crash with Signal 6
> >
> > Problem was in rewriting left expression which had 2 references on it. Solved with making subselect reference main.
> >
> > Item_in_optimized can have not Item_in_subselect reference in left part so type casting with no check is dangerous.
> >
> > Item::cols() should be checked after Item::fix_fields().
> >
> > ---
> ...
> > diff --git a/sql/item_cmpfunc.cc b/sql/item_cmpfunc.cc
> > index 998cb1c..51a38f0 100644
> > --- a/sql/item_cmpfunc.cc
> > +++ b/sql/item_cmpfunc.cc
> > @@ -1442,9 +1442,25 @@ bool Item_in_optimizer::eval_not_null_tables(uchar *opt_arg)
> >   bool Item_in_optimizer::fix_left(THD *thd, Item **ref)
> >   {
> >     DBUG_ENTER("Item_in_optimizer::fix_left");
> > -  if ((!args[0]->fixed && args[0]->fix_fields(thd, args)) ||
> > -      (!cache && !(cache= Item_cache::get_cache(args[0]))))
> > +  Item **ref0= args;
> > +  if (args[1]->type() == Item::SUBSELECT_ITEM &&
> > +      ((Item_subselect *)args[1])->is_in_predicate())
> > +  {
> > +    /*
> > +       left_expr->fix_fields() may cause left_expr to be substituted for
> > +       another item. (e.g. an Item_field may be changed into Item_ref). This
> > +       transformation is undone at the end of statement execution (e.g. the
> > +       Item_ref is deleted). However, Item_in_optimizer::args[0] may keep
> > +       the pointer to the post-transformation item. Because of that, on the
> > +       next execution we need to copy args[1]->left_expr again.
> > +    */
> > +    ref0= &(((Item_in_subselect *)args[1])->left_expr);
> > +    args[0]= ref0[0];
> > +  }
> > +  if ((!args[0]->fixed && args[0]->fix_fields(thd, ref0)) ||
> > +      (!cache && !(cache= Item_cache::get_cache(ref0[0]))))
> >       DBUG_RETURN(1);
> > +  args[0]= ref0[0];
> >     DBUG_PRINT("info", ("actual fix fields"));
> >   
> >     cache->setup(args[0]);
> > @@ -1500,6 +1516,16 @@ bool Item_in_optimizer::fix_left(THD *thd, Item **ref)
> >   bool Item_in_optimizer::fix_fields(THD *thd, Item **ref)
> >   {
> >     DBUG_ASSERT(fixed == 0);
> > +  Item_subselect *sub= 0;
> > +  uint col;
> > +
> > +  /*
> > +     MAX/MIN optimization can convert the subquery into
> > +     expr + Item_singlerow_subselect
> > +   */
> (*)
> > +  if (args[1]->type() == Item::SUBSELECT_ITEM)
> > +    sub= (Item_subselect *)args[1];
> > +
> I was looking for the case where the above if statement is not taken.
> Eventually I have applied this patch, the patch after it
> (e540d023e2ec6f37efc9ab695ccdfd4a6744ad64) to get the test cases,
>
> then added this at line (*):
>    DBUG_ASSERT(args[1]->type() == Item::SUBSELECT_ITEM);
>
> and ran the entire main test suite, with and without --ps-protocol. It worked.
> Can you add test coverage for this case?
Nowdays min/max transformation done later so code was there "just in 
case" to be in line with all other places where we could expect 
non-subselect (like execution). But non of the code should show a bad 
example so it was fixed.

> >     if (fix_left(thd, ref))
> >       return TRUE;
> >     if (args[0]->maybe_null)
> > @@ -1507,10 +1533,10 @@ bool Item_in_optimizer::fix_fields(THD *thd, Item **ref)
> >   
> >     if (!args[1]->fixed && args[1]->fix_fields(thd, args+1))
> >       return TRUE;
> > -  Item_in_subselect * sub= (Item_in_subselect *)args[1];
> > -  if (args[0]->cols() != sub->engine->cols())
> > +  if ((sub && ((col= args[0]->cols()) != sub->engine->cols())) ||
> > +      (!sub && (args[1]->cols() != (col= 1))))
> >     {
> > -    my_error(ER_OPERAND_COLUMNS, MYF(0), args[0]->cols());
> > +    my_error(ER_OPERAND_COLUMNS, MYF(0), col);
> >       return TRUE;
> >     }
> >     if (args[1]->maybe_null)
> ...
>
> BR
>   Sergei