[Commits] cb07364: MDEV-15035 Wrong results when calling a stored procedure

Oleksandr Byelkin sanja at mariadb.com
Wed Apr 25 10:55:37 EEST 2018


Hi, Igor!

In general everything is good and it can be pushed.

I see that void Item_func_interval::fix_length_and_dec() is doing something
which fix_length_and_dec is not made for (collect statistics which should
be collected by fix_fields),  but it is not your problem and it is not the
version and task to fix it.


On Wed, Apr 25, 2018 at 12:51 AM, IgorBabaev <igor at mariadb.com> wrote:

> revision-id: cb07364f1d1ec47febbcaf2397e3ed2b4c3dc06b
> (mariadb-5.5.59-59-gcb07364)
> parent(s): 5e61e1716e763315009318081fba5994b8910242
> author: Igor Babaev
> committer: Igor Babaev
> timestamp: 2018-04-24 15:51:49 -0700
> message:
>
> MDEV-15035 Wrong results when calling a stored procedure
> multiple times with different arguments.
>
> If the ON expression of an outer join is an OR formula with one
> of the disjunct being a constant formula then the expression
> cannot be null-rejected if the constant formula is true. Otherwise
> it can be null-rejected and if so the outer join can be converted
> into inner join. This optimization was added in the patch for
> mdev-4817. Yet the code had a defect: if the query was used in
> a stored procedure with parameters and the constant item contained
> some of them then the value of this constant item depended on the
> values of the parameters. With some parameters it may be true,
> for others not. The validity of conversion to inner join is checked
> only once and it happens only for the first call of procedure.
> So if the  parameters in the first call allowed the conversion it
> was done and next calls used the transformed query though there
> could be calls whose parameters made the conversion invalid.
>
> Fixed by cheking whether the constant disjunct in the ON expression
> originally contained an SP parameter. If so the expression is not
> considered as null-rejected. For this check a new item's attribute
> was intruduced: Item::with_param. It is calculated for each item
> by fix fields() functions.
> Also moved the call of optimize_constant_subqueries() in
> JOIN::optimize after the call of simplify_joins(). The reason
> for this is that after the optimization introduced by the patch
> for mdev-4817 simplify_joins() can use the results of execution
> of non-expensive constant subqueries and this is not valid.
>
> ---
>  mysql-test/r/sp-innodb.result | 34 ++++++++++++++++++++++++++++++++++
>  mysql-test/t/sp-innodb.test   | 42 ++++++++++++++++++++++++++++++
> ++++++++++++
>  sql/item.cc                   |  7 +++++++
>  sql/item.h                    |  1 +
>  sql/item_cmpfunc.cc           | 43 ++++++++++++++++++++++++++++++
> +------------
>  sql/item_func.cc              |  1 +
>  sql/item_func.h               |  7 +++++++
>  sql/item_row.cc               |  1 +
>  sql/item_sum.cc               |  3 +++
>  sql/sql_select.cc             |  6 +++---
>  10 files changed, 130 insertions(+), 15 deletions(-)
>
> diff --git a/mysql-test/r/sp-innodb.result b/mysql-test/r/sp-innodb.result
> index b5fe920..9daf2c4 100644
> --- a/mysql-test/r/sp-innodb.result
> +++ b/mysql-test/r/sp-innodb.result
> @@ -138,3 +138,37 @@ SET @@innodb_lock_wait_timeout=
> @innodb_lock_wait_timeout_saved;
>  #
>  # BUG 16041903: End of test case
>  #
> +#
> +# MDEV-15035: SP using query with outer join and a parameter
> +#             in ON expression
> +#
> +CREATE TABLE t1 (
> +id int NOT NULL,
> +PRIMARY KEY (id)
> +) ENGINE=InnoDB;
> +INSERT INTO t1 VALUES (1), (2);
> +CREATE TABLE t2 (
> +id int NOT NULL,
> +id_foo int NOT NULL,
> +PRIMARY KEY (id)
> +) ENGINE=InnoDB;
> +INSERT INTO t2 VALUES (1, 1);
> +DROP PROCEDURE IF EXISTS test_proc;
> +CREATE PROCEDURE test_proc(IN param int)
> +LANGUAGE SQL
> +READS SQL DATA
> +BEGIN
> +SELECT DISTINCT f.id
> +FROM t1 f
> +LEFT OUTER JOIN t2 b ON b.id_foo = f.id
> +WHERE (param <> 0 OR b.id IS NOT NULL);
> +END|
> +CALL test_proc(0);
> +id
> +1
> +CALL test_proc(1);
> +id
> +1
> +2
> +DROP PROCEDURE IF EXISTS test_proc;
> +DROP TABLE t1, t2;
> diff --git a/mysql-test/t/sp-innodb.test b/mysql-test/t/sp-innodb.test
> index 2371516..e44a853 100644
> --- a/mysql-test/t/sp-innodb.test
> +++ b/mysql-test/t/sp-innodb.test
> @@ -158,5 +158,47 @@ SET @@innodb_lock_wait_timeout=
> @innodb_lock_wait_timeout_saved;
>  --echo # BUG 16041903: End of test case
>  --echo #
>
> +--echo #
> +--echo # MDEV-15035: SP using query with outer join and a parameter
> +--echo #             in ON expression
> +--echo #
> +
> +CREATE TABLE t1 (
> +       id int NOT NULL,
> +       PRIMARY KEY (id)
> +) ENGINE=InnoDB;
> +
> +INSERT INTO t1 VALUES (1), (2);
> +
> +CREATE TABLE t2 (
> +       id int NOT NULL,
> +       id_foo int NOT NULL,
> +       PRIMARY KEY (id)
> +) ENGINE=InnoDB;
> +
> +INSERT INTO t2 VALUES (1, 1);
> +
> +--disable_warnings
> +DROP PROCEDURE IF EXISTS test_proc;
> +--enable_warnings
> +
> +DELIMITER |;
> +CREATE PROCEDURE test_proc(IN param int)
> +LANGUAGE SQL
> +READS SQL DATA
> +BEGIN
> +       SELECT DISTINCT f.id
> +       FROM t1 f
> +       LEFT OUTER JOIN t2 b ON b.id_foo = f.id
> +       WHERE (param <> 0 OR b.id IS NOT NULL);
> +END|
> +DELIMITER ;|
> +
> +CALL test_proc(0);
> +CALL test_proc(1);
> +
> +DROP PROCEDURE IF EXISTS test_proc;
> +DROP TABLE t1, t2;
> +
>  # Wait till we reached the initial number of concurrent sessions
>  --source include/wait_until_count_sessions.inc
> diff --git a/sql/item.cc b/sql/item.cc
> index 08a0061..c5c6df0 100644
> --- a/sql/item.cc
> +++ b/sql/item.cc
> @@ -504,6 +504,7 @@ Item::Item():
>    in_rollup= 0;
>    decimals= 0; max_length= 0;
>    with_subselect= 0;
> +  with_param= 0;
>    cmp_context= IMPOSSIBLE_RESULT;
>     /* Initially this item is not attached to any JOIN_TAB. */
>    join_tab_idx= MAX_TABLES;
> @@ -550,6 +551,7 @@ Item::Item(THD *thd, Item *item):
>    null_value(item->null_value),
>    unsigned_flag(item->unsigned_flag),
>    with_sum_func(item->with_sum_func),
> +  with_param(item->with_param),
>    with_field(item->with_field),
>    fixed(item->fixed),
>    is_autogenerated_name(item->is_autogenerated_name),
> @@ -1486,6 +1488,9 @@ bool Item_sp_variable::fix_fields(THD *thd, Item **)
>    max_length= it->max_length;
>    decimals= it->decimals;
>    unsigned_flag= it->unsigned_flag;
> +  with_param= 1;
> +  if (thd->lex->current_select->master_unit()->item)
> +    thd->lex->current_select->master_unit()->item->with_param= 1;
>    fixed= 1;
>    collation.set(it->collation.collation, it->collation.derivation);
>
> @@ -7234,6 +7239,7 @@ void Item_ref::set_properties()
>      split_sum_func() doesn't try to change the reference.
>    */
>    with_sum_func= (*ref)->with_sum_func;
> +  with_param= (*ref)->with_param;
>    with_field= (*ref)->with_field;
>    unsigned_flag= (*ref)->unsigned_flag;
>    fixed= 1;
> @@ -7681,6 +7687,7 @@ Item_cache_wrapper::Item_cache_wrapper(Item
> *item_arg)
>    decimals=   orig_item->decimals;
>    collation.set(orig_item->collation);
>    with_sum_func= orig_item->with_sum_func;
> +  with_param= orig_item->with_param;
>    with_field= orig_item->with_field;
>    unsigned_flag= orig_item->unsigned_flag;
>    name= item_arg->name;
> diff --git a/sql/item.h b/sql/item.h
> index 830f8bf..d756cf8 100644
> --- a/sql/item.h
> +++ b/sql/item.h
> @@ -644,6 +644,7 @@ class Item {
>    bool null_value;                     /* if item is null */
>    bool unsigned_flag;
>    bool with_sum_func;                   /* True if item contains a sum
> func */
> +  bool with_param;                      /* True if contains an SP
> parameter */
>    /**
>      True if any item except Item_sum_func contains a field. Set during
> parsing.
>    */
> diff --git a/sql/item_cmpfunc.cc b/sql/item_cmpfunc.cc
> index 39f497e..6fb650b 100644
> --- a/sql/item_cmpfunc.cc
> +++ b/sql/item_cmpfunc.cc
> @@ -1546,6 +1546,7 @@ bool Item_in_optimizer::fix_left(THD *thd, Item
> **ref)
>    }
>    eval_not_null_tables(NULL);
>    with_sum_func= args[0]->with_sum_func;
> +  with_param= args[0]->with_param || args[1]->with_param;
>    with_field= args[0]->with_field;
>    if ((const_item_cache= args[0]->const_item()))
>    {
> @@ -1587,6 +1588,7 @@ bool Item_in_optimizer::fix_fields(THD *thd, Item
> **ref)
>    with_subselect= 1;
>    with_sum_func= with_sum_func || args[1]->with_sum_func;
>    with_field= with_field || args[1]->with_field;
> +  with_param= args[0]->with_param || args[1]->with_param;
>    used_tables_cache|= args[1]->used_tables();
>    const_item_cache&= args[1]->const_item();
>    fixed= 1;
> @@ -2108,6 +2110,7 @@ void Item_func_interval::fix_length_and_dec()
>    used_tables_cache|= row->used_tables();
>    not_null_tables_cache= row->not_null_tables();
>    with_sum_func= with_sum_func || row->with_sum_func;
> +  with_param= with_param || row->with_param;
>    with_field= with_field || row->with_field;
>    const_item_cache&= row->const_item();
>  }
> @@ -4335,6 +4338,7 @@ Item_cond::fix_fields(THD *thd, Item **ref)
>    List_iterator<Item> li(list);
>    Item *item;
>    uchar buff[sizeof(char*)];                   // Max local vars in
> function
> +  bool is_and_cond= functype() == Item_func::COND_AND_FUNC;
>    not_null_tables_cache= used_tables_cache= 0;
>    const_item_cache= 1;
>
> @@ -4396,26 +4400,33 @@ Item_cond::fix_fields(THD *thd, Item **ref)
>         (item= *li.ref())->check_cols(1))
>        return TRUE; /* purecov: inspected */
>      used_tables_cache|=     item->used_tables();
> -    if (item->const_item())
> +    if (item->const_item() && !item->with_param &&
> +        !item->is_expensive() && !cond_has_datetime_is_null(item))
>      {
> -      if (!item->is_expensive() && !cond_has_datetime_is_null(item) &&
> -          item->val_int() == 0)
> +      if (item->val_int() == is_and_cond && top_level())
>        {
>          /*
> -          This is "... OR false_cond OR ..."
> +          a. This is "... AND true_cond AND ..."
> +          In this case, true_cond  has no effect on
> cond_and->not_null_tables()
> +          b. This is "... OR false_cond/null cond OR ..."
>            In this case, false_cond has no effect on
> cond_or->not_null_tables()
>          */
>        }
>        else
>        {
>          /*
> -          This is  "... OR const_cond OR ..."
> +          a. This is "... AND false_cond/null_cond AND ..."
> +          The whole condition is FALSE/UNKNOWN.
> +          b. This is  "... OR const_cond OR ..."
>            In this case, cond_or->not_null_tables()=0, because the
> condition
>            const_cond might evaluate to true (regardless of whether some
> tables
>            were NULL-complemented).
>          */
> +        not_null_tables_cache= (table_map) 0;
>          and_tables_cache= (table_map) 0;
>        }
> +      if (thd->is_error())
> +        return TRUE;
>      }
>      else
>      {
> @@ -4427,6 +4438,7 @@ Item_cond::fix_fields(THD *thd, Item **ref)
>      }
>
>      with_sum_func=         with_sum_func || item->with_sum_func;
> +    with_param=             with_param || item->with_param;
>      with_field=             with_field || item->with_field;
>      with_subselect|=        item->has_subquery();
>      if (item->maybe_null)
> @@ -4443,30 +4455,36 @@ bool
>  Item_cond::eval_not_null_tables(uchar *opt_arg)
>  {
>    Item *item;
> +  bool is_and_cond= functype() == Item_func::COND_AND_FUNC;
>    List_iterator<Item> li(list);
>    not_null_tables_cache= (table_map) 0;
>    and_tables_cache= ~(table_map) 0;
>    while ((item=li++))
>    {
>      table_map tmp_table_map;
> -    if (item->const_item())
> +    if (item->const_item() && !item->with_param &&
> +        !item->is_expensive() && !cond_has_datetime_is_null(item))
>      {
> -      if (!item->is_expensive() && !cond_has_datetime_is_null(item) &&
> -          item->val_int() == 0)
> +      if (item->val_int() == is_and_cond && top_level())
>        {
>          /*
> -          This is "... OR false_cond OR ..."
> +          a. This is "... AND true_cond AND ..."
> +          In this case, true_cond  has no effect on
> cond_and->not_null_tables()
> +          b. This is "... OR false_cond/null cond OR ..."
>            In this case, false_cond has no effect on
> cond_or->not_null_tables()
>          */
>        }
>        else
>        {
>          /*
> -          This is  "... OR const_cond OR ..."
> +          a. This is "... AND false_cond/null_cond AND ..."
> +          The whole condition is FALSE/UNKNOWN.
> +          b. This is  "... OR const_cond OR ..."
>            In this case, cond_or->not_null_tables()=0, because the
> condition
> -          some_cond_or might be true regardless of what tables are
> -          NULL-complemented.
> +          const_cond might evaluate to true (regardless of whether some
> tables
> +          were NULL-complemented).
>          */
> +        not_null_tables_cache= (table_map) 0;
>          and_tables_cache= (table_map) 0;
>        }
>      }
> @@ -5118,6 +5136,7 @@ Item_func_regex::fix_fields(THD *thd, Item **ref)
>         args[1]->fix_fields(thd, args + 1)) || args[1]->check_cols(1))
>      return TRUE;                               /* purecov: inspected */
>    with_sum_func=args[0]->with_sum_func || args[1]->with_sum_func;
> +  with_param=args[0]->with_param || args[1]->with_param;
>    with_field= args[0]->with_field || args[1]->with_field;
>    with_subselect= args[0]->has_subquery() || args[1]->has_subquery();
>    max_length= 1;
> diff --git a/sql/item_func.cc b/sql/item_func.cc
> index 9e4edfc..8b3c72dd3 100644
> --- a/sql/item_func.cc
> +++ b/sql/item_func.cc
> @@ -222,6 +222,7 @@ Item_func::fix_fields(THD *thd, Item **ref)
>         maybe_null=1;
>
>        with_sum_func= with_sum_func || item->with_sum_func;
> +      with_param= with_param || item->with_param;
>        with_field= with_field || item->with_field;
>        used_tables_cache|=     item->used_tables();
>        const_item_cache&=      item->const_item();
> diff --git a/sql/item_func.h b/sql/item_func.h
> index 5781822..3a609fc 100644
> --- a/sql/item_func.h
> +++ b/sql/item_func.h
> @@ -83,6 +83,7 @@ class Item_func :public Item_result_field
>      args= tmp_arg;
>      args[0]= a;
>      with_sum_func= a->with_sum_func;
> +    with_param= a->with_param;
>      with_field= a->with_field;
>    }
>    Item_func(Item *a,Item *b):
> @@ -91,6 +92,7 @@ class Item_func :public Item_result_field
>      args= tmp_arg;
>      args[0]= a; args[1]= b;
>      with_sum_func= a->with_sum_func || b->with_sum_func;
> +    with_param= a->with_param || b->with_param;
>      with_field= a->with_field || b->with_field;
>    }
>    Item_func(Item *a,Item *b,Item *c):
> @@ -102,6 +104,7 @@ class Item_func :public Item_result_field
>        arg_count= 3;
>        args[0]= a; args[1]= b; args[2]= c;
>        with_sum_func= a->with_sum_func || b->with_sum_func ||
> c->with_sum_func;
> +      with_param= a->with_param || b->with_param || c->with_param;
>        with_field= a->with_field || b->with_field || c->with_field;
>      }
>    }
> @@ -115,6 +118,8 @@ class Item_func :public Item_result_field
>        args[0]= a; args[1]= b; args[2]= c; args[3]= d;
>        with_sum_func= a->with_sum_func || b->with_sum_func ||
>         c->with_sum_func || d->with_sum_func;
> +      with_param= a->with_param || b->with_param ||
> +        c->with_param || d->with_param;
>        with_field= a->with_field || b->with_field ||
>          c->with_field || d->with_field;
>      }
> @@ -128,6 +133,8 @@ class Item_func :public Item_result_field
>        args[0]= a; args[1]= b; args[2]= c; args[3]= d; args[4]= e;
>        with_sum_func= a->with_sum_func || b->with_sum_func ||
>         c->with_sum_func || d->with_sum_func || e->with_sum_func ;
> +      with_param= a->with_param || b->with_param ||
> +        c->with_param || d->with_param || e->with_param;
>        with_field= a->with_field || b->with_field ||
>          c->with_field || d->with_field || e->with_field;
>      }
> diff --git a/sql/item_row.cc b/sql/item_row.cc
> index 9e81c05..9fe34dd 100644
> --- a/sql/item_row.cc
> +++ b/sql/item_row.cc
> @@ -125,6 +125,7 @@ bool Item_row::fix_fields(THD *thd, Item **ref)
>      with_sum_func= with_sum_func || item->with_sum_func;
>      with_field= with_field || item->with_field;
>      with_subselect|= item->with_subselect;
> +    with_param|= item->with_param;
>    }
>    fixed= 1;
>    return FALSE;
> diff --git a/sql/item_sum.cc b/sql/item_sum.cc
> index 709c2b6..16334cd 100644
> --- a/sql/item_sum.cc
> +++ b/sql/item_sum.cc
> @@ -1164,6 +1164,7 @@ Item_sum_num::fix_fields(THD *thd, Item **ref)
>        return TRUE;
>      set_if_bigger(decimals, args[i]->decimals);
>      with_subselect|= args[i]->with_subselect;
> +    with_param|= args[i]->with_param;
>    }
>    result_field=0;
>    max_length=float_length(decimals);
> @@ -1195,6 +1196,7 @@ Item_sum_hybrid::fix_fields(THD *thd, Item **ref)
>      return TRUE;
>    decimals=item->decimals;
>    with_subselect= args[0]->with_subselect;
> +  with_param= args[0]->with_param;
>
>    switch (hybrid_type= item->result_type()) {
>    case INT_RESULT:
> @@ -3430,6 +3432,7 @@ Item_func_group_concat::fix_fields(THD *thd, Item
> **ref)
>          args[i]->check_cols(1))
>        return TRUE;
>      with_subselect|= args[i]->with_subselect;
> +    with_param|= args[i]->with_param;
>    }
>
>    /* skip charset aggregation for order columns */
> diff --git a/sql/sql_select.cc b/sql/sql_select.cc
> index 90bb536..1e9f1c0 100644
> --- a/sql/sql_select.cc
> +++ b/sql/sql_select.cc
> @@ -1034,9 +1034,6 @@ JOIN::optimize()
>
>    eval_select_list_used_tables();
>
> -  if (optimize_constant_subqueries())
> -    DBUG_RETURN(1);
> -
>    table_count= select_lex->leaf_tables.elements;
>
>    if (setup_ftfuncs(select_lex)) /* should be after having->fix_fields */
> @@ -1098,6 +1095,9 @@ JOIN::optimize()
>        thd->restore_active_arena(arena, &backup);
>    }
>
> +  if (optimize_constant_subqueries())
> +    DBUG_RETURN(1);
> +
>    if (setup_jtbm_semi_joins(this, join_list, &conds))
>      DBUG_RETURN(1);
>
> _______________________________________________
> commits mailing list
> commits at mariadb.org
> https://lists.askmonty.org/cgi-bin/mailman/listinfo/commits
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.askmonty.org/pipermail/commits/attachments/20180425/b5c3a8dc/attachment-0001.html>


More information about the commits mailing list