From: Ingo Strüwing
Date: November 18 2008 5:47pm
Subject: Re: bzr commit into mysql-5.1 branch (ingo.struewing:2751) Bug#28234
List-Archive: http://lists.mysql.com/commits/59109
Message-Id: <4922FF99.7080803@sun.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=ISO-8859-1
Content-Transfer-Encoding: 8BIT

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