maria-developers team mailing list archive

[Sergey Vojtovich <svoj@xxxxxxxxxxx>]

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