Hi, Kristofer!
On Apr 29, Kristofer Pettersson wrote:
> #At file:///home/thek/Development/cpp/mysqlbzr/51-bug19027/ based on
> revid:holyfoot@stripped
>
> 2877 Kristofer Pettersson 2009-04-29
> Bug#19027 MySQL 5.0 starts even with Fatal InnoDB errors
Pretty good, we're almost there :)
a couple of minor comments, see below.
> === modified file 'mysql-test/lib/mtr_cases.pm'
> --- a/mysql-test/lib/mtr_cases.pm 2009-01-21 10:17:16 +0000
> +++ b/mysql-test/lib/mtr_cases.pm 2009-04-29 12:08:45 +0000
> @@ -887,7 +887,7 @@ sub collect_one_test_case {
> if ( $tinfo->{'innodb_test'} )
> {
> # This is a test that need innodb
> - if ( $::mysqld_variables{'innodb'} ne "TRUE" )
> + if ( $::mysqld_variables{'innodb'} eq "OFF" )
why ?
> {
> # innodb is not supported, skip it
> $tinfo->{'skip'}= 1;
>
> === modified file 'sql/mysqld.cc'
> --- a/sql/mysqld.cc 2009-04-24 11:28:46 +0000
> +++ b/sql/mysqld.cc 2009-04-29 12:08:45 +0000
> @@ -300,6 +300,16 @@ TYPELIB sql_mode_typelib= { array_elemen
> sql_mode_names,
> (unsigned int *)sql_mode_names_len };
>
> +/**
> + @note The order of the enumeration is critical.
:) good!
> + @see construct_options
> +*/
> +static const char *global_plugin_typelib_names[]=
> + { "OFF", "ON", "FORCE", NULL };
> +static const unsigned int global_plugin_typelib_len[]= {2,3,5};
> +TYPELIB global_plugin_typelib= { 3, "", global_plugin_typelib_names,
> + (unsigned int *)global_plugin_typelib_len };
You don't need to provide lengths - they're only used in copy_typelib(),
that is for ENUM/SET fields.
And instead of hard-coded 3 use
array_elements(global_plugin_typelib_names)-1
(unfortunately, the count is necessary, even though it's redundant)
> static const char *optimizer_switch_names[]=
> {
> "index_merge","index_merge_union","index_merge_sort_union",
>
> === modified file 'sql/sql_plugin.cc'
> --- a/sql/sql_plugin.cc 2009-03-16 10:37:13 +0000
> +++ b/sql/sql_plugin.cc 2009-04-29 12:08:45 +0000
> @@ -28,6 +28,7 @@
> #endif
>
> extern struct st_mysql_plugin *mysqld_builtins[];
> +extern TYPELIB global_plugin_typelib;
better to define the typelib here, as a static variable. What is the
benefit of having it in mysqld.cc ?
> char *opt_plugin_load= NULL;
> char *opt_plugin_dir_ptr;
> @@ -2934,14 +2940,25 @@ static int construct_options(MEM_ROOT *m
> *((my_bool *)(name -1))= **enabled;
> *enabled= (my_bool *)(name - 1);
>
> options[1].name= (options[0].name= name) + namelen + 1;
> options[0].id= options[1].id= 256; /* must be >255. dup id ok */
> - options[0].var_type= options[1].var_type= GET_BOOL;
> - options[0].arg_type= options[1].arg_type= NO_ARG;
> - options[0].def_value= options[1].def_value= **enabled;
> - options[0].value= options[0].u_max_value=
> - options[1].value= options[1].u_max_value= (uchar**) (name - 1);
> + options[0].var_type= options[1].var_type= GET_ENUM;
> + options[0].arg_type= options[1].arg_type= OPT_ARG;
> + /*
> + Set the default value: 0 = OFF, 1 = ON
> + */
> + options[0].def_value= options[1].def_value= (int)**enabled;
> + options[0].typelib= options[1].typelib= &global_plugin_typelib;
> + /*
> + Allocate temporary space for the value of the tristate.
> + This option will have a limited lifetime and is not used beyond
> + server initialization.
> + */
> + options[0].value= options[1].value= (uchar **)alloc_root(mem_root,
> + sizeof(my_option **));
all ok, but you've rendered 'enabled' useless. I'd suggest to reuse it
instead - that is, to write
options[1].value= options[1].u_max_value= *enabled;
as before, and fix the code so that enabled is uint**, not my_bool**,
and allocated appropriately (sizeof(uint) bytes, not 1).
> + *((uint*) options[0].value)= *((uint*) options[1].value)=
> + (uint) options[0].def_value;
> +
> options+= 2;
>
> /*
> === modified file 'sql/sql_plugin.h'
> --- a/sql/sql_plugin.h 2008-12-17 15:45:34 +0000
> +++ b/sql/sql_plugin.h 2009-04-29 12:08:45 +0000
> @@ -79,6 +79,7 @@ struct st_plugin_int
> void *data; /* plugin type specific, e.g. handlerton */
> MEM_ROOT mem_root; /* memory for dynamic plugin structures */
> sys_var *system_vars; /* server variables for this plugin */
> + bool is_mandatory; /* If true then plugin must not fail to load */
I don't quite like this solution - st_plugin_int is used for the whole
plugin lifetime, while is_mandatory is only meaningful on load, after
that it's just a waste. But I don't have anything better :(
> };
Regards / Mit vielen Grüßen,
Sergei
--
__ ___ ___ ____ __
/ |/ /_ __/ __/ __ \/ / Sergei Golubchik <serg@stripped>
/ /|_/ / // /\ \/ /_/ / /__ Principal Software Engineer/Server Architect
/_/ /_/\_, /___/\___\_\___/ Sun Microsystems GmbH, HRB München 161028
<___/ Sonnenallee 1, 85551 Kirchheim-Heimstetten
Geschäftsführer: Thomas Schroeder, Wolfgang Engels, Dr. Roland Boemer
Vorsitzender des Aufsichtsrates: Martin Häring
