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
