List:Internals« Previous MessageNext Message »
From:Sergey Vojtovich Date:March 24 2009 10:17am
Subject:Re: Patch for MYISAM to mmap keys
View as plain text  
Hi Mark,

thanks for the quick response.

On Mon, Mar 23, 2009 at 09:52:51PM +0000, Mark Robson wrote:
> 2009/3/23 Sergey Vojtovich <svoj@stripped>
> 
> > 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...
> >
> 
> Hi Sergey,
> 
> You identified code style issues, but of course I feel that the one that
> Monty identified was far more serious - that the patch is essentially
> unsound because it's not thread-safe.
Sorry, I wasn't aware about this issue.

> Whenever one thread is doing an mremap to extend the file size (concurrent
> insert), a thread simultaneously reading the keys could have the mmap area
> invalidated while it's reading, causing a segfault. While this is not very
> likely, it would require significant and potentially expensive locking to
> avoid.
I tend to disagree that the things are that bad here. The only critical code
should be around enlargement of the index file when concurrent inserts are
enabled. We have a couple of options here:
- disallow concurrent inserts when index mmap is used - the easiest one;
- protect this code by rwlocks - will somewhat decrease concurrent
  inserts performance (should be fine);
- probably postpone remap until some non-critical part of execution of
  concurrent insert - a wild idea, likely difficult to implement and I
  have no good vision on this yet.

Generally I'd suggest to go with the second solution (rwlocks) and have a
look into mi_dynrec.c for an example how concurrent inserts with mmaped
data file are handled.

Regards,
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