maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxxx>]

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@xxxxxxxxxxxx-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