maria-developers team mailing list archive
Mailing list archive
Re: MDEV-6089 review
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
> > Otherwise looks ok. Mostly a straightforward change.
> Should I take it as ok to push? :)