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:i...@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 _______________________________________________ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp