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