Hi, Igor! First, just to make it clear - you asked me to review the "post-review fixes" patch, so that's what I did, I looked at how you implemented Monty comments, I did not review the partitioned key caches feature - it would need much more time - and I don't understand all of it. See below, I have two questions and two comments. Otherwise ok. On Apr 01, Igor Babaev wrote:
Monty,
I addressed almost all problems you pointed to in your review.
The only issue that I did not cover concerns requirement to put select.test in separate suite. People would be screaming if I did it, because this is one of the most touchable tests.
Technically, you could've put it in include (bzr mv t/select.test include/select.inc) it will preserve all history and MySQL updates of this will will be automatically directed to its new location. And write several tests that set different keycache parameters and include inc/select.inc
Here's the fix:
#At lp:maria/5.2 based on revid:igor@askmonty.org-20100331233728-obczp1ecrffnaa2s
2747 Igor Babaev 2010-04-01 Post-review fixes. modified: include/keycache.h mysql-test/r/key_cache.result mysql-test/t/key_cache.test mysys/mf_keycache.c sql/sql_show.cc
=== modified file 'include/keycache.h' --- a/include/keycache.h 2010-02-16 16:41:11 +0000 +++ b/include/keycache.h 2010-04-01 21:42:40 +0000 @@ -53,6 +52,8 @@ typedef struct st_key_cache_statistics ulonglong writes; /* number of actual writes from buffers into files */ } KEY_CACHE_STATISTICS;
+#define NO_LONG_KEY_CACHE_STAT_VARIABLES 3
Try to use "NUM" for "number", "NO" is ambiguous. Like, "what do you mean, no long key cache variables ?"
+ /* The type of a key cache object */ typedef enum key_cache_type { @@ -61,6 +62,55 @@ typedef enum key_cache_type .... +typedef + void (*GET_KEY_CACHE_STATISTICS) + (void *keycache_cb, uint partition_no, + KEY_CACHE_STATISTICS *key_cache_stats); +typedef + ulonglong (*GET_KEY_CACHE_STAT_VALUE) + (void *keycache_cb, uint var_no);
Hmm, you put a lot of declarations in the include/keycache.h - which is a public header, included in other files. But these declarations are only needed in the mf_keycache.c, they are completely internal to the implementation. There is no reason to export them, it only clutters up the namespace and adds unnecessary dependencies.
/* An object of the type KEY_CACHE_FUNCS contains pointers to all functions from the key cache interface. === modified file 'mysql-test/r/key_cache.result' --- a/mysql-test/r/key_cache.result 2010-03-31 23:37:28 +0000 +++ b/mysql-test/r/key_cache.result 2010-04-01 21:42:40 +0000 @@ -672,12 +672,12 @@ insert into t2 values (2000, 3, 'yyyy'); select * from information_schema.key_caches where key_cache_name like "keycache2" and partition_number is null; KEY_CACHE_NAME PARTITIONS PARTITION_NUMBER FULL_SIZE BLOCK_SIZE USED_BLOCKS UNUSED_BLOCKS DIRTY_BLOCKS READ_REQUESTS READS WRITE_REQUESTS WRITES -keycache2 NULL NULL 1048576 1024 0 # 0 0 0 0 0 +keycache2 NULL NULL 1048576 1024 6 # 0 6 6 3 3
how is this relared to "post-review fixes" ?
select * from information_schema.key_caches where key_cache_name like === modified file 'mysys/mf_keycache.c' --- a/mysys/mf_keycache.c 2010-02-16 16:41:11 +0000 +++ b/mysys/mf_keycache.c 2010-04-01 21:42:40 +0000 @@ -5188,36 +5181,35 @@ int p_init_key_cache(void *keycache_cb, .... + if (!--partitions) + break; if (i == 0) { i--; - partitions--; - if (partitions) - mem_per_cache = use_mem / partitions; + mem_per_cache = use_mem / partitions; + continue; } - continue;
I don't understand that part. What is if (i==0) { i--; ? And how this change corresponds to "I changed the code to have all allocated partitions of the same size" ?
}
Regards, Sergei