Hi Sergei, On Tue, Mar 03, 2015 at 03:38:31PM +0100, Sergei Golubchik wrote: > Hi. > > Here're I'll be asking questions about the patches I'm looking at. > > 1. Is this mostly a merge of WL#7305 or mostly your own implementation? > That is, most of the changes and design decisions are yours or > "because MySQL did it"? Implementations differ. All design decisions are mine, like if I were to implement this from scratch. There is nothing like "because MySQL did it".
Though I must admit I studied MySQL implementation and used some of it's comments and names. Things that I didn't like in MySQL implementation were mainly: - changes to lf hash (not very clean) - big workaround for that lf-allocator purgatory bug - huge MDL_lock_strategy - MDL_lock construction/destruction and re-initialization is not very clean either > > 2. Do you work like I do (commit, push, fix failing tests, combine > commits with "git rebase -i" to get a clean history) or you've just > magically got 32bit rdiffs correctly in every commit? :) There was some magic indeed. At least non-embedded rdiffs worked according to bb. No results for embedded though. But yes, normally I'd fix it so that history is kept clean. > > 3. Why "MDL objects cache won't be needed"? The comment said "On some > systems (e.g. Windows XP) constructing/destructing MDL_lock objects > can be fairly expensive. We use this cache to avoid these costs..." > Why is this no longer an issue? Because MDL_lock's are now cached by lf-allocator. > > 4. MDL_lock_strategy - this object is in 5.7 too, but you've done it > very differently. Why? And I wonder why didn't you move the common > part of two strategies to the base class. You wanted a pure interface > class? Probably the main difference is that MySQL implemented it as C-like structure with function pointers, while I did it as C++ classes. MySQL's implementation takes like 10x more lines than mine. What are those common parts? They should have nothing in common. > > 5. Added initializer callback to lf-hash. May be you should remove this > hack with decreasing LF_HASH::element_size etc and move this > functionality to a custom initializer? It was added in 6ba12f. > In particular, see changes to lf* files and whether they can use your > initializer instead. You mean I should fix waiting threads (and probably table cache) to use initializer function instead of decreasing LF_HASH::element_size? I don't mind, but probably in a separate patch. > > 6. Replaced hash with lock-free hash: test result changes. Is the order > of rows stable? perhaps you should use --sort_results ? Order of rows depends on hash implementation. If we'll have to change this again, then it is definitely a good idea to fix it. > > in MDL_lock constructor, I'd set m_strategy to 0 (ori to 1 or, may > be, keep it uninitialized) as a safety measure, to ensure it's not > used before MDL_lock::lf_hash_initializer(). I guess it's for one of intermediate patches. Last version does it. > > Otherwise looks ok. Mostly a straightforward change. Should I take it as ok to push? :) Thanks, Sergey _______________________________________________ Mailing list: https://launchpad.net/~maria-developers Post to : [email protected] Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
