MDEV-3812: Remove unneeded extra call to engine->exec() in Item_subselect::exec, remove enum store_key_result

This task fixes an ineffeciency that is a remainder from MySQL 5.0/5.1. There, subqueries
were optimized in a lazy manner, when executed for the first time. During this lazy optimization
it may happen that the server finds a more efficient subquery engine, and substitute the current
engine of the query being executed with the new engine. This required re-execution of the engine.

MariaDB 5.3 pre-optimizes subqueries in almost all cases, and the engine is chosen in most cases,
except when subquery materialization found that it must use partial matching. In this case, the
current code was performing one extra re-execution although it was not needed at all. The patch
performs the re-execution only if the engine was changed while executing.

In addition the patch performs small cleanup by removing "enum store_key_result" because it is
essentially a boolean, and the code that uses it already maps it to a boolean.

6 files changed, 31 insertions, 33 deletions

diff --git a/mysql-test/r/limit_rows_examined.result b/mysql-test/r/limit_rows_examined.result
index f4242f17a14..a51798a5883 100644
--- a/mysql-test/r/limit_rows_examined.result
+++ b/mysql-test/r/limit_rows_examined.result
@@ -318,7 +318,7 @@ LIMIT ROWS EXAMINED 9;
 c1
 bb
 Warnings:
-Warning	1931	Query execution was interrupted. The query examined at least 10 rows, which exceeds LIMIT ROWS EXAMINED (9). The query result may be incomplete.
+Warning	1931	Query execution was interrupted. The query examined at least 12 rows, which exceeds LIMIT ROWS EXAMINED (9). The query result may be incomplete.
 Same as above, without subquery cache
 set @@optimizer_switch='subquery_cache=off';
 select * from t1
@@ -347,7 +347,7 @@ LIMIT ROWS EXAMINED 5;
 c1
 bb
 Warnings:
-Warning	1931	Query execution was interrupted. The query examined at least 6 rows, which exceeds LIMIT ROWS EXAMINED (5). The query result may be incomplete.
+Warning	1931	Query execution was interrupted. The query examined at least 7 rows, which exceeds LIMIT ROWS EXAMINED (5). The query result may be incomplete.
 Subqueries with materialization
 set @@optimizer_switch='semijoin=off,in_to_exists=off,materialization=on,subquery_cache=on';
 explain
diff --git a/mysql-test/r/subselect3.result b/mysql-test/r/subselect3.result
index b33e7e113f2..049c4d14b1a 100644
--- a/mysql-test/r/subselect3.result
+++ b/mysql-test/r/subselect3.result
@@ -126,7 +126,7 @@ Handler_read_next	0
 Handler_read_prev	0
 Handler_read_rnd	0
 Handler_read_rnd_deleted	0
-Handler_read_rnd_next	50
+Handler_read_rnd_next	41
 select 'No key lookups, seq reads: 29= 5 reads from t2 + 4 * 6 reads from t1.' Z;
 Z
 No key lookups, seq reads: 29= 5 reads from t2 + 4 * 6 reads from t1.
diff --git a/mysql-test/r/subselect3_jcl6.result b/mysql-test/r/subselect3_jcl6.result
index 4660cd60603..815a8985d44 100644
--- a/mysql-test/r/subselect3_jcl6.result
+++ b/mysql-test/r/subselect3_jcl6.result
@@ -136,7 +136,7 @@ Handler_read_next	0
 Handler_read_prev	0
 Handler_read_rnd	0
 Handler_read_rnd_deleted	0
-Handler_read_rnd_next	50
+Handler_read_rnd_next	41
 select 'No key lookups, seq reads: 29= 5 reads from t2 + 4 * 6 reads from t1.' Z;
 Z
 No key lookups, seq reads: 29= 5 reads from t2 + 4 * 6 reads from t1.
