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.
Marc Alff, 17.11.2008 20:13:
>
> Hi Ingo and All
>
> Good work,
> please see some comments below.
...
> 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.
>
> 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.
>
> As a result, "slave_skip_errors" is implemented with sys_var_const,
> that's correct.
>
> 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.
>
> 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.
Regards
Ingo
--
Ingo Strüwing, Database Group
Sun Microsystems GmbH, Sonnenallee 1, D-85551 Kirchheim-Heimstetten
Geschäftsführer: Thomas Schröder, Wolfgang Engels, Dr. Roland Bömer
Vorsitzender des Aufsichtsrates: Martin Häring HRB München 161028
