Bug #23184: SELECT causes server crash

 Item::val_xxx() may be called by the server several times at execute time 
 for a single query. Calls to val_xxx() may be very expensive and sometimes
 (count(distinct), sum(distinct), avg(distinct)) not possible.
 To avoid that problem the results of calculation for these aggregate 
 functions are cached so that val_xxx() methods just return the calculated 
 value for the second and subsequent calls.


mysql-test/r/func_group.result:
  Bug #23184: SELECT causes server crash
   - test case
mysql-test/t/func_group.test:
  Bug #23184: SELECT causes server crash
   - test case
sql/item_sum.cc:
  Bug #23184: SELECT causes server crash
   - caching of the aggregate function results so no need to recalculate at val_xxx()
sql/item_sum.h:
  Bug #23184: SELECT causes server crash
   - caching of the aggregate function results so no need to recalculate at val_xxx()

2 files changed, 60 insertions, 21 deletions

diff --git a/sql/item_sum.cc b/sql/item_sum.cc
index 5ca1dbba94b..fccbdfa7925 100644
--- a/sql/item_sum.cc
+++ b/sql/item_sum.cc
@@ -893,6 +893,7 @@ bool Item_sum_distinct::setup(THD *thd)
   tree= new Unique(simple_raw_key_cmp, &tree_key_length, tree_key_length,
                    thd->variables.max_heap_table_size);
 
+  is_evaluated= FALSE;
   DBUG_RETURN(tree == 0);
 }
 
@@ -900,6 +901,7 @@ bool Item_sum_distinct::setup(THD *thd)
 bool Item_sum_distinct::add()
 {
   args[0]->save_in_field(table->field[0], FALSE);
+  is_evaluated= FALSE;
   if (!table->field[0]->is_null())
   {
     DBUG_ASSERT(tree);
@@ -929,6 +931,7 @@ void Item_sum_distinct::clear()
   DBUG_ASSERT(tree != 0);                        /* we always have a tree */
   null_value= 1;
   tree->reset();
+  is_evaluated= FALSE;
   DBUG_VOID_RETURN;
 }
 
@@ -938,6 +941,7 @@ void Item_sum_distinct::cleanup()
   delete tree;
   tree= 0;
   table= 0;
+  is_evaluated= FALSE;
 }
 
 Item_sum_distinct::~Item_sum_distinct()
@@ -949,16 +953,20 @@ Item_sum_distinct::~Item_sum_distinct()
 
 void Item_sum_distinct::calculate_val_and_count()
 {
-  count= 0;
-  val.traits->set_zero(&val);
-  /*
-    We don't have a tree only if 'setup()' hasn't been called;
-    this is the case of sql_select.cc:return_zero_rows.
-  */
-  if (tree)
+  if (!is_evaluated)
   {
-    table->field[0]->set_notnull();
-    tree->walk(item_sum_distinct_walk, (void*) this);
+    count= 0;
+    val.traits->set_zero(&val);
+    /*
+      We don't have a tree only if 'setup()' hasn't been called;
+      this is the case of sql_select.cc:return_zero_rows.
+     */
+    if (tree)
+    {
+      table->field[0]->set_notnull();
+      tree->walk(item_sum_distinct_walk, (void*) this);
+    }
+    is_evaluated= TRUE;
   }
 }
 
@@ -1014,9 +1022,13 @@ Item_sum_avg_distinct::fix_length_and_dec()
 void
 Item_sum_avg_distinct::calculate_val_and_count()
 {
-  Item_sum_distinct::calculate_val_and_count();
-  if (count)
-    val.traits->div(&val, count);
+  if (!is_evaluated)
+  {
+    Item_sum_distinct::calculate_val_and_count();
+    if (count)
+      val.traits->div(&val, count);
+    is_evaluated= TRUE;
+  }
 }
 
 
@@ -2477,6 +2489,7 @@ void Item_sum_count_distinct::cleanup()
     */
     delete tree;
     tree= 0;
+    is_evaluated= FALSE;
     if (table)
     {
       free_tmp_table(table->in_use, table);
@@ -2498,6 +2511,7 @@ void Item_sum_count_distinct::make_unique()
   original= 0;
   force_copy_fields= 1;
   tree= 0;
+  is_evaluated= FALSE;
   tmp_table_param= 0;
   always_null= FALSE;
 }
@@ -2617,6 +2631,7 @@ bool Item_sum_count_distinct::setup(THD *thd)
       but this has to be handled - otherwise someone can crash
       the server with a DoS attack
     */
