maria-developers team mailing list archive

[Michael Widenius <monty@xxxxxxxxxxxx>]

Hi!

Here is the review:

>>>>> "sanja" == sanja  <sanja@xxxxxxxxxxxx> 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<Item*> &dependants, Item *value);
   virtual ~Expression_cache_tmptable();

Ok to push after above changes.

Regards,
Monty