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