Magne,
first of all it is a question. Not request, not even a
suggestion.
(heh, nice rhyme)
When I looked at this patch for the first time I wasn't
aware of WL#4305. At that time your patch was the only
viable fix. Now, I think we should consider WL#4305.
The core problem you're fixing is performance bottleneck on
mi_open().
Said the above, what I mean when I say "less intrusive":
- keep list;
- search hash only on open;
- on open: add new hash entry;
- on close: remove hash entry;
- on panic: free hash;
- probably failover to list if hash is broken;
- probably consider hash broken if myisam is used not by the server.
Mine arguments for less intrusive patch are: it should be safer, it
should be easier to remove when we do WL#4305 based solution.
Mine arguments against less intrusive patch are: current patch needs
to be adjusted (probably 1-2 days of coding).
Regards,
Sergey
On Fri, Nov 19, 2010 at 05:52:24PM +0100, Magne Mæhre wrote:
> On 11/19/2010 05:22 PM, Sergey Vojtovich wrote:
> > Magne,
> >
> > ok, do you think it is worth to consider that we may rework
> > this part sooner or later and make less intrusive patch?
>
> Hi Sergey / Mattias
>
> I'm not entirely sure I follow you. What the patch does is
> simply to exchange one data structure with another. Since the
> container classes in mysql doesn't provide a common interface,
> there is a need to change some code. Most of the code change
> is in peripheral functions (mi_panic, debugging, etc.), where
> there is a need to traverse all stored objects. I also had to
> do some extra initialization, since the MyISAM code is also used
> directly from supporting tools, which does not use the
> handlerton.
>
> I feel that this solution is fairly good, as a trade-off
> between speed, complexity and implementation risk. It is
> entirely possible to get an even faster lookup mechanism
> than this, but I feel it's not worth the extra effort. The
> benchmarking we did, show a tremendous(!) improvement over the
> old implementation (see my _private_ entry on the bug report
> page, dated Sep. 30th)
>
> Since WL#4305 isn't, to my knowledge, started on yet, it means
> the impact on that work is minimal. What is absolutely certain
> is that we won't be using the old iterate-and-compare-name-string
> approach in that worklog, so I believe something like my patch
> will naturally find its place in that code.
>
> Of course, I'm open to suggestions on how to improve the code,
> and other approaches. I am, however, reluctant to have code
> living in my sandbox, while waiting for a worklog that has
> unknown priority, and might even be stopped (not that I
> have any indications on that happening :)
>
> So - to sum it up -- I'm, of course, very eager to get this
> patch in. I'm, on the other hand, ready to accept it if it is
> thought to be a bad idea :)
>
> --Magne
--
Sergey Vojtovich <svoj@stripped>
MySQL AB, Software Engineer
Izhevsk, Russia, www.mysql.com
