maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

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