maria-developers team mailing list archive
Mailing list archive
Re: MDEV-6089 review
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".
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
> 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
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? :)