Ingo, hello.
Thanks for dragging my attention to Marc's suggestion!
I am sorry I managed to miss it.
> Hi Andrei,
>
> do you agree with Marc regarding "slave_skip_errors"?
> I do, but you are the replication guru, so better have an ok from you too.
...
>> Overall comments:
>>
>> 1) Removing the fixed_var array : excellent
>>
>> 2) Adding a sys_var_const class : excellent.
>>
>> There is some debate in the review thread about how to design this class
>> in details, and about usage of C++ inheritance to save 1 line of code.
>>
>> My take on this is:
>> this debate makes the assumption that the overall design of mysql system
>> variables is already perfect, and discusses how the general C++ class
>> hierarchy can be modeled.
>>
>> My personal opinion is that some major refactoring of the server system
>> variables is needed anyway, for other reasons, because there are still
>> many design flaws there.
>> For example, see Bug#27508.
>>
>> This fix already does some major cleanup by removing the fixed_var array
>> and integrating "fixed system variables", and is good enought for the
>> problem reported.
>>
(to Marc: I did not want to make any variable to be of class
sys_var_read_only, but rather make the new class sys_var_const to
inherit form it).
>> I am not saying whether sys_var_read_only is a good or bad idea, it may
>> very well appear later, but I think it's premature to discuss it, and
>> it's not needed for this bug.
>>
>> 3) About the "slave_skip_errors" system variable.
>>
>> If I understand correctly, "slave_skip_errors" is set once for all when
>> the server starts, so it really is a "constant", not a "read-only"
>> variable.
agreed.
>>
>> As a result, "slave_skip_errors" is implemented with sys_var_const,
>> that's correct.
sure.
>>
>> Now, for a constant system variable, which is by definition frozen for
>> the entire server life time, I think using SHOW_FUNC is a bad idea.
>> If it's that constant, the value should be computed once, and stored in
>> a global variable, instead of being recalculated at every request.
This is better than that I previously suggested.
>>
>> Requested changes:
>>
>> a) remove the function show_slave_skip_errors() entirely.
>> b) implement a global variable to hold the content of the
>> "slave_skip_errors" system variable
>> c) move the logic of show_slave_skip_errors() in
>> init_slave_skip_errors(), to initialize this global variable once.
>>
>> With that, support for SHOW_FUNC for sys_var_const is not needed
>> anymore, and support for SHOW_FUNC should in fact never be needed,
>> otherwise the system variable won't really be a "constant" one.
>>
>> This should simplify greatly the implementation of:
>> - sys_var::check
>> - show_status_array
>> so that the special code added for SHOW_FUNC only for
>> "slave_skip_errors" can be removed.
>
cheers,
Andrei
