Sergei, I have pushed the updates in the separate commit:
> diff --git a/storage/perfschema/pfs_variable.cc
>> b/storage/perfschema/pfs_variable.cc
>> > index 984b68e699b..85656532c72 100644
>> > --- a/storage/perfschema/pfs_variable.cc
>> > +++ b/storage/perfschema/pfs_variable.cc
>> > @@ -100,6 +102,15 @@ bool
>> PFS_system_variable_cache::init_show_var_array(enum_var_type scope, bool st
>> >
>> > mysql_prlock_unlock(&LOCK_system_variables_hash);
>> >
>> > + for (uint i= 0; i < m_show_var_array.elements(); i++)
>>
>> why separately, not in the loop above?
>>
> I wanted to do ti outside of LOCK_system_variables_hash, only that's why.
>
>>
>> > + {
>> > + sys_var *var= (sys_var *)m_show_var_array.at(i).value;
>> > + if (!var) continue;
>>
>> can var be NULL?
>>
> I have seen this protection elsewhere in PS code, and blindly relied on
> that it's important:)
>
> See for example the loop
> in PFS_system_variable_cache::do_materialize_global.
> They only continue until the value is NULL. Do you have a clue why?
>
>
>> > + sys_var *prev= i == 0 ? NULL : (sys_var *)m_show_var_array.at
>> (i-1).value;
>>
>> hmm, normally this is done with
>>
>> sys_var *prev= NULL;
>>
>> before the loop and
>>
>> prev= var;
>>
>> in the loop, after plugin_lock_by_var_if_different().
>>
>>
> Right, that'll be better. I'll rewrite.
>
>
Since it is already a separate loop, I have extracted it to
the plugin_lock_by_sys_var_array function altogether. The function's
meaning is now much cleaner itself. And besides there's much less
conditions computed in the loop, so it's faster.
> @@ -316,7 +327,27 @@ class PFS_system_variable_cache_apc: public
>> Apc_target::Apc_call
>> >
>> > void call_in_target_thread() override
>> > {
>> > - (m_pfs->*m_func)(m_param);
>> > + call(m_pfs, m_func, m_param);
>> > + }
>> > +public:
>> > + static void call(PFS_system_variable_cache *pfs, Request func, uint
>> param)
>> > + {
>> > + THD *safe_thd= pfs->safe_thd();
>> > +
>> > + if (pfs->query_scope() == OPT_SESSION)
>>
>> what's the point doing apc call if the query_scope() is not OPT_SESSION?
>> It seems like here you need a DBUG_ASSERT,
>> and the if() should be much earlier up the stack.
>>
>
> I just used the same mechanism for global variables as well. I understand
> now it's an overkill. Will simplify!
>
>
I realized i didn't touch global variables traverse, so moving this check
to an assertion was enough
--
Yours truly,
Nikita Malyavin
_______________________________________________
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help : https://help.launchpad.net/ListHelp
