Hi Marc,
thank you for your suggestions. I followed them, except of the read-only
versus global error message. Please see below.
Marc Alff, 17.11.2008 20:13:
...
> Ingo Struewing wrote:
...
>> 2751 Ingo Struewing 2008-10-04
>> Bug#28234 - global/session scope - documentation vs implementation
>> Several system variables did not behave like system
>> variables should do.
>> When trying to SET them or use them in SELECT, they were
>> reported as
>> "unknown system variable". But they appeared in SHOW VARIABLES.
>> This has been fixed by removing the "fixed_vars" array of
>> variables
>> and integrating the variables into the normal system variables
>> chain.
>> All of these variables do now behave as read-only global-only
>> variables. Trying to SET them tells they are read-only, trying to
>> SELECT the session value tells they are global only. Selecting the
>> global value works. It delivers the same value as SHOW VARIABLES.
...
>> +SET @@session.log_slave_updates= true;
>> +ERROR HY000: Variable 'log_slave_updates' is a read only variable
>
> I don't agree here.
> This error message is confusing, since by saying that the server
> implicitly agreed that a variable named "@@session.log_slave_updates"
> does exist, which is not the case.
>
> I would much rather prefer the following error instead:
> ERROR HY000: Variable 'log_slave_updates' is a GLOBAL variable
>
> I did not look at the code to fix this, I assume it's only a matter of
> changing the order of tests.
Yes, it is trivial to change the order of tests in set_var::check(). But
I don't want to do it:
1. The full message for the global variable is: "Variable ... is a
GLOBAL variable and should be set with SET GLOBAL". The user would then
try to set it globally and receive the "read only" message. I think this
is confusing too.
2. Changing the test order would not only require to update a couple of
test cases, but might also affect user's scripts. It is kind of a
behavioral change. It is too late to do it in 5.1.
...
>> @@ -1762,7 +1835,7 @@ Item *sys_var::item(THD *thd, enum_var_t
>> pthread_mutex_lock(&LOCK_global_system_variables);
>> value= *(uint*) value_ptr(thd, var_type, base);
>> pthread_mutex_unlock(&LOCK_global_system_variables);
>> - return new Item_uint((ulonglong) value);
>> + DBUG_RETURN(new Item_uint((ulonglong) value));
>
> Thanks for adding DBUG code here, but please don't do that.
>
> Instead:
> result= new Item_uint((ulonglong) value)
> DBUG_RETURN(result)
>
> It makes debugging step by step much easier, for the same binary anyway.
I agree, but have to note that my proposal was based on normal MySQL style.
Anyway, I found a way to improve it even more. Every branch of the
switch had its own return. I changed them all to end with result= and
return the result after the switch.
...
> Overall comments:
...
> 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
You probably mean sys_var::item(), where I don't need to add SHOW_FUNC?
The new patch: http://lists.mysql.com/commits/59410
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
