maria-developers team mailing list archive

[Alexander Barkov <bar@xxxxxxxxxxx>]

Hi Sergei,

Thanks for the review!

On 04/27/2016 06:24 PM, Sergei Golubchik wrote:
> Hi, Alexander!
>
> The approach is fine. Two comments below:
>
> > The remaining heavy piece of the code is:
> >
> >    Item* propagate_equal_fields(THD *thd, const Context &ctx, COND_EQUAL *cond)
> >    {
> >      Context cmpctx(ANY_SUBST, cmp.compare_type(), cmp.compare_collation());
> >      args[0]->propagate_equal_fields_and_change_item_tree(thd, cmpctx,
> >                                                           cond, &args[0]);
> >      args[1]->propagate_equal_fields_and_change_item_tree(thd, cmpctx,
> >                                                           cond, &args[1]);
> >      args[2]->propagate_equal_fields_and_change_item_tree(thd,
> >                                                           Context_identity(),
> >                                                           cond, &args[2]);
> >      return this;
> >    }
> >
> > We cannot use the same approach here, because args[0] and args[2]
> > are propagated with a different context (ANY_SUBST vs IDENTITY_SUBST).
>
> An idea:
>
>    Item *old= args[0];
>    args[0]->propagate_equal_fields_and_change_item_tree(thd, cmpctx,
>                                                         cond, &args[0]);
>    if (args[0] != old)
>        args[2]->propagate_equal_fields_and_change_item_tree(thd, Context_identity(),
>                                                             cond, &args[2]);
>
> First substitution uses more relaxed rules, so, supposedly, if it did
> not substitute anything, than a stricter IDENTITY_SUBST ssubstitution
> won't substitute anything either.

Looks like a very good solution.

This reduced execution time for the reported query back to 0.0 seconds 
again :)

Brilliant!

> > @@ -2513,7 +2530,14 @@ void Item_func_nullif::update_used_tables()
> >     }
> >     else
> >     {
> > -    Item_func::update_used_tables();
> > +    /*
> > +      No needs to iterate through args[2] when it's just a copy of args[0].
> > +      See MDEV-9712 Performance degradation of nested NULLIF
> > +    */
> > +    DBUG_ASSERT(arg_count == 3);
> > +    used_tables_and_const_cache_init();
> > +    used_tables_and_const_cache_update_and_join(args[0] == args[2] ? 2 : 3,
> > +                                                args);
>
> here, you can unconditionally run
>
>    used_tables_and_const_cache_update_and_join(2, args);
>
> because even if args[2] differs from args[0], it cannot use more
> tables, so for the purpose of this method, it's redundant.

I'm not sure.

After equal const propagation, update_used_tables() is called again 2 
times in this script:

DROP TABLE IF EXISTS t1;
CREATE TABLE t1 (a BINARY(1));
INSERT INTO t1 VALUES ('a'),('b');
SELECT * FROM t1 WHERE  NULLIF(a,'a') IS NULL AND a='a';

<offtopic>
Btw, update_used_tables() is called:
- 3 times before propagate_equal_fields()
- 2 times after propagate_equal_fields()
Perhaps we should generally try to reduce the amount of 
update_used_tables().
</offtopic>


When update_used_tables() is called after propagate_equal_fields(),
args[0] can already be a constant, while args[2] can still point to the
original expression involving tables.

So args[2] can have more tables, it seems.

Can you clarify please?


> >     }
> >   }
>
> Regards,
> Sergei
> Chief Architect MariaDB
> and security@xxxxxxxxxxx