New subject: [Maria-developers] bzr commit into Mariadb 5.2, with Maria 2.0:maria/5.2 branch (igor:2742) WL#85

9 Mar 2010

      Hi!
...
...
...
...
...
"Igor" == Igor Babaev <igor@askmonty.org> writes:
Igor> Please review this patch that is based on the contribution patch
Igor> attached to WL#86.

<cut>

First some things we discussed in Iceland:

Functions with starts with s_.... should be changed to simple_....

Move select.test to suite/select and run it with different keycache
parameters (to not repeat select.result)

Igor>  2742 Igor Babaev	2010-02-16
Igor>       WL#86: Partitioned key cache for MyISAM.
Igor>       This is the base patch for the task.

Igor> === modified file 'mysys/mf_keycache.c'

Igor> +#define KEYCACHE_BASE_EXPR(f, pos)
Igor>        \
Igor> +  ((ulong) ((pos) / keycache->key_cache_block_size) +	 (ulong) (f))
Igor>  #define KEYCACHE_HASH(f, pos)
Igor>        \
Igor> -(((ulong) ((pos) / keycache->key_cache_block_size) +
Igor>        \
Igor> -                                     (ulong) (f)) &
Igor> (keycache->hash_entries-1))
Igor> +  ((KEYCACHE_BASE_EXPR(f, pos) / keycache->hash_factor) &
Igor>        \
Igor> +      (keycache->hash_entries-1))
Igor>  #define FILE_HASH(f)                 ((uint) (f) & (CHANGED_BLOCKS_HASH-1))

As all the divisiors are powers of two, would it be more efficent to store
the shift factor in 'keycache' and do shifts instead of / ?
(I assume that even on modern CPU:s shift is much faster then /)

