Hi, Sergey! On Mar 03, Sergey Vojtovich wrote:
On Tue, Mar 03, 2015 at 03:38:31PM +0100, Sergei Golubchik wrote:
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".
Great. I only wanted to know whether I should bug you about design choices or study MySQL implementation and try to guess from there.
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.
Separate patch is ok. My old implementation in 6ba12f used a small change to LF_HASH that was sufficient for waiting threads code. But if I had a proper initializer in LF_HASH I'd probably have used it, instead of coming with hacks like decreasing LF_HASH::element_size. It is kind of unobvious, so I'd prefer to get rid of it.
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.
Okay, that means it's stable.
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.
MDL_lock has two constructors. One sets m_strategy to 0, the other to &m_scoped_lock_strategy.
Otherwise looks ok. Mostly a straightforward change. Should I take it as ok to push? :)
Yes, please. Regards, Sergei