List:Internals« Previous MessageNext Message »
From:Sergey Vojtovich Date:March 23 2009 12:37pm
Subject:Re: Patch for MYISAM to mmap keys
View as plain text  
Hi Mark,

please accept my congratulations with signing SCA and excuses for not
responding for such a long time. Your contribution looks like a great
feature and I believe it'll be really useful in some scenarios.

I was assigned to review this patch and expect it'll take a few
review cycles until we will end up with a "good to push" patch.

Ok, now a few issues that were identified during quick overview of the
patch...

First of all we need to put your patch in conformance with MySQL
Internals Coding Guidelines, as described at:
http://forge.mysql.com/wiki/MySQL_Internals_Coding_Guidelines

The top three rules are:
- for identation use two spaces (tabs are disallowed);
- open braces should be on a separate line (except for "switch");
- for assigning values to variables we use var= val (this makes easier
  to search for assignments using grep tool);

The second point is that for most system calls we use mysys wrappers.
These wrappers mainly guarantee portability. In our case we should use
these wrappers for mapping/unmapping index file. See also mysys/my_mmap.c.

We need to extend the patch, so this feature can be easily
disabled/enabled. I'd suggest to add myisam_use_mmap_for_index variable
for this purpose (for an example see how myisam_use_mmap is implemented).

That's all for now. Please let me know if you have any
concerns/questions.

Thanks,
Sergey
-- 
Sergey Vojtovich <svoj@stripped>
MySQL AB, Software Engineer
Izhevsk, Russia, www.mysql.com
Thread
Patch for MYISAM to mmap keysMark Robson2 Dec
  • Re: Patch for MYISAM to mmap keysMasood Mortazavi20 Mar
    • Re: Patch for MYISAM to mmap keysMARK CALLAGHAN20 Mar
    • scope of SCA? (was: Re: Patch for MYISAM to mmap keys)Kristian Nielsen20 Mar
      • Re: scope of SCA? (was: Re: Patch for MYISAM to mmap keys)Masood Mortazavi21 Mar
Re: Patch for MYISAM to mmap keysSergey Vojtovich23 Mar
Re: Patch for MYISAM to mmap keysSergey Vojtovich24 Mar