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

IgorBabaev igor at mariadb.com
Wed Apr 25 01:51:49 EEST 2018


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);
 


More information about the commits mailing list