Igor> +static
Igor> +void s_finish_resize_key_cache(void *keycache_cb,
Igor> +                               my_bool with_resize_queue,
Igor> +                               my_bool acquire_lock)
Igor> +{
Igor> +  S_KEY_CACHE_CB *keycache= (S_KEY_CACHE_CB *) keycache_cb;
Igor> +  DBUG_ENTER("s_finish_resize_key_cache");
Igor> +
Igor> +  if (acquire_lock)
Igor> +    keycache_pthread_mutex_lock(&keycache->cache_lock);
Igor> +

Add after this:

safe_mutex_assert_owner((&keycache->cache_lock);

<cut>

Igor> +static
Igor> +int s_resize_key_cache(void *keycache_cb, uint key_cache_block_size,
Igor> +		       size_t use_mem, uint division_limit,
Igor> +		       uint age_threshold)
Igor> +{
Igor> +  S_KEY_CACHE_CB *keycache= (S_KEY_CACHE_CB *) keycache_cb;

It may be better if this function would take S_KEY_CACHE_CB as a parameter.
This can easily be done by doing a cast when storing the function pointer
into the st_key_cache_funcs array.

This way all casts are in one place and all real functions can avoid
casts.  The same thing applies for all other interface functions.
This mainly makes the real code look a bit nicer

Igor> +  int blocks= 0;
Igor> +  DBUG_ENTER("s_resize_key_cache");
Igor> +
Igor> +  if (!keycache->key_cache_inited)
Igor> +    DBUG_RETURN(keycache->disk_blocks);

Shouldn't we return 0 if cache was not initied ?
(if not inited, disk_blocks may contain random data)

Igor> @@ -670,7 +894,7 @@ finish:
Igor>  /*
Igor>    Increment counter blocking resize key cache operation
Igor>  */
Igor> -static inline void inc_counter_for_resize_op(KEY_CACHE *keycache)
Igor> +static inline void inc_counter_for_resize_op(S_KEY_CACHE_CB *keycache)
Igor>  {
Igor>    keycache->cnt_for_resize_op++;
Igor>  }
Igor> @@ -680,35 +904,49 @@ static inline void inc_counter_for_resiz
Igor>    Decrement counter blocking resize key cache operation;
Igor>    Signal the operation to proceed when counter becomes equal zero
Igor>  */
Igor> -static inline void dec_counter_for_resize_op(KEY_CACHE *keycache)
Igor> +static inline void dec_counter_for_resize_op(S_KEY_CACHE_CB *keycache)

Many changes are just like the one above.

Wouldn't it been easier to keep KEY_CACHE as the orignal value
and add KEY_CACHE_MULTI as the new one ?

(This would make future merger with the MySQL code easier too)

<cut>

Igor> +
Igor> +/*
Igor> +  Get statistics for a simple key cache
Igor> +
Igor> +  SYNOPSIS
Igor> +    get_key_cache_statistics()
Igor> +    keycache_cb         pointer to the control block of a simple key cache
Igor> +    partition_no        partition number (not used)
Igor> +    key_cache_stats OUT pointer to the structure for the returned
Igor> statistics
Igor> +
Igor> +  DESCRIPTION
Igor> +    This function is the implementation of the get_key_cache_statistics
Igor> +    interface function that is employed by simple (non-partitioned) key
Igor> caches.
Igor> +    The function considers the parameter keycache_cb as a pointer to the
Igor> +    control block structure of the type S_KEY_CACHE_CB for a simple key
Igor> cache.
Igor> +    This function returns the statistical data for the key cache.
Igor> +    The parameter partition_no is not used by this function.
Igor> +
Igor> +  RETURN
Igor> +    none
Igor> +
Igor> +*/

A small thing, but better point them out (to ensure we agree on the
coding style):

Remove the extra empty line after 'none'.

The current style is:

No empty lines at end of comments
One empty line between function comment and function.
Two empty lines before function comment (to separate it from previous function)

Igor> +
Igor> +static
Igor> +void s_get_key_cache_statistics(void *keycache_cb,
Igor> +                                uint partition_no __attribute__((unused)),
Igor> +                                KEY_CACHE_STATISTICS *key_cache_stats)
Igor> +{
Igor> +  S_KEY_CACHE_CB *keycache= (S_KEY_CACHE_CB *) keycache_cb;
Igor> +  DBUG_ENTER("s_get_key_cache_statistics");
Igor> +
Igor> +  key_cache_stats->mem_size= (longlong) keycache->key_cache_mem_size;
Igor> +  key_cache_stats->block_size= (longlong) keycache->key_cache_block_size;
Igor> +  key_cache_stats->blocks_used= keycache->blocks_used;
Igor> +  key_cache_stats->blocks_unused= keycache->blocks_unused;
Igor> +  key_cache_stats->blocks_changed= keycache->global_blocks_changed;
Igor> +  key_cache_stats->read_requests= keycache->global_cache_r_requests;
Igor> +  key_cache_stats->reads= keycache->global_cache_read;
Igor> +  key_cache_stats->write_requests= keycache->global_cache_w_requests;
Igor> +  key_cache_stats->writes= keycache->global_cache_write;
Igor> +  DBUG_VOID_RETURN;
Igor> +}

Please ensure that all keycache variables are of the same style.
Use either keycache or key_cache everywhere!
(See above for confusing example)

Igor> +
Igor> +
Igor> +static size_t s_key_cache_stat_var_offsets[]=
Igor> +{
Igor> +  offsetof(S_KEY_CACHE_CB, blocks_used),
Igor> +  offsetof(S_KEY_CACHE_CB, blocks_unused),
Igor> +  offsetof(S_KEY_CACHE_CB, global_blocks_changed),
Igor> +  offsetof(S_KEY_CACHE_CB, global_cache_w_requests),
Igor> +  offsetof(S_KEY_CACHE_CB, global_cache_write),
Igor> +  offsetof(S_KEY_CACHE_CB, global_cache_r_requests),
Igor> +  offsetof(S_KEY_CACHE_CB, global_cache_read)
Igor> +};
Igor> +
Igor> +
Igor> +/*
Igor> +  Get the value of a statistical variable for a simple key cache
Igor> +
Igor> +  SYNOPSIS
Igor> +    s_get_key_cache_stat_value()
Igor> +    keycache_cb         pointer to the control block of a simple key cache
Igor> +    var_no              the ordered number of a statistical variable
Igor> +
Igor> +  DESCRIPTION
Igor> +    This function is the implementation of the s_get_key_cache_stat_value
Igor> +    interface function that is employed by simple (non-partitioned) key
Igor> caches.
Igor> +    The function considers the parameter keycache_cb as a pointer to the
Igor> +    control block structure of the type S_KEY_CACHE_CB for a simple key
Igor> cache.
Igor> +    This function returns the value of the statistical variable var_no
Igor> +    for this key cache. The variables are numbered starting from 0 to 6.
Igor> +
Igor> +  RETURN
Igor> +    The value of the specified statistical variable
Igor> +
Igor> +*/
Igor> +
Igor> +static
Igor> +ulonglong s_get_key_cache_stat_value(void *keycache_cb, uint var_no)
Igor> +{
Igor> +  S_KEY_CACHE_CB *keycache= (S_KEY_CACHE_CB *) keycache_cb;
Igor> +  size_t var_ofs= s_key_cache_stat_var_offsets[var_no];
Igor> +  ulonglong res= 0;
Igor> +  DBUG_ENTER("s_get_key_cache_stat_value");
Igor> +
Igor> +  if (var_no < 3)

Change the above to a define that is in the header file and add a
comment to where s_key_cache_stat_var_offsets are defined).
(Otherwise someone will surely break things sooner or later!)

Igor> +    res= (ulonglong) (*(long *) ((char *) keycache + var_ofs));
Igor> +  else
Igor> +    res= *(ulonglong *) ((char *) keycache + var_ofs);
Igor> +
Igor> +  DBUG_RETURN(res);
Igor> +}
Igor> +
Igor> +
Igor> +/*
Igor> +  The array of pointer to the key cache interface functions used for simple
Igor> +  key caches. Any simple key cache objects including those incorporated
Igor> into
Igor> +  partitioned keys caches exploit this array.
Igor> +
Igor> +  The current implementation of these functions allows to call them from
Igor> +  the MySQL server code directly. We don't do it though.
Igor> +*/
Igor> +
Igor> +static KEY_CACHE_FUNCS s_key_cache_funcs =
Igor> +{
Igor> +  s_init_key_cache,
Igor> +  s_resize_key_cache,
Igor> +  s_change_key_cache_param,
Igor> +  s_key_cache_read,
Igor> +  s_key_cache_insert,
Igor> +  s_key_cache_write,
Igor> +  s_flush_key_blocks,
Igor> +  s_reset_key_cache_counters,
Igor> +  s_end_key_cache,
Igor> +  s_get_key_cache_statistics,
Igor> +  s_get_key_cache_stat_value
Igor> +};

Here is where you can add casts so that you don't need them in the functions.

<cut>

Igor> +/* Control block for a partitioned key cache */
Igor> +
Igor> +typedef struct st_p_key_cache_cb
Igor> +{
Igor> +  my_bool key_cache_inited;     /*<=> control block is allocated
Igor>      */
Igor> +  S_KEY_CACHE_CB **partition_array; /* array of the key cache
Igor> partitions    */
Igor> +  uint partitions;              /* number of partitions in the key
Igor> cache    */
Igor> +  size_t key_cache_mem_size;    /* specified size of the cache memory
Igor>      */
Igor> +  uint key_cache_block_size;    /* size of the page buffer of a cache
Igor> block */
Igor> +} P_KEY_CACHE_CB;

Please move the uint values after each other (gives better alignment)

Igor> +
Igor> +static
Igor> +void p_end_key_cache(void *keycache_cb, my_bool cleanup);

Move the above declaration to start of file (together with all other
static declarations)

<cut>

Igor> +
Igor> +static
Igor> +S_KEY_CACHE_CB *get_key_cache_partition_for_write(P_KEY_CACHE_CB
Igor> *keycache,
Igor> +                                                  File file, my_off_t
Igor> filepos,
Igor> +                                                  ulonglong*
Igor> dirty_part_map)

Put a new line after 'S_KEY_CACHE_CB *';  this makes the line more readable.

Igor> +    If keycache->key_cache_inited != 0 then we assume that the memory for
Igor> +    the array of partitions has been already allocated.
Igor> +
Igor> +    It's assumed that no two threads call this function simultaneously
Igor> +    referring to the same key cache handle.
Igor> +*/

I don't think we can assume that any memory is initialized to 0.
Better to have an argument that tells if the memory is already
initialized or not.

Igor> +
Igor> +static
Igor> +int p_init_key_cache(void *keycache_cb, uint key_cache_block_size,
Igor> +                     size_t use_mem, uint division_limit,
Igor> +                     uint age_threshold)
Igor> +{
Igor> +  int i;
Igor> +  size_t mem_per_cache;
Igor> +  int cnt;
Igor> +  S_KEY_CACHE_CB *partition;
Igor> +  S_KEY_CACHE_CB **partition_ptr;
Igor> +  P_KEY_CACHE_CB *keycache= (P_KEY_CACHE_CB *) keycache_cb;
Igor> +  uint partitions= keycache->partitions;

It would be better if partitions would be part of the init call, as
otherwise the caller need to initialize the keycache->partitions
before the call, which is more cumbersome.

Igor> +  int blocks= -1;
Igor> +  DBUG_ENTER("p_init_key_cache");
Igor> +
Igor> +  keycache->key_cache_block_size = key_cache_block_size;
Igor> +
Igor> +  if (keycache->key_cache_inited)

Remove above test;  We can't make a decisions bases on a struct that we are
about to initialize.

Igor> +    partition_ptr= keycache->partition_array;
Igor> +  else
Igor> +  {
Igor> +    if(!(partition_ptr=
Igor> +       (S_KEY_CACHE_CB **) my_malloc(sizeof(S_KEY_CACHE_CB *) * partitions,
Igor> +                                     MYF(0))))
Igor> +      DBUG_RETURN(blocks);

MYF(0) -> MYF(MY_WME)  (So that we get a proper error message)
Please return -1 instead of blocks (easier to understand).

Igor> +    keycache->partition_array= partition_ptr;
Igor> +  }
Igor> +
Igor> +  mem_per_cache = use_mem / partitions;
Igor> +
Igor> +  for (i= 0; i < (int) partitions; i++)
Igor> +  {
Igor> +    my_bool key_cache_inited= keycache->key_cache_inited;
Igor> +    if (key_cache_inited)
Igor> +      partition= *partition_ptr;
Igor> +    else
Igor> +    {
Igor> +      if (!(partition= (S_KEY_CACHE_CB *)
Igor> my_malloc(sizeof(S_KEY_CACHE_CB),
Igor> +						     MYF(0))))
Igor> +        continue;

If we fail, shouldn't we abort ?
After all, we can't assume we will be able to allocate any other
partition after this one.
Here we should probably use MYF(MY_WME) too.

Igor> +      partition->key_cache_inited= 0;
Igor> +    }
Igor> +
Igor> +    if ((cnt= s_init_key_cache(partition,
Igor> +                                key_cache_block_size, mem_per_cache,
Igor> +                                division_limit, age_threshold)) <= 0)
Igor> +    {
Igor> +      s_end_key_cache(partition, 1);
Igor> +      my_free((uchar *) partition,  MYF(0));

my_free() don't need a cast.

Igor> +      partition= 0;
Igor> +      if (key_cache_inited)
Igor> +      {
Igor> +        memmove(partition_ptr, partition_ptr+1,
Igor> +                sizeof(partition_ptr)*(partitions-i-1));
Igor> +      }
Igor> +      if (i == 0)
Igor> +      {
Igor> +        i--;
Igor> +        partitions--;
Igor> +        if (partitions)
Igor> +          mem_per_cache = use_mem / partitions;
Igor> +      }
Igor> +      continue;
Igor> +    }

I don't like the above code to try to recalculate the memory size:

- You uncrease the size of the next partitions, but the total will
  not be use_mem anyway, so it's a bit pointless.
- It will make the caches of different sizes, which is probably not
what the user wants anyway.

I think it would be better to be able to just fail if we can't
allocate all memory.
- Less confusing for the user
- If we get a memory error here, something is probably very wrong anyway so
  it's better to release memory and abort.
- Simpler code

Igor> +    if (blocks < 0)
Igor> +      blocks= 0;

Whey the special logic with the first loop ?
Please add a commment or just set block to 0 instead of -1 and
set block to -1 on error / at abort

<cut>

Igor> +static
Igor> +int p_resize_key_cache(void *keycache_cb, uint key_cache_block_size,
Igor> +		       size_t use_mem, uint division_limit,
Igor> +		       uint age_threshold)
Igor> +{
Igor> +  uint i;
Igor> +  P_KEY_CACHE_CB *keycache= (P_KEY_CACHE_CB *) keycache_cb;
Igor> +  uint partitions= keycache->partitions;
Igor> +  my_bool cleanup= use_mem == 0;
Igor> +  int blocks= -1;
Igor> +  int err= 0;
Igor> +  DBUG_ENTER("p_resize_key_cache");
Igor> +  if (use_mem == 0)

Change to test if (cleanup)

Igor> +  {
Igor> +    p_end_key_cache(keycache_cb, 0);
Igor> +    DBUG_RETURN(blocks);

change to return -1

Igor> +  }
Igor> +  for (i= 0; i < partitions; i++)
Igor> +  {
Igor> +    err|= s_prepare_resize_key_cache(keycache->partition_array[i], 0, 1);
Igor> +  }
Igor> +  if (!err && use_mem)

You don't need to test use_mem here (you know it's not 0)

Igor> +    blocks= p_init_key_cache(keycache_cb, key_cache_block_size, use_mem,
Igor> +                             division_limit, age_threshold);
Igor> +  if (blocks > 0 && !cleanup)

Cleanup is always 0 here (as otherwise we return -1 at function start
functabove)

Igor> +  {
Igor> +    for (i= 0; i < partitions; i++)
Igor> +    {
Igor> +      s_finish_resize_key_cache(keycache->partition_array[i], 0, 1);
Igor> +    }
Igor> +  }
Igor> +  DBUG_RETURN(blocks);
Igor> +}

<cut>

Igor> +static
Igor> +void p_end_key_cache(void *keycache_cb, my_bool cleanup)
Igor> +{
Igor> +  uint i;
Igor> +  P_KEY_CACHE_CB *keycache= (P_KEY_CACHE_CB *) keycache_cb;
Igor> +  uint partitions= keycache->partitions;
Igor> +  DBUG_ENTER("p_end_key_cache");
Igor> +  DBUG_PRINT("enter", ("key_cache: 0x%lx", (long) keycache));
Igor> +
Igor> +  for (i= 0; i < partitions; i++)
Igor> +  {
Igor> +    s_end_key_cache(keycache->partition_array[i], cleanup);
Igor> +  }
Igor> +  if (cleanup) {

Move { to next line

Igor> +    for (i= 0; i < partitions; i++)
Igor> +      my_free((uchar*) keycache->partition_array[i], MYF(0));
Igor> +    my_free((uchar*) keycache->partition_array, MYF(0));
Igor> +    keycache->key_cache_inited= 0;
Igor> +  }
Igor> +  DBUG_VOID_RETURN;
Igor> +}

<cut>

Offset variable can be used (it's only set).

Igor> +static
Igor> +int p_key_cache_insert(void *keycache_cb,
Igor> +                       File file, my_off_t filepos, int level,
Igor> +                       uchar *buff, uint length)
Igor> +{
Igor> +  uint w_length;
Igor> +  P_KEY_CACHE_CB *keycache= (P_KEY_CACHE_CB *) keycache_cb;
Igor> +  uint offset= (uint) (filepos % keycache->key_cache_block_size);
Igor> +  DBUG_ENTER("p_key_cache_insert");
Igor> +  DBUG_PRINT("enter", ("fd: %u  pos: %lu  length: %u",
Igor> +               (uint) file,(ulong) filepos, length));
Igor> +
Igor> +
Igor> +  /* Write data in key_cache_block_size increments */
Igor> +  do
Igor> +  {
Igor> +    S_KEY_CACHE_CB *partition= get_key_cache_partition(keycache,
Igor> +                                                       file, filepos);
Igor> +    w_length= length;
Igor> +    set_if_smaller(w_length, keycache->key_cache_block_size);

Should be:
   set_if_smaller(w_length, keycache->key_cache_block_size - offset);

Igor> +    if (s_key_cache_insert((void *) partition,
Igor> +                             file, filepos, level,
Igor> +                             buff, w_length))
Igor> +      DBUG_RETURN(1);
Igor> +

<cut>

Igor> +static
Igor> +int p_key_cache_write(void *keycache_cb,
Igor> +                      File file, void *file_extra,
Igor> +                      my_off_t filepos, int level,
Igor> +                      uchar *buff, uint length,
Igor> +                      uint block_length  __attribute__((unused)),
Igor> +                      int dont_write)
Igor> +{
Igor> +  uint w_length;
Igor> +  ulonglong *part_map= (ulonglong *) file_extra;
Igor> +  P_KEY_CACHE_CB *keycache= (P_KEY_CACHE_CB *) keycache_cb;
Igor> +  uint offset= (uint) (filepos % keycache->key_cache_block_size);
Igor> +  DBUG_ENTER("p_key_cache_write");
Igor> +  DBUG_PRINT("enter",
Igor> +             ("fd: %u  pos: %lu  length: %u  block_length: %u"
Igor> +              "  key_block_length: %u",
Igor> +              (uint) file, (ulong) filepos, length, block_length,
Igor> +              keycache ? keycache->key_cache_block_size : 0));
Igor> +
Igor> +
Igor> +  /* Write data in key_cache_block_size increments */
Igor> +  do
Igor> +  {
Igor> +    S_KEY_CACHE_CB *partition= get_key_cache_partition_for_write(keycache,
Igor> +                                                                 file,
Igor> filepos,
Igor> +                                                                 part_map);
Igor> +    w_length = length;
Igor> +    set_if_smaller(w_length, keycache->key_cache_block_size );

Should be:
   set_if_smaller(w_length, keycache->key_cache_block_size - offset);

Igor> +    if (s_key_cache_write(partition,
Igor> +                          file, 0, filepos, level,
Igor> +                          buff, w_length, block_length,
Igor> +                          dont_write))
Igor> +      DBUG_RETURN(1);

<cut>

Igor> +static
Igor> +int p_flush_key_blocks(void *keycache_cb,
Igor> +                       File file, void *file_extra,
Igor> +                       enum flush_type type)
Igor> +{
Igor> +  uint i;
Igor> +  P_KEY_CACHE_CB *keycache= (P_KEY_CACHE_CB *) keycache_cb;
Igor> +  uint partitions= keycache->partitions;
Igor> +  int err= 0;
Igor> +  ulonglong *dirty_part_map= (ulonglong *) file_extra;
Igor> +  DBUG_ENTER("p_flush_key_blocks");
Igor> +  DBUG_PRINT("enter", ("keycache: 0x%lx", (long) keycache));
Igor> +
Igor> +  for (i= 0; i < partitions; i++)
Igor> +  {
Igor> +    S_KEY_CACHE_CB *partition= keycache->partition_array[i];
Igor> +    if ((type == FLUSH_KEEP || type == FLUSH_FORCE_WRITE) &&
Igor> +        !((*dirty_part_map) & (1<<i)))

Change 1<<i to ((ulonglong) 1 << i)

Igor> +      continue;
Igor> +    err+= test(s_flush_key_blocks(partition, file, 0, type));

You can change the + to |, and then remove the err > 0 below.

Igor> +  }
Igor> +  *dirty_part_map= 0;
Igor> +
Igor> +  if (err > 0)
Igor> +    err= 1;
Igor> +
Igor> +  DBUG_RETURN(err);
Igor> +}

<cut>

Igor> +static
Igor> +void p_get_key_cache_statistics(void *keycache_cb, uint partition_no,
Igor> +                                KEY_CACHE_STATISTICS *key_cache_stats)
Igor> +{
Igor> +  uint i;
Igor> +  S_KEY_CACHE_CB *partition;
Igor> +  P_KEY_CACHE_CB *keycache= (P_KEY_CACHE_CB *) keycache_cb;
Igor> +  uint partitions= keycache->partitions;
Igor> +  DBUG_ENTER("p_get_key_cache_statistics_");
Igor> +
Igor> +  if (partition_no != 0)
Igor> +  {
Igor> +    partition= keycache->partition_array[partition_no-1];
Igor> +    s_get_key_cache_statistics((void *) partition, 0, key_cache_stats);
Igor> +    DBUG_VOID_RETURN;
Igor> +  }
Igor> +  key_cache_stats->mem_size= (longlong) keycache->key_cache_mem_size;
Igor> +  key_cache_stats->block_size= (longlong) keycache->key_cache_block_size;
Igor> +  for (i = 0; i < partitions; i++)
Igor> +  {
Igor> +    partition= keycache->partition_array[i];
Igor> +    key_cache_stats->blocks_used+= partition->blocks_used;
Igor> +    key_cache_stats->blocks_unused+= partition->blocks_unused;
Igor> +    key_cache_stats->blocks_changed+= partition->global_blocks_changed;
Igor> +    key_cache_stats->read_requests+= partition->global_cache_r_requests;
Igor> +    key_cache_stats->reads+= partition->global_cache_read;
Igor> +    key_cache_stats->write_requests+= partition->global_cache_w_requests;
Igor> +    key_cache_stats->writes+= partition->global_cache_write;
Igor> +  }

Shouldn't you start by reseting key_cache_stats ?
I think it's better to do the reset here than in
get_key_cache_statistics()

As you don't need the reset when you call s_get_key_cache_statistics()
(as this overwrites all stats entries)

Igor> +  DBUG_VOID_RETURN;
Igor> +}

<cut>

Igor> +static
Igor> +ulonglong p_get_key_cache_stat_value(void *keycache_cb, uint var_no)
Igor> +{
Igor> +  uint i;
Igor> +  P_KEY_CACHE_CB *keycache= (P_KEY_CACHE_CB *) keycache_cb;
Igor> +  uint partitions= keycache->partitions;
Igor> +  size_t var_ofs= s_key_cache_stat_var_offsets[var_no];
Igor> +  ulonglong res= 0;
Igor> +  DBUG_ENTER("p_get_key_cache_stat_value");
Igor> +
Igor> +  if (var_no < 3)

Change to define (see earlier comment)

<cut>

Igor> +int init_key_cache(KEY_CACHE *keycache, uint key_cache_block_size,
Igor> +		   size_t use_mem, uint division_limit,
Igor> +		   uint age_threshold, uint partitions)
Igor> +{
Igor> +  void *keycache_cb;
Igor> +  int blocks;
Igor> +  if (keycache->key_cache_inited)
Igor> +    keycache_cb= keycache->keycache_cb;

See earlier comment about making this a parameter.

<cut>

Igor> +int resize_key_cache(KEY_CACHE *keycache, uint key_cache_block_size,
Igor> +		     size_t use_mem, uint division_limit, uint age_threshold)
Igor> +{
Igor> +  int blocks= -1;
Igor> +  if (keycache->key_cache_inited)
Igor> +  {
Igor> +    if ((uint) keycache->param_partitions != keycache->partitions &&
Igor> use_mem)
Igor> +      blocks= repartition_key_cache (keycache,
Igor> +                                     key_cache_block_size, use_mem,
Igor> +                                     division_limit, age_threshold,
Igor> +                                     (uint) keycache->param_partitions);

Remove space after function name

Igor> +    else
Igor> +    {
Igor> +      blocks= keycache->interface_funcs->resize(keycache->keycache_cb,
Igor> +                                                key_cache_block_size,
Igor> +                                                use_mem, division_limit,
Igor> +                                                age_threshold);
Igor> +
Igor> +      if (keycache->partitions)
Igor> +        keycache->partitions=
Igor> +          ((P_KEY_CACHE_CB *)(keycache->keycache_cb))->partitions;
Igor> +    }
Igor> +    if (blocks <= 0)
Igor> +      keycache->can_be_used= 0;

The last should probably be:

  keycache->can_be_used= (blocks >= 0);

Safer, as it ensures that can_be_used is always correct.

Igor> +uchar *key_cache_read(KEY_CACHE *keycache,
Igor> +                      File file, my_off_t filepos, int level,
Igor> +                      uchar *buff, uint length,
Igor> +		      uint block_length, int return_buffer)
Igor> +{
Igor> +  if (keycache->key_cache_inited && keycache->can_be_used)

Could we change this to only depend on keycache->can_be_used ?

(Faster if)

We can of course assume that keycache_init() has been called for the object.

Igor> +    return keycache->interface_funcs->read(keycache->keycache_cb,
Igor> +                                           file, filepos, level,
Igor> +                                           buff, length,
Igor> +                                           block_length, return_buffer);
Igor> +
Igor> +  /* We can't use mutex here as the key cache may not be initialized */
Igor> +  keycache->global_cache_r_requests++;
Igor> +  keycache->global_cache_read++;
Igor> +
Igor> +  if (my_pread(file, (uchar*) buff, length, filepos, MYF(MY_NABP)))
Igor> +    return (uchar *) 0;
Igor> +
Igor> +  return buff;
Igor> +}

<cut>

Igor> +int key_cache_insert(KEY_CACHE *keycache,
Igor> +                     File file, my_off_t filepos, int level,
Igor> +                     uchar *buff, uint length)
Igor> +{
Igor> +  if (keycache->key_cache_inited && keycache->can_be_used)

Same here as above.

Igor> +    return keycache->interface_funcs->insert(keycache->keycache_cb,
Igor> +                                             file, filepos, level,
Igor> +                                             buff, length);
Igor> +  return 0;
Igor> +}
Igor> +

<cut>

Igor> +int key_cache_write(KEY_CACHE *keycache,
Igor> +                    File file, void *file_extra,
Igor> +                    my_off_t filepos, int level,
Igor> +                    uchar *buff, uint length,
Igor> +		    uint block_length, int force_write)
Igor> +{
Igor> +  if (keycache->key_cache_inited && keycache->can_be_used)

And here.

Igor> +void get_key_cache_statistics(KEY_CACHE *keycache, uint partition_no,
Igor> +                              KEY_CACHE_STATISTICS *key_cache_stats)
Igor> +{
Igor> +  bzero(key_cache_stats, sizeof(KEY_CACHE_STATISTICS));

Remove the above; Better to assume that get_stats() overwrites this.
(Which is the case for s_get_key_cache_statistics())

<cut>

Igor> === modified file 'sql/handler.cc'
Igor> --- a/sql/handler.cc	2010-02-01 06:14:12 +0000

Igor> +int ha_repartition_key_cache(KEY_CACHE *key_cache)
Igor> +{
Igor> +  DBUG_ENTER("ha_repartition_key_cache");
Igor> +
Igor> +  if (key_cache->key_cache_inited)
Igor> +  {
Igor> +    pthread_mutex_lock(&LOCK_global_system_variables);
Igor> +    size_t tmp_buff_size= (size_t) key_cache->param_buff_size;
Igor> +    long tmp_block_size= (long) key_cache->param_block_size;
Igor> +    uint division_limit= key_cache->param_division_limit;
Igor> +    uint age_threshold=  key_cache->param_age_threshold;
Igor> +    uint partitions= key_cache->param_partitions;
Igor> +    pthread_mutex_unlock(&LOCK_global_system_variables);
Igor> +    DBUG_RETURN(!repartition_key_cache(key_cache, tmp_block_size,
Igor> +				       tmp_buff_size,
Igor> +				       division_limit, age_threshold,
Igor> +                                       partitions));
Igor> +  }

key_cache_inited may be 0 if we used a 0 size key cache.
In this case one can't reparttion this cache. Is this correct?
Assume on does the following:

- Set key cache size to 0
- changes partition numbers
- set key cache to 1G

In this case, will things work and the new key cache have the new
number of partitions ?
(It may work, just wanted to be sure that you have tested this)

<cut>
Igor> === modified file 'sql/sql_show.cc'
Igor> --- a/sql/sql_show.cc	2010-02-01 06:14:12 +0000
Igor> +++ b/sql/sql_show.cc	2010-02-16 16:41:11 +0000
Igor> @@ -2220,6 +2220,31 @@ void remove_status_vars(SHOW_VAR *list)
Igor>  }

Igor> +
Igor> +static void update_key_cache_stat_var(KEY_CACHE *key_cache, size_t ofs)
Igor> +{
Igor> +  uint var_no;
Igor> +  switch (ofs) {
Igor> +  case offsetof(KEY_CACHE, blocks_used):
Igor> +  case offsetof(KEY_CACHE, blocks_unused):
Igor> +  case offsetof(KEY_CACHE, global_blocks_changed):
Igor> +    var_no= (ofs-offsetof(KEY_CACHE, blocks_used))/sizeof(ulong);
Igor> +    *(ulong *)((char *) key_cache + ofs)=
Igor> +      (ulong) get_key_cache_stat_value(key_cache, var_no);
Igor> +    break;
Igor> +  case offsetof(KEY_CACHE, global_cache_r_requests):
Igor> +  case offsetof(KEY_CACHE, global_cache_read):
Igor> +  case offsetof(KEY_CACHE, global_cache_w_requests):
Igor> +  case offsetof(KEY_CACHE, global_cache_write):
Igor> +    var_no= 3+(ofs-offsetof(KEY_CACHE, global_cache_w_requests))/
Igor> +              sizeof(ulonglong);

Change 3 to constant.

Igor> +    *(ulonglong *)((char *) key_cache + ofs)=
Igor> +      get_key_cache_stat_value(key_cache, var_no);
Igor> +    break;
Igor> +  }
Igor> +}

<cut>

Igor> +static
Igor> +int store_key_cache_table_record(THD *thd, TABLE *table,
Igor> +                                 const char *name, uint name_length,
Igor> +                                 KEY_CACHE *key_cache,
Igor> +                                 uint partitions, uint partition_no)
Igor> +{
Igor> +  KEY_CACHE_STATISTICS key_cache_stats;
Igor> +  uint err;
Igor> +  DBUG_ENTER("store_key_cache_table_record");
Igor> +
Igor> +  get_key_cache_statistics(key_cache, partition_no, &key_cache_stats);
Igor> +
Igor> +  if (key_cache_stats.mem_size == 0)
Igor> +    DBUG_RETURN(0);

Shouldn't we also here test for keycache->key_cache_inited ?

Regards,
Monty

Re: [Maria-developers] bzr commit into Mariadb 5.2, with Maria 2.0:maria/5.2 branch (igor:2742) WL#86

Michael Widenius

Igor Babaev

Sergei Golubchik

tags

participants (3)