Hi! Here is the review:

...

...

...

...

"sanja" == sanja writes:

<cut> sanja> Fix problem of making on-disk cache table. sanja> Removed 'uniques=1' which lead to creating unneeded unique index creation. <cut> ---------------------------------------------------------------------- === modified file 'sql/sql_expression_cache.cc' --- a/sql/sql_expression_cache.cc 2010-11-02 09:47:36 +0000 +++ b/sql/sql_expression_cache.cc 2011-01-05 19:37:56 +0000 <cut> @@ -123,7 +126,6 @@ void Expression_cache_tmptable::init() goto error; } cache_table->s->keys= 1; - cache_table->s->uniques= 1; ref.null_rejecting= 1; ref.disable_cache= FALSE; ref.has_record= 0; The above should now already be in 5.3 tree as part of Sergei's push of my patch). (Just for your information) @@ -195,10 +197,12 @@ Expression_cache::result Expression_cach if (res) { subquery_cache_miss++; + miss++; DBUG_RETURN(MISS); } subquery_cache_hit++; + hit++; *value= cached_result; DBUG_RETURN(Expression_cache::HIT); As subquery_cache_miss and hit are global statistics variables, please use statistic_increment(subquery_cache_miss, &LOCK_status) with these. } @@ -225,7 +229,7 @@ my_bool Expression_cache_tmptable::put_v DBUG_ENTER("Expression_cache_tmptable::put_value"); DBUG_ASSERT(inited); - if (!cache_table) + if (!cache_table || frozen) { Is it not eonugh to test for 'frozen'? (You should set frozen to 0 if cache_table is not set) @@ -239,12 +243,22 @@ my_bool Expression_cache_tmptable::put_v if ((error= cache_table->file->ha_write_row(cache_table->record[0]))) { /* create_myisam_from_heap will generate error if needed */ + /* + TODO: add estimation if ondisk table is efficient if (cache_table->file->is_fatal_error(error, HA_CHECK_DUP) && create_internal_tmp_table_from_heap(table_thd, cache_table, cache_table_param.start_recinfo, &cache_table_param.recinfo, error, 1)) goto err; + */ Please change comment to #ifdef OVERFLOW_CACHE_TO_DISK_BASED_TABLE + if (cache_table->file->is_fatal_error(error, HA_CHECK_DUP)) + goto err; + /* should we leave the table */ + if ((((ulonglong)hit) * 100) / miss < EXPRASSION_TMPTABLE_ALLOWED_HITRATE) What happens if miss is 0 Shouldn't the above be? ((ulonglong) hit*100) / (hit + miss))... + goto err; // remove the temporary table + else Remove else + frozen= TRUE; // leave the temporary table } Try to move the above to before #ifdef and make it in such that one can just define the ifdef if one wants overflow to disk. === modified file 'sql/sql_expression_cache.h' <cut> @@ -50,6 +50,8 @@ class Item_field; class Expression_cache_tmptable :public Expression_cache { + ulong hit, miss; + bool frozen; ulong -> ulonglong (in a join you can have a LOT of row combinations) public: Expression_cache_tmptable(THD *thd, List &dependants, Item *value); virtual ~Expression_cache_tmptable(); Ok to push after above changes. Regards, Monty