Hi, Evgeny!
On Nov 21, Evgeny Potemkin wrote:
> Sergei Golubchik wrote:
>> Hi, Evgeny!
>> On Nov 13, Evgeny Potemkin wrote:
>>> #At file:///work/bzrroot/33546-bug-5.1-bugteam/ based on
>>> revid:epotemkin@stripped
>>>
>>> 3202 Evgeny Potemkin 2009-11-13
>>> Bug#33546: Slowdown on re-evaluation of constant expressions.
>>
>>> === modified file 'mysql-test/r/select.result'
>>> --- a/mysql-test/r/select.result 2009-10-30 14:13:13 +0000
>>> +++ b/mysql-test/r/select.result 2009-11-13 15:21:37 +0000
>>> @@ -4425,7 +4425,7 @@ INSERT INTO t1 VALUES (2),(3);
>>> SELECT 1 FROM t1 WHERE a <> 1 AND NOT
>>> ROW(a,a) <=> ROW((SELECT 1 FROM t1 WHERE 1=2),(SELECT 1 FROM t1))
>>> INTO @var0;
>>> -ERROR 21000: Subquery returns more than 1 row
>>> +ERROR 42000: Result consisted of more than one row
>> why ?
> This thing is: without this fix prior to comparison Arg_comparator
> calls row->bring_value(), it evaluates each subquery, the first one
> returns 0 rows, and the the second returns 2, and the error is thrown.
> with this fix the ROW() function gets cached and
> Item_cache::bring_value does nothing and subqueries are evaluated only
> when called for a value. Since the first subquery doesn't return row,
> its comparison with 'a' is always false and the second subquery is
> never evaluated. This query sends its result to @var0 variable, and
> when it tries to save second value the error is thrown. But if there
> would be no INTO clause the query would return rows, which is correct
> IMO because WHERE is correctly evaluated to TRUE even without
> evaluating second subquery.
Hm, okay, I tend to agree with it.
Few comments:
1. strictly speaking ROW is not a function :)
2. why is it get cached ?
3. we need to keep the original test case - that is a test case for "Bug
#48291 : crash with row() operator,select into @var, and subquery
returning multiple rows". I suppose you could modify the query to
make sure this case is covered - e.g. replace the first subquery
with a constant, or something.
4. but it's good to have a test for what you've described too - so you
can keep the original query (but I think it'll be clearer without
INTO and with a comment explaing why second subquery doesn't cause an
error) and a new modified query that tests bug#48291.
>> 2. add a test that BENCHMARK() function works as before. That is, a
>> test that BENCHMARK argument is not cached (that's easy to do, just
>> filter out BENCHMAK_FUNC in the analyze callback).
> Our manual says that BENCHMARK is supposed to be used in the select
> list, not in WHERE/ON clauses, and the select list currently isn't
> cached. The reason for not caching is because there isn't much
> constant expressions and they're evaluated only for sending result and
> saving to tmp table. If you think it's worth to add caching of select
> list, I'll do and add test for BENCHMARK. Currently the test isn't
> needed.
ok. I still think that it's kind of weird that BENCHMARK will be
optimized in WHERE or HAVING, but believe that it's acceptable.
I agree that we don't need to optimize expressions in the SELECT clause.
>>> === modified file 'sql/sql_select.cc'
>>> --- a/sql/sql_select.cc 2009-11-06 14:54:19 +0000
>>> +++ b/sql/sql_select.cc 2009-11-13 15:21:37 +0000
>>> @@ -1078,6 +1078,9 @@ JOIN::optimize()
>>> {
>>> conds=new Item_int((longlong) 0,1); // Always false
>>> }
>>> + /* Cache constant expressions in select list, WHERE, HAVING, ON. */
>>> + cache_const_exprs();
>> why here ?
>> isn't that a bit too early ? e.g. const tables aren't read yet, that is
>> some values are fields, but they'll turn into constants just few lines
>> below.
> No, const tables are read in make_join_statistics. At this point
> all transformations of WHERE/ON clauses are finished and it's easy to tell
> which parts are constant.
Ah, yes, sorry.
Regards
Sergei