diff --git a/sql/item_subselect.cc b/sql/item_subselect.cc
index e26c3a47912..04151ebbdb2 100644
--- a/sql/item_subselect.cc
+++ b/sql/item_subselect.cc
@@ -49,7 +49,7 @@ Item_subselect::Item_subselect():
   used_tables_cache(0), have_to_be_excluded(0), const_item_cache(1),
   inside_first_fix_fields(0), done_first_fix_fields(FALSE), 
   expr_cache(0), forced_const(FALSE), substitution(0), engine(0), eliminated(FALSE),
-  engine_changed(0), changed(0), is_correlated(FALSE)
+  changed(0), is_correlated(FALSE)
 {
   DBUG_ENTER("Item_subselect::Item_subselect");
   DBUG_PRINT("enter", ("this: 0x%lx", (ulong) this));
@@ -623,6 +623,8 @@ bool Item_subselect::walk(Item_processor processor, bool walk_subquery,
 
 bool Item_subselect::exec()
 {
+  subselect_engine *org_engine= engine;
+
   DBUG_ENTER("Item_subselect::exec");
 
   /*
@@ -644,11 +646,14 @@ bool Item_subselect::exec()
 #ifndef DBUG_OFF
   ++exec_counter;
 #endif
-  if (engine_changed)
+  if (engine != org_engine)
   {
-    engine_changed= 0;
-    res= exec();
-    DBUG_RETURN(res);
+    /*
+      If the subquery engine changed during execution due to lazy subquery
+      optimization, or because the original engine found a more efficient other
+      engine, re-execute the subquery with the new engine.
+    */
+    DBUG_RETURN(exec());
   }
   DBUG_RETURN(res);
 }
@@ -3141,10 +3146,8 @@ int subselect_single_select_engine::exec()
           DBUG_RETURN(1);                        /* purecov: inspected */
       }
     }