+    is_evaluated= FALSE;
     if (! tree)
       return TRUE;
   }
@@ -2633,8 +2648,11 @@ Item *Item_sum_count_distinct::copy_or_same(THD* thd)
 void Item_sum_count_distinct::clear()
 {
   /* tree and table can be both null only if always_null */
+  is_evaluated= FALSE;
   if (tree)
+  {
     tree->reset();
+  }
   else if (table)
   {
     table->file->extra(HA_EXTRA_NO_CACHE);
@@ -2655,6 +2673,7 @@ bool Item_sum_count_distinct::add()
     if ((*field)->is_real_null(0))
       return 0;					// Don't count NULL
 
+  is_evaluated= FALSE;
   if (tree)
   {
     /*
@@ -2680,12 +2699,14 @@ longlong Item_sum_count_distinct::val_int()
     return LL(0);
   if (tree)
   {
-    ulonglong count;
+    if (is_evaluated)
+      return count;
 
     if (tree->elements == 0)
       return (longlong) tree->elements_in_tree(); // everything fits in memory
     count= 0;
     tree->walk(count_distinct_walk, (void*) &count);
+    is_evaluated= TRUE;
     return (longlong) count;
   }
   table->file->info(HA_STATUS_VARIABLE | HA_STATUS_NO_LOCK);
diff --git a/sql/item_sum.h b/sql/item_sum.h
index f1ea95214de..a2b2f7cab92 100644
--- a/sql/item_sum.h
+++ b/sql/item_sum.h
@@ -321,12 +321,23 @@ public:
 
 class Item_sum_num :public Item_sum
 {
+protected:
+  /*
+   val_xxx() functions may be called several times during the execution of a 
+   query. Derived classes that require extensive calculation in val_xxx()
+   maintain cache of aggregate value. This variable governs the validity of 
+   that cache.
+  */
+  bool is_evaluated;
 public:
-  Item_sum_num() :Item_sum() {}
-  Item_sum_num(Item *item_par) :Item_sum(item_par) {}
-  Item_sum_num(Item *a, Item* b) :Item_sum(a,b) {}
-  Item_sum_num(List<Item> &list) :Item_sum(list) {}
-  Item_sum_num(THD *thd, Item_sum_num *item) :Item_sum(thd, item) {}
+  Item_sum_num() :Item_sum(),is_evaluated(FALSE) {}
+  Item_sum_num(Item *item_par) 
+    :Item_sum(item_par), is_evaluated(FALSE) {}
+  Item_sum_num(Item *a, Item* b) :Item_sum(a,b),is_evaluated(FALSE) {}
+  Item_sum_num(List<Item> &list) 
+    :Item_sum(list), is_evaluated(FALSE) {}
+  Item_sum_num(THD *thd, Item_sum_num *item) 
+    :Item_sum(thd, item),is_evaluated(item->is_evaluated) {}
   bool fix_fields(THD *, Item **);
   longlong val_int()
   {
@@ -509,6 +520,12 @@ class Item_sum_count_distinct :public Item_sum_int
   */
   Unique *tree;
   /*
+   Storage for the value of count between calls to val_int() so val_int()
+   will not recalculate on each call. Validitiy of the value is stored in
+   is_evaluated.
+  */
+  longlong count;
+  /*
     Following is 0 normal object and pointer to original one for copy 
     (to correctly free resources)
   */
@@ -525,14 +542,15 @@ class Item_sum_count_distinct :public Item_sum_int
 public:
   Item_sum_count_distinct(List<Item> &list)
     :Item_sum_int(list), table(0), field_lengths(0), tmp_table_param(0),
-     force_copy_fields(0), tree(0), original(0), always_null(FALSE)
+     force_copy_fields(0), tree(0), count(0),
+     original(0), always_null(FALSE)
   { quick_group= 0; }
   Item_sum_count_distinct(THD *thd, Item_sum_count_distinct *item)
     :Item_sum_int(thd, item), table(item->table),
      field_lengths(item->field_lengths),
      tmp_table_param(item->tmp_table_param),
-     force_copy_fields(0), tree(item->tree), original(item),
-     tree_key_length(item->tree_key_length),
+     force_copy_fields(0), tree(item->tree), count(item->count), 
+     original(item), tree_key_length(item->tree_key_length),
      always_null(item->always_null)
   {}
   ~Item_sum_count_distinct();