From b2baeed4a7a19ee498e963fe374cf254d230d2bc Mon Sep 17 00:00:00 2001 From: Tor Didriksen Date: Wed, 28 Oct 2009 11:07:30 +0100 Subject: [PATCH] Bug#48060 Memory leak - Item::val_bool() (item.cc:184) from optimizer_subquery grammar Item_sum::set_aggregator() may be called multiple times during query preparation. On subsequent calls: verify that the aggregator type is the same, and re-use the existing Aggregator. sql/item_sum.cc: In Item_sum::set_aggregator(): re-use existing Aggregator if already set. Remove some friend declarations, add some accessor functions. Cleanup some DBUG_ENTER and DBUG_RETURN code. sql/item_sum.h: Make some member fields private, add accessors instead. Remove some un-necessary friend declarations. Remove some default arguments from constructors. sql/opt_sum.cc: Use accessor functions in Item_sum. sql/sql_select.cc: Fix mis-spelled DBUG_ENTER text. Use accessor functions in Item_sum. sql/sql_yacc.yy: Use explicit true/false rather than default arguments when constructing Item_sum_xxx objects. --- sql/item_sum.cc | 25 +++++++++++++++---------- sql/item_sum.h | 43 +++++++++++++++++++++++-------------------- sql/opt_sum.cc | 12 ++++++------ sql/sql_select.cc | 12 +++++++----- sql/sql_yacc.yy | 4 ++-- 5 files changed, 53 insertions(+), 43 deletions(-) diff --git a/sql/item_sum.cc b/sql/item_sum.cc index c8576722c69..273e996a6a0 100644 --- a/sql/item_sum.cc +++ b/sql/item_sum.cc @@ -564,6 +564,11 @@ Item *Item_sum::set_arg(uint i, THD *thd, Item *new_val) int Item_sum::set_aggregator(Aggregator::Aggregator_type aggregator) { + if (aggr) + { + DBUG_ASSERT(aggregator == aggr->Aggrtype()); + return FALSE; + } switch (aggregator) { case Aggregator::DISTINCT_AGGREGATOR: @@ -736,12 +741,12 @@ bool Aggregator_distinct::setup(THD *thd) if (list.push_back(item)) return TRUE; // End of memory if (item->const_item() && item->is_null()) - always_null=1; + always_null= true; } if (always_null) return FALSE; - count_field_types(select_lex,tmp_table_param,list,0); - tmp_table_param->force_copy_fields= item_sum->force_copy_fields; + count_field_types(select_lex, tmp_table_param, list, 0); + tmp_table_param->force_copy_fields= item_sum->has_force_copy_fields(); DBUG_ASSERT(table == 0); /* Make create_tmp_table() convert BIT columns to BIGINT. @@ -844,10 +849,10 @@ bool Aggregator_distinct::setup(THD *thd) List field_list; Create_field field_def; /* field definition */ Item *arg; - DBUG_ENTER("Item_sum_distinct::setup"); + DBUG_ENTER("Aggregator_distinct::setup"); /* It's legal to call setup() more than once when in a subquery */ if (tree) - return FALSE; + DBUG_RETURN(FALSE); /* Virtual table and the tree are created anew on each re-execution of @@ -855,23 +860,23 @@ bool Aggregator_distinct::setup(THD *thd) mem_root. */ if (field_list.push_back(&field_def)) - return TRUE; + DBUG_RETURN(TRUE); item_sum->null_value= item_sum->maybe_null= 1; item_sum->quick_group= 0; DBUG_ASSERT(item_sum->get_arg(0)->fixed); - arg = item_sum->get_arg(0); + arg= item_sum->get_arg(0); if (arg->const_item()) { (void) arg->val_int(); if (arg->null_value) - always_null=1; + always_null= true; } if (always_null) - return FALSE; + DBUG_RETURN(FALSE); enum enum_field_types field_type; @@ -884,7 +889,7 @@ bool Aggregator_distinct::setup(THD *thd) arg->unsigned_flag); if (! (table= create_virtual_tmp_table(thd, field_list))) - return TRUE; + DBUG_RETURN(TRUE); /* XXX: check that the case of CHAR(0) works OK */ tree_key_length= table->s->reclength - table->s->null_bytes; diff --git a/sql/item_sum.h b/sql/item_sum.h index 62addf4e033..6d7418204fc 100644 --- a/sql/item_sum.h +++ b/sql/item_sum.h @@ -302,13 +302,14 @@ class st_select_lex; class Item_sum :public Item_result_field { -public: +protected: /** Aggregator class instance. Not set initially. Allocated only after it is determined if the incoming data are already distinct. */ Aggregator *aggr; +private: /** Used in making ROLLUP. Set for the ROLLUP copies of the original Item_sum and passed to create_tmp_field() to cause it to work @@ -324,6 +325,11 @@ public: */ bool with_distinct; +public: + + bool has_force_copy_fields() const { return force_copy_fields; } + bool has_with_distinct() const { return with_distinct; } + enum Sumfunctype { COUNT_FUNC, COUNT_DISTINCT_FUNC, SUM_FUNC, SUM_DISTINCT_FUNC, AVG_FUNC, AVG_DISTINCT_FUNC, MIN_FUNC, MAX_FUNC, STD_FUNC, @@ -447,12 +453,12 @@ public: may be initialized to 0 by clear() and to NULL by no_rows_in_result(). */ - void no_rows_in_result() + virtual void no_rows_in_result() { if (!aggr) - set_aggregator(with_distinct ? - Aggregator::DISTINCT_AGGREGATOR : - Aggregator::SIMPLE_AGGREGATOR); + set_aggregator(with_distinct ? + Aggregator::DISTINCT_AGGREGATOR : + Aggregator::SIMPLE_AGGREGATOR); reset(); } virtual void make_unique() { force_copy_fields= TRUE; } @@ -511,11 +517,12 @@ public: */ int set_aggregator(Aggregator::Aggregator_type aggregator); + virtual void clear()= 0; virtual bool add()= 0; - virtual bool setup(THD *thd) {return 0;} + virtual bool setup(THD *thd) { return false; } - void cleanup (); + virtual void cleanup(); }; @@ -533,9 +540,6 @@ class Unique; class Aggregator_distinct : public Aggregator { friend class Item_sum_sum; - friend class Item_sum_count; - friend class Item_sum_avg; -protected: /* flag to prevent consecutive runs of endup(). Normally in endup there are @@ -567,7 +571,7 @@ protected: uint32 *field_lengths; /* - used in conjunction with 'table' to support the access to Field classes + Used in conjunction with 'table' to support the access to Field classes for COUNT(DISTINCT). Needed by copy_fields()/copy_funcs(). */ TMP_TABLE_PARAM *tmp_table_param; @@ -637,7 +641,6 @@ public: class Item_sum_num :public Item_sum { - friend class Aggregator_distinct; protected: /* val_xxx() functions may be called several times during the execution of a @@ -692,14 +695,14 @@ protected: void fix_length_and_dec(); public: - Item_sum_sum(Item *item_par, bool distinct= FALSE) :Item_sum_num(item_par) + Item_sum_sum(Item *item_par, bool distinct) :Item_sum_num(item_par) { set_distinct(distinct); } Item_sum_sum(THD *thd, Item_sum_sum *item); enum Sumfunctype sum_func () const { - return with_distinct ? SUM_DISTINCT_FUNC : SUM_FUNC; + return has_with_distinct() ? SUM_DISTINCT_FUNC : SUM_FUNC; } void clear(); bool add(); @@ -713,7 +716,7 @@ public: void no_rows_in_result() {} const char *func_name() const { - return with_distinct ? "sum(distinct " : "sum("; + return has_with_distinct() ? "sum(distinct " : "sum("; } Item *copy_or_same(THD* thd); }; @@ -752,7 +755,7 @@ class Item_sum_count :public Item_sum_int {} enum Sumfunctype sum_func () const { - return with_distinct ? COUNT_DISTINCT_FUNC : COUNT_FUNC; + return has_with_distinct() ? COUNT_DISTINCT_FUNC : COUNT_FUNC; } void no_rows_in_result() { count=0; } void make_const(longlong count_arg) @@ -765,7 +768,7 @@ class Item_sum_count :public Item_sum_int void update_field(); const char *func_name() const { - return with_distinct ? "count(distinct " : "count("; + return has_with_distinct() ? "count(distinct " : "count("; } Item *copy_or_same(THD* thd); }; @@ -806,7 +809,7 @@ public: uint prec_increment; uint f_precision, f_scale, dec_bin_size; - Item_sum_avg(Item *item_par, bool distinct= FALSE) + Item_sum_avg(Item *item_par, bool distinct) :Item_sum_sum(item_par, distinct), count(0) {} Item_sum_avg(THD *thd, Item_sum_avg *item) @@ -816,7 +819,7 @@ public: void fix_length_and_dec(); enum Sumfunctype sum_func () const { - return with_distinct ? AVG_DISTINCT_FUNC : AVG_FUNC; + return has_with_distinct() ? AVG_DISTINCT_FUNC : AVG_FUNC; } void clear(); bool add(); @@ -832,7 +835,7 @@ public: void no_rows_in_result() {} const char *func_name() const { - return with_distinct ? "avg(distinct " : "avg("; + return has_with_distinct() ? "avg(distinct " : "avg("; } Item *copy_or_same(THD* thd); Field *create_tmp_field(bool group, TABLE *table, uint convert_blob_length); diff --git a/sql/opt_sum.cc b/sql/opt_sum.cc index 9d546b24c7c..13b10ac2e8f 100644 --- a/sql/opt_sum.cc +++ b/sql/opt_sum.cc @@ -356,9 +356,9 @@ int opt_sum_query(TABLE_LIST *tables, List &all_fields,COND *conds) const_result= 0; break; } - item_sum->set_aggregator (item_sum->with_distinct ? - Aggregator::DISTINCT_AGGREGATOR : - Aggregator::SIMPLE_AGGREGATOR); + item_sum->set_aggregator(item_sum->has_with_distinct() ? + Aggregator::DISTINCT_AGGREGATOR : + Aggregator::SIMPLE_AGGREGATOR); if (!count) { /* If count == 0, then we know that is_exact_count == TRUE. */ @@ -447,9 +447,9 @@ int opt_sum_query(TABLE_LIST *tables, List &all_fields,COND *conds) const_result= 0; break; } - item_sum->set_aggregator (item_sum->with_distinct ? - Aggregator::DISTINCT_AGGREGATOR : - Aggregator::SIMPLE_AGGREGATOR); + item_sum->set_aggregator(item_sum->has_with_distinct() ? + Aggregator::DISTINCT_AGGREGATOR : + Aggregator::SIMPLE_AGGREGATOR); if (!count) { /* If count != 1, then we know that is_exact_count == TRUE. */ diff --git a/sql/sql_select.cc b/sql/sql_select.cc index 84a641ec99a..01b21dd7e2d 100644 --- a/sql/sql_select.cc +++ b/sql/sql_select.cc @@ -2094,8 +2094,10 @@ JOIN::exec() if (curr_join->make_sum_func_list(*curr_all_fields, *curr_fields_list, 1, TRUE) || - prepare_sum_aggregators (curr_join->sum_funcs, !curr_join->join_tab || - !curr_join->join_tab->is_using_agg_loose_index_scan()) || + prepare_sum_aggregators(curr_join->sum_funcs, + !curr_join->join_tab || + !curr_join->join_tab-> + is_using_agg_loose_index_scan()) || setup_sum_funcs(curr_join->thd, curr_join->sum_funcs) || thd->is_fatal_error) DBUG_VOID_RETURN; @@ -4008,7 +4010,7 @@ is_indexed_agg_distinct(JOIN *join, List *out_args) join->select_lex->olap == ROLLUP_TYPE) /* Check (B3) for ROLLUP */ return false; - if (join->make_sum_func_list(join->all_fields, join->fields_list, 1)) + if (join->make_sum_func_list(join->all_fields, join->fields_list, true)) return false; for (sum_item_ptr= join->sum_funcs; *sum_item_ptr; sum_item_ptr++) @@ -15477,10 +15479,10 @@ static bool setup_sum_funcs(THD *thd, Item_sum **func_ptr) static bool prepare_sum_aggregators(Item_sum **func_ptr, bool need_distinct) { Item_sum *func; - DBUG_ENTER("setup_sum_funcs"); + DBUG_ENTER("prepare_sum_aggregators"); while ((func= *(func_ptr++))) { - if (func->set_aggregator(need_distinct && func->with_distinct ? + if (func->set_aggregator(need_distinct && func->has_with_distinct() ? Aggregator::DISTINCT_AGGREGATOR : Aggregator::SIMPLE_AGGREGATOR)) DBUG_RETURN(TRUE); diff --git a/sql/sql_yacc.yy b/sql/sql_yacc.yy index e81ebf15af6..60c40ec05d6 100644 --- a/sql/sql_yacc.yy +++ b/sql/sql_yacc.yy @@ -8249,7 +8249,7 @@ udf_expr: sum_expr: AVG_SYM '(' in_sum_expr ')' { - $$= new (YYTHD->mem_root) Item_sum_avg($3); + $$= new (YYTHD->mem_root) Item_sum_avg($3, FALSE); if ($$ == NULL) MYSQL_YYABORT; } @@ -8357,7 +8357,7 @@ sum_expr: } | SUM_SYM '(' in_sum_expr ')' { - $$= new (YYTHD->mem_root) Item_sum_sum($3); + $$= new (YYTHD->mem_root) Item_sum_sum($3, FALSE); if ($$ == NULL) MYSQL_YYABORT; }