Hi Ingo and All
Good work,
please see some comments below.
Regards,
-- Marc
Ingo Struewing wrote:
> #At file:///home/mydev/bzrroot/mysql-5.1-bug28234/
>
> 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.
> modified:
> mysql-test/r/variables-notembedded.result
> mysql-test/r/variables.result
> mysql-test/t/variables-notembedded.test
> mysql-test/t/variables.test
> sql/set_var.cc
> sql/set_var.h
> sql/sql_repl.cc
> sql/sql_show.cc
>
> per-file messages:
> mysql-test/r/variables-notembedded.result
> Bug#28234 - global/session scope - documentation vs implementation
> New test result.
> mysql-test/r/variables.result
> Bug#28234 - global/session scope - documentation vs implementation
> New test result.
> mysql-test/t/variables-notembedded.test
> Bug#28234 - global/session scope - documentation vs implementation
> Added a test for each moved variable that is not present in an
> embedded server.
> mysql-test/t/variables.test
> Bug#28234 - global/session scope - documentation vs implementation
> Added a test for each moved variable that is also present in an
> embedded server.
> sql/set_var.cc
> Bug#28234 - global/session scope - documentation vs implementation
> Moved all variables from the "fixed_vars" array into the normal
> system variables chain by using the new variable class sys_var_const.
> Extended sys_var::item() by SHOW_BOOL and SHOW_FUNC.
> Removed the fixed_show_vars array and its initialization in
> enumerate_sys_vars().
> Removed mysql_append_static_vars(), which added fixed_vars arrays
> to the fixed_show_vars array.
> sql/set_var.h
> Bug#28234 - global/session scope - documentation vs implementation
> Added the new system variable class sys_var_const.
> Removed declaration of mysql_append_static_vars().
> sql/sql_repl.cc
> Bug#28234 - global/session scope - documentation vs implementation
> Moved all variables from the "fixed_vars" array into the normal
> system variables chain by using the new variable class sys_var_const.
> Extended sys_var::item() by SHOW_BOOL and SHOW_FUNC.
> Moved the declaration of show_slave_skip_errors() up to be before
> its new usage.
> Removed the call to mysql_append_static_vars().
> sql/sql_show.cc
> Bug#28234 - global/session scope - documentation vs implementation
> Fixed show_status_array() so that it can handle system variables
> of SHOW_FUNC type.
> === modified file 'mysql-test/r/variables-notembedded.result'
> --- a/mysql-test/r/variables-notembedded.result 2008-02-15 12:54:04 +0000
> +++ b/mysql-test/r/variables-notembedded.result 2008-10-04 19:21:39 +0000
> @@ -15,3 +15,95 @@ slave_skip_errors 3,100,137,643,1752
> ---- Clean Up ----
> set global slave_net_timeout=default;
> set global sql_slave_skip_counter= 0;
> +
> +#
> +SHOW VARIABLES like 'log_slave_updates';
> +Variable_name Value
> +log_slave_updates OFF
Ok
> +SELECT @@session.log_slave_updates;
> +ERROR HY000: Variable 'log_slave_updates' is a GLOBAL variable
Ok
> +SELECT @@global.log_slave_updates;
> +@@global.log_slave_updates
> +0
Ok
> +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.
Please investigate.
> +SET @@global.log_slave_updates= true;
> +ERROR HY000: Variable 'log_slave_updates' is a read only variable
Ok
> +#
(...)
>
> === modified file 'mysql-test/r/variables.result'
> --- a/mysql-test/r/variables.result 2008-03-28 15:10:04 +0000
> +++ b/mysql-test/r/variables.result 2008-10-04 19:21:39 +0000
> @@ -1024,3 +1024,342 @@ SET GLOBAL log_output = 0;
> ERROR 42000: Variable 'log_output' can't be set to the value of '0'
>
> # -- End of Bug#34820.
> +
(...)
>
> === modified file 'mysql-test/t/variables-notembedded.test'
> --- a/mysql-test/t/variables-notembedded.test 2008-02-15 12:54:04 +0000
> +++ b/mysql-test/t/variables-notembedded.test 2008-10-04 19:21:39 +0000
> @@ -28,3 +28,83 @@ set global slave_net_timeout=default;
> # sql_slave_skip_counter is write-only, so we can't save previous
> # value and restore it here. That's ok, because it's normally 0.
> set global sql_slave_skip_counter= 0;
> +
> +#
> +# Bug#28234 - global/session scope - documentation vs implementation
> +#
> +--echo
> +#
> +# Additional variables fixed from sql_repl.cc.
> +#
> +--echo #
> +SHOW VARIABLES like 'log_slave_updates';
> +--error ER_INCORRECT_GLOBAL_LOCAL_VAR
> +SELECT @@session.log_slave_updates;
> +SELECT @@global.log_slave_updates;
> +--error ER_INCORRECT_GLOBAL_LOCAL_VAR
> +SET @@session.log_slave_updates= true;
> +--error ER_INCORRECT_GLOBAL_LOCAL_VAR
> +SET @@global.log_slave_updates= true;
> +#
> +--echo #
> +SHOW VARIABLES like 'relay_log';
> +--error ER_INCORRECT_GLOBAL_LOCAL_VAR
> +SELECT @@session.relay_log;
> +SELECT @@global.relay_log;
> +--error ER_INCORRECT_GLOBAL_LOCAL_VAR
> +SET @@session.relay_log= 'x';
> +--error ER_INCORRECT_GLOBAL_LOCAL_VAR
> +SET @@global.relay_log= 'x';
> +#
> +--echo #
> +SHOW VARIABLES like 'relay_log_index';
> +--error ER_INCORRECT_GLOBAL_LOCAL_VAR
> +SELECT @@session.relay_log_index;
> +SELECT @@global.relay_log_index;
> +--error ER_INCORRECT_GLOBAL_LOCAL_VAR
> +SET @@session.relay_log_index= 'x';
> +--error ER_INCORRECT_GLOBAL_LOCAL_VAR
> +SET @@global.relay_log_index= 'x';
> +#
> +--echo #
> +SHOW VARIABLES like 'relay_log_info_file';
> +--error ER_INCORRECT_GLOBAL_LOCAL_VAR
> +SELECT @@session.relay_log_info_file;
> +SELECT @@global.relay_log_info_file;
> +--error ER_INCORRECT_GLOBAL_LOCAL_VAR
> +SET @@session.relay_log_info_file= 'x';
> +--error ER_INCORRECT_GLOBAL_LOCAL_VAR
> +SET @@global.relay_log_info_file= 'x';
> +#
> +--echo #
> +SHOW VARIABLES like 'relay_log_space_limit';
> +--error ER_INCORRECT_GLOBAL_LOCAL_VAR
> +SELECT @@session.relay_log_space_limit;
> +SELECT @@global.relay_log_space_limit;
> +--error ER_INCORRECT_GLOBAL_LOCAL_VAR
> +SET @@session.relay_log_space_limit= 7;
> +--error ER_INCORRECT_GLOBAL_LOCAL_VAR
> +SET @@global.relay_log_space_limit= 7;
> +#
> +--echo #
> +--replace_column 2 #
> +SHOW VARIABLES like 'slave_load_tmpdir';
> +--error ER_INCORRECT_GLOBAL_LOCAL_VAR
> +SELECT @@session.slave_load_tmpdir;
> +--replace_column 1 #
> +SELECT @@global.slave_load_tmpdir;
> +--error ER_INCORRECT_GLOBAL_LOCAL_VAR
> +SET @@session.slave_load_tmpdir= 'x';
> +--error ER_INCORRECT_GLOBAL_LOCAL_VAR
> +SET @@global.slave_load_tmpdir= 'x';
> +#
> +--echo #
> +SHOW VARIABLES like 'slave_skip_errors';
> +--error ER_INCORRECT_GLOBAL_LOCAL_VAR
> +SELECT @@session.slave_skip_errors;
> +SELECT @@global.slave_skip_errors;
> +--error ER_INCORRECT_GLOBAL_LOCAL_VAR
> +SET @@session.slave_skip_errors= 7;
> +--error ER_INCORRECT_GLOBAL_LOCAL_VAR
> +SET @@global.slave_skip_errors= 7;
> +#
>
> === modified file 'mysql-test/t/variables.test'
> --- a/mysql-test/t/variables.test 2008-03-28 15:10:04 +0000
> +++ b/mysql-test/t/variables.test 2008-10-04 19:21:39 +0000
> @@ -795,3 +795,362 @@ SET GLOBAL log_output = 0;
> --echo
> --echo # -- End of Bug#34820.
>
> +#
> +# Bug#28234 - global/session scope - documentation vs implementation
> +#
> +--echo
> +--echo #
> +SHOW VARIABLES like 'ft_max_word_len';
> +--error ER_INCORRECT_GLOBAL_LOCAL_VAR
> +SELECT @@session.ft_max_word_len;
> +SELECT @@global.ft_max_word_len;
> +--error ER_INCORRECT_GLOBAL_LOCAL_VAR
> +SET @@session.ft_max_word_len= 7;
> +--error ER_INCORRECT_GLOBAL_LOCAL_VAR
> +SET @@global.ft_max_word_len= 7;
> +#
> +--echo #
> +SHOW VARIABLES like 'ft_min_word_len';
> +--error ER_INCORRECT_GLOBAL_LOCAL_VAR
> +SELECT @@session.ft_min_word_len;
> +SELECT @@global.ft_min_word_len;
> +--error ER_INCORRECT_GLOBAL_LOCAL_VAR
> +SET @@session.ft_min_word_len= 7;
> +--error ER_INCORRECT_GLOBAL_LOCAL_VAR
> +SET @@global.ft_min_word_len= 7;
> +#
> +--echo #
> +SHOW VARIABLES like 'ft_query_expansion_limit';
> +--error ER_INCORRECT_GLOBAL_LOCAL_VAR
> +SELECT @@session.ft_query_expansion_limit;
> +SELECT @@global.ft_query_expansion_limit;
> +--error ER_INCORRECT_GLOBAL_LOCAL_VAR
> +SET @@session.ft_query_expansion_limit= 7;
> +--error ER_INCORRECT_GLOBAL_LOCAL_VAR
> +SET @@global.ft_query_expansion_limit= 7;
> +#
> +--echo #
> +SHOW VARIABLES like 'ft_stopword_file';
> +--error ER_INCORRECT_GLOBAL_LOCAL_VAR
> +SELECT @@session.ft_stopword_file;
> +SELECT @@global.ft_stopword_file;
> +--error ER_INCORRECT_GLOBAL_LOCAL_VAR
> +SET @@session.ft_stopword_file= 'x';
> +--error ER_INCORRECT_GLOBAL_LOCAL_VAR
> +SET @@global.ft_stopword_file= 'x';
> +#
> +# Additional variables fixed.
> +#
> +--echo #
> +SHOW VARIABLES like 'back_log';
> +--error ER_INCORRECT_GLOBAL_LOCAL_VAR
> +SELECT @@session.back_log;
> +SELECT @@global.back_log;
> +--error ER_INCORRECT_GLOBAL_LOCAL_VAR
> +SET @@session.back_log= 7;
> +--error ER_INCORRECT_GLOBAL_LOCAL_VAR
> +SET @@global.back_log= 7;
> +#
> +--echo #
> +--replace_column 2 #
> +SHOW VARIABLES like 'large_files_support';
> +--error ER_INCORRECT_GLOBAL_LOCAL_VAR
> +SELECT @@session.large_files_support;
> +--replace_column 1 #
> +SELECT @@global.large_files_support;
> +--error ER_INCORRECT_GLOBAL_LOCAL_VAR
> +SET @@session.large_files_support= true;
> +--error ER_INCORRECT_GLOBAL_LOCAL_VAR
> +SET @@global.large_files_support= true;
> +#
> +--echo #
> +--replace_column 2 #
> +SHOW VARIABLES like 'character_sets_dir';
> +--error ER_INCORRECT_GLOBAL_LOCAL_VAR
> +SELECT @@session.character_sets_dir;
> +--replace_column 1 #
> +SELECT @@global.character_sets_dir;
> +--error ER_INCORRECT_GLOBAL_LOCAL_VAR
> +SET @@session.character_sets_dir= 'x';
> +--error ER_INCORRECT_GLOBAL_LOCAL_VAR
> +SET @@global.character_sets_dir= 'x';
> +#
> +--echo #
> +--replace_column 2 #
> +SHOW VARIABLES like 'init_file';
> +--error ER_INCORRECT_GLOBAL_LOCAL_VAR
> +SELECT @@session.init_file;
> +--replace_column 1 #
> +SELECT @@global.init_file;
> +--error ER_INCORRECT_GLOBAL_LOCAL_VAR
> +SET @@session.init_file= 'x';
> +--error ER_INCORRECT_GLOBAL_LOCAL_VAR
> +SET @@global.init_file= 'x';
> +#
> +--echo #
> +--replace_column 2 #
> +SHOW VARIABLES like 'language';
> +--error ER_INCORRECT_GLOBAL_LOCAL_VAR
> +SELECT @@session.language;
> +--replace_column 1 #
> +SELECT @@global.language;
> +--error ER_INCORRECT_GLOBAL_LOCAL_VAR
> +SET @@session.language= 'x';
> +--error ER_INCORRECT_GLOBAL_LOCAL_VAR
> +SET @@global.language= 'x';
> +#
> +--echo #
> +--replace_column 2 #
> +SHOW VARIABLES like 'large_page_size';
> +--error ER_INCORRECT_GLOBAL_LOCAL_VAR
> +SELECT @@session.large_page_size;
> +--replace_column 1 #
> +SELECT @@global.large_page_size;
> +--error ER_INCORRECT_GLOBAL_LOCAL_VAR
> +SET @@session.large_page_size= 7;
> +--error ER_INCORRECT_GLOBAL_LOCAL_VAR
> +SET @@global.large_page_size= 7;
> +#
> +--echo #
> +--replace_column 2 #
> +SHOW VARIABLES like 'large_pages';
> +--error ER_INCORRECT_GLOBAL_LOCAL_VAR
> +SELECT @@session.large_pages;
> +--replace_column 1 #
> +SELECT @@global.large_pages;
> +--error ER_INCORRECT_GLOBAL_LOCAL_VAR
> +SET @@session.large_pages= true;
> +--error ER_INCORRECT_GLOBAL_LOCAL_VAR
> +SET @@global.large_pages= true;
> +#
> +# Only ifdef HAVE_MLOCKALL
> +#--echo #
> +#--replace_column 2 #
> +#SHOW VARIABLES like 'locked_in_memory';
> +#--error ER_INCORRECT_GLOBAL_LOCAL_VAR
> +#SELECT @@session.locked_in_memory;
> +#--replace_column 1 #
> +#SELECT @@global.locked_in_memory;
> +#--error ER_INCORRECT_GLOBAL_LOCAL_VAR
> +#SET @@session.locked_in_memory= true;
> +#--error ER_INCORRECT_GLOBAL_LOCAL_VAR
> +#SET @@global.locked_in_memory= true;
> +#
I can see why the test is commented, but I would not add commented test
cases in the .test file at all, since this part is very likely to rot
and be unmaintained.
My suggestion is to remove this (and related commented tests).
Up to you.
> +--echo #
> +SHOW VARIABLES like 'log_bin';
> +--error ER_INCORRECT_GLOBAL_LOCAL_VAR
> +SELECT @@session.log_bin;
> +SELECT @@global.log_bin;
> +--error ER_INCORRECT_GLOBAL_LOCAL_VAR
> +SET @@session.log_bin= true;
> +--error ER_INCORRECT_GLOBAL_LOCAL_VAR
> +SET @@global.log_bin= true;
> +#
> +--echo #
> +--replace_column 2 #
> +SHOW VARIABLES like 'log_error';
> +--error ER_INCORRECT_GLOBAL_LOCAL_VAR
> +SELECT @@session.log_error;
> +--replace_column 1 #
> +SELECT @@global.log_error;
> +--error ER_INCORRECT_GLOBAL_LOCAL_VAR
> +SET @@session.log_error= 'x';
> +--error ER_INCORRECT_GLOBAL_LOCAL_VAR
> +SET @@global.log_error= 'x';
> +#
> +--echo #
> +--replace_column 2 #
> +SHOW VARIABLES like 'lower_case_file_system';
> +--error ER_INCORRECT_GLOBAL_LOCAL_VAR
> +SELECT @@session.lower_case_file_system;
> +--replace_column 1 #
> +SELECT @@global.lower_case_file_system;
> +--error ER_INCORRECT_GLOBAL_LOCAL_VAR
> +SET @@session.lower_case_file_system= true;
> +--error ER_INCORRECT_GLOBAL_LOCAL_VAR
> +SET @@global.lower_case_file_system= true;
> +#
> +--echo #
> +--replace_column 2 #
> +SHOW VARIABLES like 'lower_case_table_names';
> +--error ER_INCORRECT_GLOBAL_LOCAL_VAR
> +SELECT @@session.lower_case_table_names;
> +--replace_column 1 #
> +SELECT @@global.lower_case_table_names;
> +--error ER_INCORRECT_GLOBAL_LOCAL_VAR
> +SET @@session.lower_case_table_names= 7;
> +--error ER_INCORRECT_GLOBAL_LOCAL_VAR
> +SET @@global.lower_case_table_names= 7;
> +#
> +--echo #
> +SHOW VARIABLES like 'myisam_recover_options';
> +--error ER_INCORRECT_GLOBAL_LOCAL_VAR
> +SELECT @@session.myisam_recover_options;
> +SELECT @@global.myisam_recover_options;
> +--error ER_INCORRECT_GLOBAL_LOCAL_VAR
> +SET @@session.myisam_recover_options= 'x';
> +--error ER_INCORRECT_GLOBAL_LOCAL_VAR
> +SET @@global.myisam_recover_options= 'x';
> +#
> +# Only ifdef __NT__
> +#--echo #
> +#SHOW VARIABLES like 'named_pipe';
> +#--error ER_INCORRECT_GLOBAL_LOCAL_VAR
> +#SELECT @@session.named_pipe;
> +#SELECT @@global.named_pipe;
> +#--error ER_INCORRECT_GLOBAL_LOCAL_VAR
> +#SET @@session.named_pipe= true;
> +#--error ER_INCORRECT_GLOBAL_LOCAL_VAR
> +#SET @@global.named_pipe= true;
> +#
> +--echo #
> +--replace_column 2 #
> +SHOW VARIABLES like 'open_files_limit';
> +--error ER_INCORRECT_GLOBAL_LOCAL_VAR
> +SELECT @@session.open_files_limit;
> +--replace_column 1 #
> +SELECT @@global.open_files_limit;
> +--error ER_INCORRECT_GLOBAL_LOCAL_VAR
> +SET @@session.open_files_limit= 7;
> +--error ER_INCORRECT_GLOBAL_LOCAL_VAR
> +SET @@global.open_files_limit= 7;
> +#
> +--echo #
> +--replace_column 2 #
> +SHOW VARIABLES like 'pid_file';
> +--error ER_INCORRECT_GLOBAL_LOCAL_VAR
> +SELECT @@session.pid_file;
> +--replace_column 1 #
> +SELECT @@global.pid_file;
> +--error ER_INCORRECT_GLOBAL_LOCAL_VAR
> +SET @@session.pid_file= 'x';
> +--error ER_INCORRECT_GLOBAL_LOCAL_VAR
> +SET @@global.pid_file= 'x';
> +#
> +--echo #
> +--replace_column 2 #
> +SHOW VARIABLES like 'plugin_dir';
> +--error ER_INCORRECT_GLOBAL_LOCAL_VAR
> +SELECT @@session.plugin_dir;
> +--replace_column 1 #
> +SELECT @@global.plugin_dir;
> +--error ER_INCORRECT_GLOBAL_LOCAL_VAR
> +SET @@session.plugin_dir= 'x';
> +--error ER_INCORRECT_GLOBAL_LOCAL_VAR
> +SET @@global.plugin_dir= 'x';
> +#
> +--echo #
> +--replace_column 2 #
> +SHOW VARIABLES like 'port';
> +--error ER_INCORRECT_GLOBAL_LOCAL_VAR
> +SELECT @@session.port;
> +--replace_column 1 #
> +SELECT @@global.port;
> +--error ER_INCORRECT_GLOBAL_LOCAL_VAR
> +SET @@session.port= 7;
> +--error ER_INCORRECT_GLOBAL_LOCAL_VAR
> +SET @@global.port= 7;
> +#
> +--echo #
> +SHOW VARIABLES like 'protocol_version';
> +--error ER_INCORRECT_GLOBAL_LOCAL_VAR
> +SELECT @@session.protocol_version;
> +SELECT @@global.protocol_version;
> +--error ER_INCORRECT_GLOBAL_LOCAL_VAR
> +SET @@session.protocol_version= 7;
> +--error ER_INCORRECT_GLOBAL_LOCAL_VAR
> +SET @@global.protocol_version= 7;
> +#
> +#Only ifdef HAVE_SMEM
> +#--echo #
> +#--replace_column 2 #
> +#SHOW VARIABLES like 'shared_memory';
> +#--error ER_INCORRECT_GLOBAL_LOCAL_VAR
> +#SELECT @@session.shared_memory;
> +#--replace_column 1 #
> +#SELECT @@global.shared_memory;
> +#--error ER_INCORRECT_GLOBAL_LOCAL_VAR
> +#SET @@session.shared_memory= true;
> +#--error ER_INCORRECT_GLOBAL_LOCAL_VAR
> +#SET @@global.shared_memory= true;
> +#
> +#Only ifdef HAVE_SMEM
> +#--echo #
> +#--replace_column 2 #
> +#SHOW VARIABLES like 'shared_memory_base_name';
> +#--error ER_INCORRECT_GLOBAL_LOCAL_VAR
> +#SELECT @@session.shared_memory_base_name;
> +#--replace_column 1 #
> +#SELECT @@global.shared_memory_base_name;
> +#--error ER_INCORRECT_GLOBAL_LOCAL_VAR
> +#SET @@session.shared_memory_base_name= 'x';
> +#--error ER_INCORRECT_GLOBAL_LOCAL_VAR
> +#SET @@global.shared_memory_base_name= 'x';
> +#
> +--echo #
> +SHOW VARIABLES like 'skip_external_locking';
> +--error ER_INCORRECT_GLOBAL_LOCAL_VAR
> +SELECT @@session.skip_external_locking;
> +SELECT @@global.skip_external_locking;
> +--error ER_INCORRECT_GLOBAL_LOCAL_VAR
> +SET @@session.skip_external_locking= true;
> +--error ER_INCORRECT_GLOBAL_LOCAL_VAR
> +SET @@global.skip_external_locking= true;
> +#
> +--echo #
> +SHOW VARIABLES like 'skip_networking';
> +--error ER_INCORRECT_GLOBAL_LOCAL_VAR
> +SELECT @@session.skip_networking;
> +SELECT @@global.skip_networking;
> +--error ER_INCORRECT_GLOBAL_LOCAL_VAR
> +SET @@session.skip_networking= true;
> +--error ER_INCORRECT_GLOBAL_LOCAL_VAR
> +SET @@global.skip_networking= true;
> +#
> +--echo #
> +SHOW VARIABLES like 'skip_show_database';
> +--error ER_INCORRECT_GLOBAL_LOCAL_VAR
> +SELECT @@session.skip_show_database;
> +SELECT @@global.skip_show_database;
> +--error ER_INCORRECT_GLOBAL_LOCAL_VAR
> +SET @@session.skip_show_database= true;
> +--error ER_INCORRECT_GLOBAL_LOCAL_VAR
> +SET @@global.skip_show_database= true;
> +#
> +--echo #
> +--replace_column 2 #
> +SHOW VARIABLES like 'socket';
> +--error ER_INCORRECT_GLOBAL_LOCAL_VAR
> +SELECT @@session.socket;
> +--replace_column 1 #
> +SELECT @@global.socket;
> +--error ER_INCORRECT_GLOBAL_LOCAL_VAR
> +SET @@session.socket= 'x';
> +--error ER_INCORRECT_GLOBAL_LOCAL_VAR
> +SET @@global.socket= 'x';
> +#
> +# Only ifdef HAVE_THR_SETCONCURRENCY
> +#--echo #
> +#--replace_column 2 #
> +#SHOW VARIABLES like 'thread_concurrency';
> +#--error ER_INCORRECT_GLOBAL_LOCAL_VAR
> +#SELECT @@session.thread_concurrency;
> +#--replace_column 1 #
> +#SELECT @@global.thread_concurrency;
> +#--error ER_INCORRECT_GLOBAL_LOCAL_VAR
> +#SET @@session.thread_concurrency= 7;
> +#--error ER_INCORRECT_GLOBAL_LOCAL_VAR
> +#SET @@global.thread_concurrency= 7;
> +#
> +--echo #
> +#--replace_column 2 #
> +SHOW VARIABLES like 'thread_stack';
> +--error ER_INCORRECT_GLOBAL_LOCAL_VAR
> +SELECT @@session.thread_stack;
> +--replace_column 1 #
> +SELECT @@global.thread_stack;
> +--error ER_INCORRECT_GLOBAL_LOCAL_VAR
> +SET @@session.thread_stack= 7;
> +--error ER_INCORRECT_GLOBAL_LOCAL_VAR
> +SET @@global.thread_stack= 7;
> +#
>
> === modified file 'sql/set_var.cc'
> --- a/sql/set_var.cc 2008-09-09 10:19:31 +0000
> +++ b/sql/set_var.cc 2008-10-04 19:21:39 +0000
> @@ -77,7 +77,6 @@ extern ulong ndb_report_thresh_binlog_me
> extern CHARSET_INFO *character_set_filesystem;
>
>
> -static DYNAMIC_ARRAY fixed_show_vars;
Excellent.
> static HASH system_variable_hash;
>
> const char *bool_type_names[]= { "OFF", "ON", NullS };
> @@ -174,6 +173,9 @@ sys_auto_increment_offset(&vars, "auto_i
> static sys_var_bool_ptr sys_automatic_sp_privileges(&vars,
> "automatic_sp_privileges",
> &sp_automatic_privileges);
>
> +static sys_var_const sys_back_log(&vars, "back_log",
> + OPT_GLOBAL, SHOW_LONG,
> + (uchar*) &back_log);
Ok (and ok for all related sys_var_const).
> static sys_var_const_str sys_basedir(&vars, "basedir", mysql_home);
> static sys_var_long_ptr sys_binlog_cache_size(&vars, "binlog_cache_size",
> &binlog_cache_size);
> @@ -181,6 +183,11 @@ static sys_var_thd_binlog_format sys_bin
> &SV::binlog_format);
> static sys_var_thd_ulong sys_bulk_insert_buff_size(&vars,
> "bulk_insert_buffer_size",
> &SV::bulk_insert_buff_size);
> +static sys_var_const sys_character_sets_dir(&vars,
> + "character_sets_dir",
> + OPT_GLOBAL, SHOW_CHAR,
> + (uchar*)
> + mysql_charsets_dir);
> static sys_var_character_set_sv
> sys_character_set_server(&vars, "character_set_server",
> &SV::collation_server, &default_charset_info, 0,
> @@ -249,14 +256,31 @@ static sys_var_long_ptr sys_expire_logs_
> &expire_logs_days);
> static sys_var_bool_ptr sys_flush(&vars, "flush", &myisam_flush);
> static sys_var_long_ptr sys_flush_time(&vars, "flush_time", &flush_time);
> -static sys_var_str sys_ft_boolean_syntax(&vars,
> "ft_boolean_syntax",
> - sys_check_ftb_syntax,
> - sys_update_ftb_syntax,
> - sys_default_ftb_syntax,
> - ft_boolean_syntax);
> +static sys_var_str sys_ft_boolean_syntax(&vars, "ft_boolean_syntax",
> + sys_check_ftb_syntax,
> + sys_update_ftb_syntax,
> + sys_default_ftb_syntax,
> + ft_boolean_syntax);
> +static sys_var_const sys_ft_max_word_len(&vars, "ft_max_word_len",
> + OPT_GLOBAL, SHOW_LONG,
> + (uchar*) &ft_max_word_len);
> +static sys_var_const sys_ft_min_word_len(&vars, "ft_min_word_len",
> + OPT_GLOBAL, SHOW_LONG,
> + (uchar*) &ft_min_word_len);
> +static sys_var_const sys_ft_query_expansion_limit(&vars,
> + "ft_query_expansion_limit",
> + OPT_GLOBAL, SHOW_LONG,
> + (uchar*)
> +
> &ft_query_expansion_limit);
> +static sys_var_const sys_ft_stopword_file(&vars, "ft_stopword_file",
> + OPT_GLOBAL, SHOW_CHAR_PTR,
> + (uchar*) &ft_stopword_file);
> sys_var_str sys_init_connect(&vars, "init_connect", 0,
> sys_update_init_connect,
> sys_default_init_connect,0);
> +static sys_var_const sys_init_file(&vars, "init_file",
> + OPT_GLOBAL, SHOW_CHAR_PTR,
> + (uchar*) &opt_init_file);
> sys_var_str sys_init_slave(&vars, "init_slave", 0,
> sys_update_init_slave,
> sys_default_init_slave,0);
> @@ -274,14 +298,37 @@ static sys_var_key_cache_long sys_key_ca
> static sys_var_key_cache_long sys_key_cache_age_threshold(&vars,
> "key_cache_age_threshold",
> offsetof(KEY_CACHE,
> param_age_threshold));
> +static sys_var_const sys_language(&vars, "language",
> + OPT_GLOBAL, SHOW_CHAR,
> + (uchar*) language);
> +static sys_var_const sys_large_files_support(&vars, "large_files_support",
> + OPT_GLOBAL, SHOW_BOOL,
> + (uchar*) &opt_large_files);
> +static sys_var_const sys_large_page_size(&vars, "large_page_size",
> + OPT_GLOBAL, SHOW_INT,
> + (uchar*) &opt_large_page_size);
> +static sys_var_const sys_large_pages(&vars, "large_pages",
> + OPT_GLOBAL, SHOW_MY_BOOL,
> + (uchar*) &opt_large_pages);
> static sys_var_bool_ptr sys_local_infile(&vars, "local_infile",
> &opt_local_infile);
> +#ifdef HAVE_MLOCKALL
> +static sys_var_const sys_locked_in_memory(&vars, "locked_in_memory",
> + OPT_GLOBAL, SHOW_MY_BOOL,
> + (uchar*) &locked_in_memory);
> +#endif
> +static sys_var_const sys_log_bin(&vars, "log_bin",
> + OPT_GLOBAL, SHOW_BOOL,
> + (uchar*) &opt_bin_log);
> static sys_var_trust_routine_creators
> sys_trust_routine_creators(&vars, "log_bin_trust_routine_creators",
> &trust_function_creators);
> static sys_var_bool_ptr
> sys_trust_function_creators(&vars, "log_bin_trust_function_creators",
> &trust_function_creators);
> +static sys_var_const sys_log_error(&vars, "log_error",
> + OPT_GLOBAL, SHOW_CHAR,
> + (uchar*) log_error_file);
> static sys_var_bool_ptr
> sys_log_queries_not_using_indexes(&vars, "log_queries_not_using_indexes",
> &opt_log_queries_not_using_indexes);
> @@ -296,6 +343,16 @@ static sys_var_thd_bool sys_sql_low_prio
> &SV::low_priority_updates,
> fix_low_priority_updates);
> #endif
> +static sys_var_const sys_lower_case_file_system(&vars,
> + "lower_case_file_system",
> + OPT_GLOBAL, SHOW_MY_BOOL,
> + (uchar*)
> + &lower_case_file_system);
> +static sys_var_const sys_lower_case_table_names(&vars,
> + "lower_case_table_names",
> + OPT_GLOBAL, SHOW_INT,
> + (uchar*)
> + &lower_case_table_names);
> static sys_var_thd_ulong sys_max_allowed_packet(&vars, "max_allowed_packet",
> &SV::max_allowed_packet);
> static sys_var_long_ptr sys_max_binlog_cache_size(&vars,
> "max_binlog_cache_size",
> @@ -359,6 +416,10 @@ static sys_var_thd_ulong sys_multi
> static sys_var_long_ptr sys_myisam_data_pointer_size(&vars,
> "myisam_data_pointer_size",
> &myisam_data_pointer_size);
> static sys_var_thd_ulonglong sys_myisam_max_sort_file_size(&vars,
> "myisam_max_sort_file_size", &SV::myisam_max_sort_file_size,
> fix_myisam_max_sort_file_size, 1);
> +static sys_var_const sys_myisam_recover_options(&vars,
> "myisam_recover_options",
> + OPT_GLOBAL, SHOW_CHAR_PTR,
> + (uchar*)
> + &myisam_recover_options_str);
> static sys_var_thd_ulong sys_myisam_repair_threads(&vars,
> "myisam_repair_threads", &SV::myisam_repair_threads);
> static sys_var_thd_ulong sys_myisam_sort_buffer_size(&vars,
> "myisam_sort_buffer_size", &SV::myisam_sort_buff_size);
> static sys_var_bool_ptr sys_myisam_use_mmap(&vars, "myisam_use_mmap",
> @@ -369,6 +430,13 @@ static sys_var_thd_enum sys_myis
> &myisam_stats_method_typelib,
> NULL);
>
> +#ifdef __NT__
> +/* purecov: begin inspected */
> +static sys_var_const sys_named_pipe(&vars, "named_pipe",
> + OPT_GLOBAL, SHOW_MY_BOOL,
> + (uchar*)
> &opt_enable_named_pipe);
> +/* purecov: end */
> +#endif
> static sys_var_thd_ulong sys_net_buffer_length(&vars, "net_buffer_length",
> &SV::net_buffer_length);
> static sys_var_thd_ulong sys_net_read_timeout(&vars, "net_read_timeout",
> @@ -387,12 +455,29 @@ static sys_var_bool_ptr_readonly sys_old
> sys_var_thd_bool sys_old_alter_table(&vars, "old_alter_table",
> &SV::old_alter_table);
> sys_var_thd_bool sys_old_passwords(&vars, "old_passwords",
> &SV::old_passwords);
> +static sys_var_const sys_open_files_limit(&vars, "open_files_limit",
> + OPT_GLOBAL, SHOW_LONG,
> + (uchar*)
> + &open_files_limit);
> static sys_var_thd_ulong sys_optimizer_prune_level(&vars,
> "optimizer_prune_level",
> &SV::optimizer_prune_level);
> static sys_var_thd_ulong sys_optimizer_search_depth(&vars,
> "optimizer_search_depth",
>
> &SV::optimizer_search_depth);
> +static sys_var_const sys_pid_file(&vars, "pid_file",
> + OPT_GLOBAL, SHOW_CHAR,
> + (uchar*) pidfile_name);
> +static sys_var_const sys_plugin_dir(&vars, "plugin_dir",
> + OPT_GLOBAL, SHOW_CHAR,
> + (uchar*) opt_plugin_dir);
> +static sys_var_const sys_port(&vars, "port",
> + OPT_GLOBAL, SHOW_INT,
> + (uchar*) &mysqld_port);
> static sys_var_thd_ulong sys_preload_buff_size(&vars,
> "preload_buffer_size",
> &SV::preload_buff_size);
> +static sys_var_const sys_protocol_version(&vars, "protocol_version",
> + OPT_GLOBAL, SHOW_INT,
> + (uchar*)
> + &protocol_version);
> static sys_var_thd_ulong sys_read_buff_size(&vars, "read_buffer_size",
> &SV::read_buff_size);
> static sys_var_opt_readonly sys_readonly(&vars, "read_only",
> &opt_readonly);
> @@ -414,6 +499,45 @@ static sys_var_thd_ulong sys_query_alloc
> static sys_var_thd_ulong sys_query_prealloc_size(&vars, "query_prealloc_size",
> &SV::query_prealloc_size,
> 0, fix_thd_mem_root);
> +#ifdef HAVE_SMEM
> +/* purecov: begin tested */
> +static sys_var_const sys_shared_memory(&vars, "shared_memory",
> + OPT_GLOBAL, SHOW_MY_BOOL,
> + (uchar*)
> + &opt_enable_shared_memory);
> +static sys_var_const sys_shared_memory_base_name(&vars,
> + "shared_memory_base_name",
> + OPT_GLOBAL, SHOW_CHAR_PTR,
> + (uchar*)
> + &shared_memory_base_name);
> +/* purecov: end */
> +#endif
> +static sys_var_const sys_skip_external_locking(&vars,
> + "skip_external_locking",
> + OPT_GLOBAL, SHOW_MY_BOOL,
> + (uchar*)
> + &my_disable_locking);
> +static sys_var_const sys_skip_networking(&vars, "skip_networking",
> + OPT_GLOBAL, SHOW_BOOL,
> + (uchar*) &opt_disable_networking);
> +static sys_var_const sys_skip_show_database(&vars, "skip_show_database",
> + OPT_GLOBAL, SHOW_BOOL,
> + (uchar*) &opt_skip_show_db);
> +#ifdef HAVE_SYS_UN_H
> +static sys_var_const sys_socket(&vars, "socket",
> + OPT_GLOBAL, SHOW_CHAR_PTR,
> + (uchar*) &mysqld_unix_port);
> +#endif
> +#ifdef HAVE_THR_SETCONCURRENCY
> +/* purecov: begin tested */
> +static sys_var_const sys_thread_concurrency(&vars, "thread_concurrency",
> + OPT_GLOBAL, SHOW_LONG,
> + (uchar*) &concurrency);
> +/* purecov: end */
> +#endif
> +static sys_var_const sys_thread_stack(&vars, "thread_stack",
> + OPT_GLOBAL, SHOW_LONG,
> + (uchar*) &my_thread_stack_size);
> static sys_var_readonly sys_tmpdir(&vars, "tmpdir", OPT_GLOBAL,
> SHOW_CHAR, get_tmpdir);
> static sys_var_thd_ulong sys_trans_alloc_block_size(&vars,
> "transaction_alloc_block_size",
> &SV::trans_alloc_block_size,
> @@ -764,59 +888,6 @@ static sys_var_log_output sys_var_log_ou
> &log_output_typelib, 0);
>
>
> -/*
> - Additional variables (not derived from sys_var class, not accessible as
> - @@varname in SELECT or SET). Sorted in alphabetical order to facilitate
> - maintenance - SHOW VARIABLES will sort its output.
> - TODO: remove this list completely
> -*/
> -
> -#define FIXED_VARS_SIZE (sizeof(fixed_vars) / sizeof(SHOW_VAR))
> -static SHOW_VAR fixed_vars[]= {
> - {"back_log", (char*) &back_log, SHOW_LONG},
> - {"character_sets_dir", mysql_charsets_dir, SHOW_CHAR},
> - {"ft_max_word_len", (char*) &ft_max_word_len, SHOW_LONG},
> - {"ft_min_word_len", (char*) &ft_min_word_len, SHOW_LONG},
> - {"ft_query_expansion_limit",(char*) &ft_query_expansion_limit, SHOW_LONG},
> - {"ft_stopword_file", (char*) &ft_stopword_file,
> SHOW_CHAR_PTR},
> - {"init_file", (char*) &opt_init_file,
> SHOW_CHAR_PTR},
> - {"language", language, SHOW_CHAR},
> - {"large_files_support", (char*) &opt_large_files, SHOW_BOOL},
> - {"large_page_size", (char*) &opt_large_page_size, SHOW_INT},
> - {"large_pages", (char*) &opt_large_pages,
> SHOW_MY_BOOL},
> -#ifdef HAVE_MLOCKALL
> - {"locked_in_memory", (char*) &locked_in_memory, SHOW_MY_BOOL},
> -#endif
> - {"log_bin", (char*) &opt_bin_log, SHOW_BOOL},
> - {"log_error", (char*) log_error_file, SHOW_CHAR},
> - {"lower_case_file_system", (char*) &lower_case_file_system,
> SHOW_MY_BOOL},
> - {"lower_case_table_names", (char*) &lower_case_table_names, SHOW_INT},
> - {"myisam_recover_options", (char*) &myisam_recover_options_str,
> SHOW_CHAR_PTR},
> -#ifdef __NT__
> - {"named_pipe", (char*) &opt_enable_named_pipe, SHOW_MY_BOOL},
> -#endif
> - {"open_files_limit", (char*) &open_files_limit, SHOW_LONG},
> - {"pid_file", (char*) pidfile_name, SHOW_CHAR},
> - {"plugin_dir", (char*) opt_plugin_dir, SHOW_CHAR},
> - {"port", (char*) &mysqld_port, SHOW_INT},
> - {"protocol_version", (char*) &protocol_version, SHOW_INT},
> -#ifdef HAVE_SMEM
> - {"shared_memory", (char*) &opt_enable_shared_memory,
> SHOW_MY_BOOL},
> - {"shared_memory_base_name", (char*) &shared_memory_base_name,
> SHOW_CHAR_PTR},
> -#endif
> - {"skip_external_locking", (char*) &my_disable_locking,
> SHOW_MY_BOOL},
> - {"skip_networking", (char*) &opt_disable_networking, SHOW_BOOL},
> - {"skip_show_database", (char*) &opt_skip_show_db, SHOW_BOOL},
> -#ifdef HAVE_SYS_UN_H
> - {"socket", (char*) &mysqld_unix_port,
> SHOW_CHAR_PTR},
> -#endif
> -#ifdef HAVE_THR_SETCONCURRENCY
> - {"thread_concurrency", (char*) &concurrency, SHOW_LONG},
> -#endif
> - {"thread_stack", (char*) &my_thread_stack_size, SHOW_LONG},
> -};
> -
> -
Removing fixed_var: totally agree.
> bool sys_var::check(THD *thd, set_var *var)
> {
> var->save_result.ulonglong_value= var->value->val_int();
> @@ -1744,17 +1815,19 @@ err:
>
> Item *sys_var::item(THD *thd, enum_var_type var_type, LEX_STRING *base)
> {
> + DBUG_ENTER("sys_var::item");
Ok
> if (check_type(var_type))
> {
> if (var_type != OPT_DEFAULT)
> {
> my_error(ER_INCORRECT_GLOBAL_LOCAL_VAR, MYF(0),
> name, var_type == OPT_GLOBAL ? "SESSION" : "GLOBAL");
> - return 0;
> + DBUG_RETURN(0);
Ok
> }
> /* As there was no local variable, return the global value */
> var_type= OPT_GLOBAL;
> }
> + DBUG_PRINT("sysvar", ("var name: '%s' show_type: %d", name, show_type()));
Great.
> switch (show_type()) {
> case SHOW_INT:
> {
> @@ -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.
The same applies below.
> }
> case SHOW_LONG:
> {
> @@ -1770,7 +1843,7 @@ Item *sys_var::item(THD *thd, enum_var_t
> pthread_mutex_lock(&LOCK_global_system_variables);
> value= *(ulong*) 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));
> }
> case SHOW_LONGLONG:
> {
> @@ -1778,7 +1851,7 @@ Item *sys_var::item(THD *thd, enum_var_t
> pthread_mutex_lock(&LOCK_global_system_variables);
> value= *(longlong*) value_ptr(thd, var_type, base);
> pthread_mutex_unlock(&LOCK_global_system_variables);
> - return new Item_int(value);
> + DBUG_RETURN(new Item_int(value));
> }
> case SHOW_DOUBLE:
> {
> @@ -1787,7 +1860,7 @@ Item *sys_var::item(THD *thd, enum_var_t
> value= *(double*) value_ptr(thd, var_type, base);
> pthread_mutex_unlock(&LOCK_global_system_variables);
> /* 6, as this is for now only used with microseconds */
> - return new Item_float(value, 6);
> + DBUG_RETURN(new Item_float(value, 6));
> }
> case SHOW_HA_ROWS:
> {
> @@ -1795,7 +1868,15 @@ Item *sys_var::item(THD *thd, enum_var_t
> pthread_mutex_lock(&LOCK_global_system_variables);
> value= *(ha_rows*) value_ptr(thd, var_type, base);
> pthread_mutex_unlock(&LOCK_global_system_variables);
> - return new Item_int((ulonglong) value);
> + DBUG_RETURN(new Item_int((ulonglong) value));
> + }
> + case SHOW_BOOL:
> + {
> + int32 value;
> + pthread_mutex_lock(&LOCK_global_system_variables);
> + value= *(bool*) value_ptr(thd, var_type, base);
> + pthread_mutex_unlock(&LOCK_global_system_variables);
Adding SHOW_BOOL: ok.
> + DBUG_RETURN(new Item_int(value,1));
> }
> case SHOW_MY_BOOL:
> {
> @@ -1803,7 +1884,7 @@ Item *sys_var::item(THD *thd, enum_var_t
> pthread_mutex_lock(&LOCK_global_system_variables);
> value= *(my_bool*) value_ptr(thd, var_type, base);
> pthread_mutex_unlock(&LOCK_global_system_variables);
> - return new Item_int(value,1);
> + DBUG_RETURN(new Item_int(value,1));
> }
> case SHOW_CHAR_PTR:
> {
> @@ -1822,7 +1903,7 @@ Item *sys_var::item(THD *thd, enum_var_t
> tmp->collation.set(system_charset_info, DERIVATION_SYSCONST);
> }
> pthread_mutex_unlock(&LOCK_global_system_variables);
> - return tmp;
> + DBUG_RETURN(tmp);
> }
> case SHOW_CHAR:
> {
> @@ -1838,12 +1919,39 @@ Item *sys_var::item(THD *thd, enum_var_t
> tmp->collation.set(system_charset_info, DERIVATION_SYSCONST);
> }
> pthread_mutex_unlock(&LOCK_global_system_variables);
> - return tmp;
> + DBUG_RETURN(tmp);
> + }
> + case SHOW_FUNC:
> + {
> + /*
> + This has been added when moving fixed_vars[] into the system
> + variable chain (Bug#28234 - global/session scope - documentation
> + vs implementation). At this time only one SHOW_FUNC variable
> + existed (slave_skip_errors) and it delivered a character string.
> + If/when new SHOW_FUNC variables are introduced with different
> + return types, the function evaluation should be moved to the top
> + of the function, and all cases rewritten to use the resulting
> + value.
> + */
I have a different take on this, see some global comments at the end.
> + MY_ALIGNED_BYTE_ARRAY(buff_data, SHOW_VAR_FUNC_BUFF_SIZE, long);
> + SHOW_VAR svar;
> + uint length;
> + char * const buff= (char *) &buff_data;
> + Item *tmp;
> + pthread_mutex_lock(&LOCK_global_system_variables);
> + ((mysql_show_var_func)value_ptr(thd, var_type, base))(thd, &svar, buff);
> + pthread_mutex_unlock(&LOCK_global_system_variables);
> + /* Assuming a string. See comment above. */
> + DBUG_ASSERT(svar.type == SHOW_CHAR);
> + length= strlen(svar.value);
> + tmp= new Item_string(thd->strmake(svar.value, length), length,
> + system_charset_info, DERIVATION_SYSCONST);
> + DBUG_RETURN(tmp);
> }
> default:
> my_error(ER_VAR_CANT_BE_READ, MYF(0), name);
> }
> - return 0;
> + DBUG_RETURN(0);
> }
>
>
> @@ -3225,14 +3333,12 @@ static int show_cmp(SHOW_VAR *a, SHOW_VA
> SHOW_VAR* enumerate_sys_vars(THD *thd, bool sorted)
> {
> int count= system_variable_hash.records, i;
> - int fixed_count= fixed_show_vars.elements;
> - int size= sizeof(SHOW_VAR) * (count + fixed_count + 1);
> + int size= sizeof(SHOW_VAR) * (count + 1);
Ok
> SHOW_VAR *result= (SHOW_VAR*) thd->alloc(size);
>
> if (result)
> {
> - SHOW_VAR *show= result + fixed_count;
> - memcpy(result, fixed_show_vars.buffer, fixed_count * sizeof(SHOW_VAR));
> + SHOW_VAR *show= result;
Ok
>
> for (i= 0; i < count; i++)
> {
> @@ -3245,7 +3351,7 @@ SHOW_VAR* enumerate_sys_vars(THD *thd, b
>
> /* sort into order */
> if (sorted)
> - my_qsort(result, count + fixed_count, sizeof(SHOW_VAR),
> + my_qsort(result, count, sizeof(SHOW_VAR),
> (qsort_cmp) show_cmp);
Ok
>
> /* make last element empty */
> @@ -3273,13 +3379,6 @@ int set_var_init()
>
> for (sys_var *var=vars.first; var; var= var->next, count++);
>
> - if (my_init_dynamic_array(&fixed_show_vars, sizeof(SHOW_VAR),
> - FIXED_VARS_SIZE + 64, 64))
> - goto error;
> -
> - fixed_show_vars.elements= FIXED_VARS_SIZE;
> - memcpy(fixed_show_vars.buffer, fixed_vars, sizeof(fixed_vars));
> -
Ok
> if (hash_init(&system_variable_hash, system_charset_info, count, 0,
> 0, (hash_get_key) get_sys_var_length, 0, HASH_UNIQUE))
> goto error;
> @@ -3307,28 +3406,6 @@ error:
> void set_var_free()
> {
> hash_free(&system_variable_hash);
> - delete_dynamic(&fixed_show_vars);
> -}
> -
> -
> -/*
> - Add elements to the dynamic list of read-only system variables.
> -
> - SYNOPSIS
> - mysql_append_static_vars()
> - show_vars Pointer to start of array
> - count Number of elements
> -
> - RETURN VALUES
> - 0 SUCCESS
> - otherwise FAILURE
> -*/
> -int mysql_append_static_vars(const SHOW_VAR *show_vars, uint count)
> -{
> - for (; count > 0; count--, show_vars++)
> - if (insert_dynamic(&fixed_show_vars, (uchar*) show_vars))
> - return 1;
> - return 0;
> }
Ok, all that just result from removing the "fixed" system variables.
>
>
>
> === modified file 'sql/set_var.h'
> --- a/sql/set_var.h 2008-09-09 10:19:31 +0000
> +++ b/sql/set_var.h 2008-10-04 19:21:39 +0000
> @@ -930,6 +930,32 @@ public:
> };
>
>
> +/* Global-only, read-only variable. E.g. command line option. */
Please use doxygen comments here.
> +
> +class sys_var_const: public sys_var
> +{
> +public:
> + enum_var_type var_type;
> + SHOW_TYPE show_type_value;
> + uchar *ptr;
> + sys_var_const(sys_var_chain *chain, const char *name_arg, enum_var_type type,
> + SHOW_TYPE show_type_arg, uchar *ptr_arg)
> + :sys_var(name_arg), var_type(type),
> + show_type_value(show_type_arg), ptr(ptr_arg)
> + { chain_sys_var(chain); }
> + bool update(THD *thd, set_var *var) { return 1; }
> + bool check_default(enum_var_type type) { return 1; }
> + bool check_type(enum_var_type type) { return type != var_type; }
> + bool check_update_type(Item_result type) { return 1; }
> + uchar *value_ptr(THD *thd, enum_var_type type, LEX_STRING *base)
> + {
> + return ptr;
> + }
> + SHOW_TYPE show_type() { return show_type_value; }
> + bool is_readonly() const { return 1; }
> +};
> +
> +
New sys_var_const class: good.
> class sys_var_have_option: public sys_var
> {
> protected:
> @@ -1295,7 +1321,6 @@ struct sys_var_with_base
>
> int set_var_init();
> void set_var_free();
> -int mysql_append_static_vars(const SHOW_VAR *show_vars, uint count);
> SHOW_VAR* enumerate_sys_vars(THD *thd, bool sorted);
> int mysql_add_sys_var_chain(sys_var *chain, struct my_option *long_options);
> int mysql_del_sys_var_chain(sys_var *chain);
>
> === modified file 'sql/sql_repl.cc'
> --- a/sql/sql_repl.cc 2008-08-21 11:47:23 +0000
> +++ b/sql/sql_repl.cc 2008-10-04 19:21:39 +0000
> @@ -1658,32 +1658,46 @@ public:
> bool update(THD *thd, set_var *var);
> };
>
> +static int show_slave_skip_errors(THD *thd, SHOW_VAR *var, char *buff);
> +
See comments at the end.
> static sys_var_chain vars = { NULL, NULL };
>
> +static sys_var_const sys_log_slave_updates(&vars, "log_slave_updates",
> + OPT_GLOBAL, SHOW_MY_BOOL,
> + (uchar*) &opt_log_slave_updates);
> +static sys_var_const sys_relay_log(&vars, "relay_log",
> + OPT_GLOBAL, SHOW_CHAR_PTR,
> + (uchar*) &opt_relay_logname);
> +static sys_var_const sys_relay_log_index(&vars, "relay_log_index",
> + OPT_GLOBAL, SHOW_CHAR_PTR,
> + (uchar*) &opt_relaylog_index_name);
> +static sys_var_const sys_relay_log_info_file(&vars, "relay_log_info_file",
> + OPT_GLOBAL, SHOW_CHAR_PTR,
> + (uchar*) &relay_log_info_file);
> static sys_var_bool_ptr sys_relay_log_purge(&vars, "relay_log_purge",
> &relay_log_purge);
> +static sys_var_const sys_relay_log_space_limit(&vars,
> + "relay_log_space_limit",
> + OPT_GLOBAL, SHOW_LONGLONG,
> + (uchar*)
> + &relay_log_space_limit);
> +static sys_var_const sys_slave_load_tmpdir(&vars, "slave_load_tmpdir",
> + OPT_GLOBAL, SHOW_CHAR_PTR,
> + (uchar*) &slave_load_tmpdir);
> static sys_var_long_ptr sys_slave_net_timeout(&vars, "slave_net_timeout",
> &slave_net_timeout);
> +static sys_var_const sys_slave_skip_errors(&vars, "slave_skip_errors",
> + OPT_GLOBAL, SHOW_FUNC,
> + (uchar*) show_slave_skip_errors);
> static sys_var_long_ptr sys_slave_trans_retries(&vars,
> "slave_transaction_retries",
> &slave_trans_retries);
> static sys_var_sync_binlog_period sys_sync_binlog_period(&vars, "sync_binlog",
> &sync_binlog_period);
> static sys_var_slave_skip_counter sys_slave_skip_counter(&vars,
> "sql_slave_skip_counter");
>
> -static int show_slave_skip_errors(THD *thd, SHOW_VAR *var, char *buff);
> -
> -
> -static SHOW_VAR fixed_vars[]= {
> - {"log_slave_updates", (char*) &opt_log_slave_updates,
> SHOW_MY_BOOL},
> - {"relay_log" , (char*) &opt_relay_logname, SHOW_CHAR_PTR},
> - {"relay_log_index", (char*) &opt_relaylog_index_name, SHOW_CHAR_PTR},
> - {"relay_log_info_file", (char*) &relay_log_info_file, SHOW_CHAR_PTR},
> - {"relay_log_space_limit", (char*) &relay_log_space_limit,
> SHOW_LONGLONG},
> - {"slave_load_tmpdir", (char*) &slave_load_tmpdir,
> SHOW_CHAR_PTR},
> - {"slave_skip_errors", (char*) &show_slave_skip_errors, SHOW_FUNC},
> -};
>
> static int show_slave_skip_errors(THD *thd, SHOW_VAR *var, char *buff)
> {
> + DBUG_ENTER("show_slave_skip_errors");
> var->type=SHOW_CHAR;
> var->value= buff;
> if (!use_slave_mask || bitmap_is_clear_all(&slave_error_mask))
> @@ -1716,7 +1730,8 @@ static int show_slave_skip_errors(THD *t
> buff= strmov(buff, "..."); // Couldn't show all errors
> *buff=0;
> }
> - return 0;
> + DBUG_PRINT("show", ("var value: '%s' type: %d", var->value, var->type));
> + DBUG_RETURN(0);
> }
See comments at the end.
>
> bool sys_var_slave_skip_counter::check(THD *thd, set_var *var)
> @@ -1765,8 +1780,6 @@ bool sys_var_sync_binlog_period::update(
>
> int init_replication_sys_vars()
> {
> - mysql_append_static_vars(fixed_vars, sizeof(fixed_vars) / sizeof(SHOW_VAR));
> -
> if (mysql_add_sys_var_chain(vars.first, my_long_options))
> {
> /* should not happen */
>
> === modified file 'sql/sql_show.cc'
> --- a/sql/sql_show.cc 2008-08-20 22:18:33 +0000
> +++ b/sql/sql_show.cc 2008-10-04 19:21:39 +0000
> @@ -2062,6 +2062,8 @@ static bool show_status_array(THD *thd,
> LEX_STRING null_lex_str;
> SHOW_VAR tmp, *var;
> DBUG_ENTER("show_status_array");
> + DBUG_PRINT("show", ("wild: '%s' prefix: '%s' value_type: %d",
> + wild, prefix, value_type));
>
> null_lex_str.str= 0; // For sys_var->value_ptr()
> null_lex_str.length= 0;
> @@ -2073,6 +2075,8 @@ static bool show_status_array(THD *thd,
>
> for (; variables->name; variables++)
> {
> + DBUG_PRINT("show", ("variable: '%s' type: %d",
> + variables->name, variables->type));
> strnmov(prefix_end, variables->name, len);
> name_buffer[sizeof(name_buffer)-1]=0; /* Safety */
> if (ucase_names)
> @@ -2086,6 +2090,7 @@ static bool show_status_array(THD *thd,
> ((mysql_show_var_func)(var->value))(thd, &tmp, buff);
>
> SHOW_TYPE show_type=var->type;
> + DBUG_PRINT("show", ("show_type: %d", show_type));
> if (show_type == SHOW_ARRAY)
> {
> show_status_array(thd, wild, (SHOW_VAR *) var->value, value_type,
> @@ -2106,6 +2111,34 @@ static bool show_status_array(THD *thd,
> show_type= ((sys_var*) value)->show_type();
> value= (char*) ((sys_var*) value)->value_ptr(thd, value_type,
> &null_lex_str);
> + /*
> + This has been added when moving fixed_vars[] into the system
> + variable chain (Bug#28234 - global/session scope -
> + documentation vs implementation). The above SHOW_FUNC loop
> + was not used, as the variable slave_skip_errors came here as
> + SHOW_SYS type. The SHOW_FUNC type popped up after evaluating
> + show_type().
> +
> + I am however unsure at this time, if the below loop is
> + required, or if a single go would do. Also I don't know if
> + value_ptr() needs to be evaluated again to get at the
> + function address. So for now I assert that we do not have a
> + nested SHOW_FUNC.
> + */
See comments at the end.
> + for (; show_type == SHOW_FUNC;)
> + {
> + ((mysql_show_var_func)(value))(thd, &tmp, buff);
> + var= &tmp;
> + show_type= var->type;
> + value= var->value;
> +#ifdef NOT_YET
> + if (show_type == SHOW_FUNC)
> + value= (char*) ((sys_var*) value)->value_ptr(thd, value_type,
> + &null_lex_str);
> +#else
> + DBUG_ASSERT(show_type != SHOW_FUNC);
> +#endif
> + }
> }
>
> pos= end= buff;
>
>
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.
Thanks for investigating this, in hope it helps ;)
