Hello Jorgen,
In general the fix is ok. There is few minor comments (see inline).
It's ok to push after those being addressed.
Regards, Evgen.
Jorgen Loland wrote:
> #At file:///localhome/jl208045/mysql/mysql-5.1-branches/mysql-5.1/ based on
> revid:bjorn.munch@stripped
>
> 3180 Jorgen Loland 2009-10-26
> Bug#48177 - SELECTs with NOT IN subqueries containing NULL
> values return too many records
[skip]
> +
> +# Handler_read_rnd_next should be 18
> +SHOW STATUS LIKE '%Handler_read_rnd_next';
> +Variable_name Value
> +Handler_read_rnd_next 18
It's not clear why here should be exactly 18 reads. If number of reads changes
by some reason it would be hard to find the reason.
Thus I suggest you to show number of reads with only one (null,null) row,
then insert copule more, flush status and show number of reads again.
Or you can use user-variable to produce a side-effect for the subquery and
show that it isn't evaluated for second (null,null) row.
I think the last one will be even better.
> +DROP TABLE t1,t2;
> +End of 5.1 tests
>
> === modified file 'mysql-test/t/subselect3.test'
> --- a/mysql-test/t/subselect3.test 2009-08-31 14:09:09 +0000
> +++ b/mysql-test/t/subselect3.test 2009-10-26 11:49:16 +0000
> @@ -728,3 +728,55 @@ where
> from t4, t5 limit 2));
[skip]
> /*
> - We're evaluating "NULL IN (SELECT ...)". The result can be NULL or
> - FALSE, and we can return one instead of another. Just return NULL.
> + The case where a NULL value in the outer_value_list means that
> + the result shall be NULL/FALSE (makes no difference). The
> + cached value is NULL, so just return NULL.
After reading this comment one may think that there is no difference between
FALSE/NULLbeing returned at all. But this is true only when
subquery->top_level_item() is true also. Please, re-phrase it.
> */
> null_value= 1;
> }
> else
> {
[skip]
> + if (!((Item_in_subselect*)args[1])->is_correlated &&
> + all_left_cols_null && result_for_null_param != UNKNOWN)
You can remove either this or the check below because
result_for_null_param depends on the all_left_cols_null variable.
> + {
> + /*
> + This is a non-correlated subquery, all values in the outer
> + value list are NULL, and we have already evaluated the
> + subquery for all NULL values: Return the same result we
> + did last time without evaluating the subquery.
> + */
> null_value= result_for_null_param;
> - }
> - else
> + }
> + else
[skip]
> - (void) args[1]->val_bool_result();
> - result_for_null_param= null_value= !item_subs->engine->no_rows();
> -
> - /* Turn all predicates back on */
> - for (i= 0; i < ncols; i++)
> - item_subs->set_cond_guard_var(i, TRUE);
> - }
> + /* The subquery has to be evaluated */
> + (void) args[1]->val_bool_result();
> + null_value= !item_subs->engine->no_rows();
> + if (all_left_cols_null)
This check ^^^^
> + result_for_null_param= null_value;
> }
> +
> + /* Turn all predicates back on */
> + for (i= 0; i < ncols; i++)
> + item_subs->set_cond_guard_var(i, TRUE);
> }
> return 0;
> }