-    if (item->engine_changed)
-    {
+    if (item->engine_changed(this))
       DBUG_RETURN(1);
-    }
   }
   if (select_lex->uncacheable &&
       select_lex->uncacheable != UNCACHEABLE_EXPLAIN
@@ -3318,13 +3321,13 @@ bool subselect_uniquesubquery_engine::copy_ref_key(bool skip_constants)
 
   for (store_key **copy= tab->ref.key_copy ; *copy ; copy++)
   {
-    enum store_key::store_key_result store_res;
+    bool store_res;
     if (skip_constants && (*copy)->store_key_is_const())
       continue;
     store_res= (*copy)->copy();
     tab->ref.key_err= store_res;
 
-    if (store_res == store_key::STORE_KEY_FATAL)
+    if (store_res)
     {
       /*
        Error converting the left IN operand to the column type of the right
diff --git a/sql/item_subselect.h b/sql/item_subselect.h
index 2a64c63a1be..1da129380e7 100644
--- a/sql/item_subselect.h
+++ b/sql/item_subselect.h
@@ -122,8 +122,6 @@ public:
   */
   bool eliminated;
   
-  /* changed engine indicator */
-  bool engine_changed;
   /* subquery is transformed */
   bool changed;
 
@@ -200,9 +198,9 @@ public:
   {
     old_engine= engine;
     engine= eng;
-    engine_changed= 1;
     return eng == 0;
   }
+  bool engine_changed(subselect_engine *eng) { return engine != eng; }
   /*
     True if this subquery has been already evaluated. Implemented only for
     single select and union subqueries only.
@@ -260,7 +258,6 @@ public:
                                              st_select_lex*, st_select_lex*,
                                              Field*, Item*, Item_ident*);
   friend bool convert_join_subqueries_to_semijoins(JOIN *join);
-
 };
 
 /* single value subselect */
diff --git a/sql/sql_select.h b/sql/sql_select.h
index 118a684ab62..f465b08e910 100644
--- a/sql/sql_select.h
+++ b/sql/sql_select.h
@@ -1459,7 +1459,6 @@ class store_key :public Sql_alloc
 {
 public:
   bool null_key; /* TRUE <=> the value of the key has a null part */
-  enum store_key_result { STORE_KEY_OK, STORE_KEY_FATAL, STORE_KEY_CONV };
   enum Type { FIELD_STORE_KEY, ITEM_STORE_KEY, CONST_ITEM_STORE_KEY };
   store_key(THD *thd, Field *field_arg, uchar *ptr, uchar *null, uint length)
     :null_key(0), null_ptr(null), err(0)
@@ -1496,9 +1495,9 @@ public:
     @details this function makes sure truncation warnings when preparing the
     key buffers don't end up as errors (because of an enclosing INSERT/UPDATE).
   */
-  enum store_key_result copy()
+  bool copy()
   {
-    enum store_key_result result;
+    bool result;
     THD *thd= to_field->table->in_use;
     enum_check_fields saved_count_cuted_fields= thd->count_cuted_fields;
     ulonglong sql_mode= thd->variables.sql_mode;
@@ -1520,7 +1519,7 @@ public:
   uchar *null_ptr;
   uchar err;
 
-  virtual enum store_key_result copy_inner()=0;
+  virtual bool copy_inner()=0;
 };
 
 
@@ -1552,7 +1551,7 @@ class store_key_field: public store_key
   }
 
  protected: 
-  enum store_key_result copy_inner()
+  bool copy_inner()
   {
     TABLE *table= copy_field.to_field->table;
     my_bitmap_map *old_map= dbug_tmp_use_all_columns(table,
@@ -1569,7 +1568,7 @@ class store_key_field: public store_key
     copy_field.do_copy(&copy_field);
     dbug_tmp_restore_column_map(table->write_set, old_map);
     null_key= to_field->is_null();
-    return err != 0 ? STORE_KEY_FATAL : STORE_KEY_OK;
+    return test(err);
   }
 };
 
@@ -1599,12 +1598,12 @@ public:
   const char *name() const { return "func"; }
 
  protected:  
-  enum store_key_result copy_inner()
+  bool copy_inner()
   {
     TABLE *table= to_field->table;
     my_bitmap_map *old_map= dbug_tmp_use_all_columns(table,
                                                      table->write_set);
-    int res= FALSE;
+    int res= 0;
 
     /* 
       It looks like the next statement is needed only for a simplified
@@ -1623,11 +1622,10 @@ public:
      we need to check for errors executing it and react accordingly
     */
     if (!res && table->in_use->is_error())
-      res= 1; /* STORE_KEY_FATAL */
+      res= 1;
     dbug_tmp_restore_column_map(table->write_set, old_map);
     null_key= to_field->is_null() || item->null_value;
-    return ((err != 0 || res < 0 || res > 2) ? STORE_KEY_FATAL : 
-            (store_key_result) res);
+    return ((err != 0 || res < 0 || res > 2) ? true : test(res));
   }
 };
 
@@ -1653,7 +1651,7 @@ public:
   bool store_key_is_const() { return true; }
 
 protected:  
-  enum store_key_result copy_inner()
+  bool copy_inner()
   {
     int res;
     if (!inited)
@@ -1665,18 +1663,18 @@ protected:
       if ((res= item->save_in_field(to_field, 1)))
       {       
         if (!err)
-          err= res < 0 ? 1 : res; /* 1=STORE_KEY_FATAL */
+          err= res < 0 ? 1 : res;
       }
       /*
         Item::save_in_field() may call Item::val_xxx(). And if this is a subquery
         we need to check for errors executing it and react accordingly
         */
       if (!err && to_field->table->in_use->is_error())
-        err= 1; /* STORE_KEY_FATAL */
+        err= 1;
       dbug_tmp_restore_column_map(table->write_set, old_map);
     }
     null_key= to_field->is_null() || item->null_value;
-    return (err > 2 ? STORE_KEY_FATAL : (store_key_result) err);
+    return (err > 2 ? true : test(err));
   }
 };