MDEV-3812: Remove unneeded extra call to engine->exec() in...

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.

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

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.

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.

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

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 */

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