Hello Marc,
Thanks for the patch, this is very aligned with the WL design. There are
some comments below, prefixed with GB.
Marc Alff a écrit, Le 28.05.2009 23:33:
> #At file:///home/malff/BZR-TREE/mysql-6.0-wl4876/ based on
> revid:marc.alff@stripped
>
> 3156 Marc Alff 2009-05-28
> WL#4876 Parse options before initializing mysys
> === modified file 'mysys/my_init.c'
> --- a/mysys/my_init.c 2009-05-07 06:42:31 +0000
> +++ b/mysys/my_init.c 2009-05-28 21:33:45 +0000
> @@ -43,6 +43,7 @@ static void netware_init();
> #endif
>
> my_bool my_init_done= 0;
> +my_bool my_basic_init_done= 0;
> uint mysys_usage_id= 0; /* Incremented for each my_init() */
> ulong my_thread_stack_size= 65536;
>
> @@ -66,26 +67,11 @@ static MYSQL_FILE instrumented_stdout;
> static MYSQL_FILE instrumented_stderr;
> #endif
>
> -/*
> - Init my_sys functions and my_sys variabels
> -
> - SYNOPSIS
> - my_init()
> -
> - RETURN
> - 0 ok
> - 1 Couldn't initialize environment
> -*/
> -
> -my_bool my_init(void)
> +my_bool my_basic_init(void)
GB Needs a doxygen comment.
> {
> - char * str;
> - if (my_init_done)
> + if (my_basic_init_done)
> return 0;
> - my_init_done=1;
> - mysys_usage_id++;
> - my_umask= 0660; /* Default umask for new files */
> - my_umask_dir= 0700; /* Default umask for new directories */
GB This my_basic_init() cherry-picks in the code of the existing
my_init(). Wouldn't it be safer to preserve the existing order of things
(to minimize modifications hence risk)? I.e. my_basic_init() would be a
minimum "prefix" of the existing my_init(), i.e. it would also do
mysys_usage_id++, my_umask=0660, my_umask_dir=0700, and
init_glob_errs() and setting my_progname_short (and then my_init()
wouldn't do that). Those few lines of code don't look harmful to add.
> + my_basic_init_done= 1;
>
> #ifdef HAVE_PSI_INTERFACE
> instrumented_stdin.m_file= stdin;
> === modified file 'mysys/my_thr_init.c'
> --- a/mysys/my_thr_init.c 2009-05-12 03:15:02 +0000
> +++ b/mysys/my_thr_init.c 2009-05-28 21:33:45 +0000
> @@ -102,6 +102,77 @@ void my_threadattr_global_init(void)
>
> static uint get_thread_lib(void);
>
> +static my_bool my_thread_basic_global_init_done= 0;
> +
> +my_bool my_thread_basic_global_init(void)
GB Needs a doxygen comment.
> +{
> + int pth_ret;
> +
> + if (my_thread_basic_global_init_done)
> + return 0;
> + my_thread_basic_global_init_done= 1;
> +
> + /* Mutex used by my_thread_init() and after my_thread_destroy_mutex() */
> + mysql_mutex_init_extra(key_THR_LOCK_threads, &THR_LOCK_threads,
> MY_MUTEX_INIT_FAST,
> + "THR_LOCK_threads", MYF_NO_DEADLOCK_DETECTION);
> + mysql_mutex_init_extra(key_THR_LOCK_malloc, &THR_LOCK_malloc,
> MY_MUTEX_INIT_FAST,
> + "THR_LOCK_malloc", MYF_NO_DEADLOCK_DETECTION);
> + mysql_mutex_init(key_THR_LOCK_open, &THR_LOCK_open, MY_MUTEX_INIT_FAST);
> + mysql_mutex_init(key_THR_LOCK_charset, &THR_LOCK_charset,
> MY_MUTEX_INIT_FAST);
> +
> + if ((pth_ret= pthread_key_create(&THR_KEY_mysys, NULL)) != 0)
> + {
> + fprintf(stderr, "Can't initialize threads: error %d\n", pth_ret);
> + return 1;
> + }
> +
> + if (my_thread_init())
> + return 1;
> +
> + return 0;
> +}
GB So, what moved from my_thread_global_init() to
my_thread_basic_global_init() is:
1) init of 4 mutexes
2) creation of THR_KEY_mysys
3) call to my_thread_init()
Moving (2) is required by moving (3). Why was (3) moved? is it because
code executed in mysqld.cc:main() between my_thread_basic_global_init()
and my_thread_global_init() needs the settings done in my_thread_init()?
> +my_bool my_thread_basic_global_reinit(void)
> +{
> + struct st_my_thread_var *tmp;
> +
> + DBUG_ASSERT(my_thread_basic_global_init_done);
> +
> + /*
> + These mutexes were initialized before the instrumentation.
> + Destroy + create them again, now that the instrumentation
> + is in place.
> + This is safe, since my_thread_basic_global_reinit()
> + is called before creating new threads, so the mutexes
> + are not in use.
> + */
GB Please transform this nice comment into a function comment.
> + mysql_mutex_destroy(&THR_LOCK_threads);
> + mysql_mutex_init_extra(key_THR_LOCK_threads, &THR_LOCK_threads,
> MY_MUTEX_INIT_FAST,
> + "THR_LOCK_threads", MYF_NO_DEADLOCK_DETECTION);
> +
> + mysql_mutex_destroy(&THR_LOCK_malloc);
> + mysql_mutex_init_extra(key_THR_LOCK_malloc, &THR_LOCK_malloc,
> MY_MUTEX_INIT_FAST,
> + "THR_LOCK_malloc", MYF_NO_DEADLOCK_DETECTION);
> +
> + mysql_mutex_destroy(&THR_LOCK_open);
> + mysql_mutex_init(key_THR_LOCK_open, &THR_LOCK_open, MY_MUTEX_INIT_FAST);
> +
> + mysql_mutex_destroy(&THR_LOCK_charset);
> + mysql_mutex_init(key_THR_LOCK_charset, &THR_LOCK_charset,
> MY_MUTEX_INIT_FAST);
> +
> + tmp= my_pthread_getspecific(struct st_my_thread_var*, THR_KEY_mysys);
> + DBUG_ASSERT(tmp);
> +
> + mysql_mutex_destroy(&tmp->mutex);
> + mysql_mutex_init(key_my_thread_var_mutex, &tmp->mutex,
> MY_MUTEX_INIT_FAST);
> +
> + mysql_cond_destroy(&tmp->suspend);
> + mysql_cond_init(key_my_thread_var_suspend, &tmp->suspend, NULL);
> +
> + return 0;
GB it always returns 0; make the function return "void" then.
> === modified file 'sql/mysqld.cc'
> --- a/sql/mysqld.cc 2009-05-26 21:15:12 +0000
> +++ b/sql/mysqld.cc 2009-05-28 21:33:45 +0000
> @@ -4501,6 +4496,14 @@ static void test_lc_time_sz()
> }
> #endif//DBUG_OFF
>
> +#ifdef HAVE_PERFORMANCE_SCHEMA
> +extern struct my_option my_long_perfschema_options[];
> +
> +my_bool get_one_perfschema_option(int, const struct my_option *, char *)
> +{
> + return 0;
> +}
GB just reuse get_one_plugin_option().
> +#endif
>
> #ifdef __WIN__
> int win_main(int argc, char **argv)
> @@ -4508,33 +4511,54 @@ int win_main(int argc, char **argv)
> int main(int argc, char **argv)
> #endif
> {
> - MY_INIT(argv[0]); // init my_sys library & pthreads
> - /* nothing should come before this line ^^^ */
> + /* Perform basic thread library and malloc initialization. */
GB please add in the comment something like "to be able to read defaults
files and parse options".
> + if (my_basic_init())
> + {
> + fprintf(stderr, "my_basic_init() failed.");
> + return 1;
> + }
> +
> + orig_argc= argc;
> + orig_argv= argv;
> + load_defaults(MYSQL_CONFIG_NAME, load_default_groups, &argc, &argv);
> + defaults_argc= argc;
> + defaults_argv= argv;
>
> #ifdef HAVE_PERFORMANCE_SCHEMA
> -#ifndef HAVE_WL4876
> + {
GB Here, please add a comment like "Performance Schema needs to be
initialized as early as possible, before to-be-instrumented objects of
the server are initialized."
> + int ho_error;
> +
> + my_getopt_register_get_addr(NULL);
> + my_getopt_error_reporter= option_error_reporter;
GB If there is an error when parsing a P_S option,
option_error_reporter() will be called, which will call
vprint_msg_to_log() which will use non-initialized objects then we get a
crash; below I used an ambiguous option name to provoke it:
(gdb) r --performance-schema-max
safe_mutex: Trying to lock unitialized mutex at log.cc, line 6733
Program received signal SIGABRT, Aborted.
0x00007fbc32a6a015 in raise () from /lib/libc.so.6
(gdb) bt
#0 0x00007fbc32a6a015 in raise () from /lib/libc.so.6
#1 0x00007fbc32a6bb83 in abort () from /lib/libc.so.6
#2 0x0000000000e020a5 in safe_mutex_lock (mp=0x15db420, my_flags=0,
file=0x101dfed "log.cc",
line=6733) at thr_mutex.c:181
#3 0x00000000008669da in inline_mysql_mutex_lock (that=0x15db420, flags=0,
src_file=0x101dfed "log.cc", src_line=6733) at
../include/mysql/psi/mysql_thread.h:413
#4 0x00000000008696b0 in print_buffer_to_file (level=ERROR_LEVEL,
buffer=0x7fff3c3fdd90 "(null): ambiguous option
'--performance-schema-max' (performance_schema_max_cond,
performance_schema_max_thread_instruments)") at log.cc:6733
#5 0x0000000000869875 in vprint_msg_to_log (level=ERROR_LEVEL,
format=0x10e3cb0 "%s: ambiguous option '--%s' (%s, %s)",
args=0x7fff3c3fe1d0) at log.cc:6766
#6 0x000000000077b91f in option_error_reporter (level=ERROR_LEVEL,
format=0x10e3cb0 "%s: ambiguous option '--%s' (%s, %s)") at
mysqld.cc:9030
#7 0x0000000000dffbfd in handle_options (argc=0x7fff3c3fe5dc,
argv=0x7fff3c3fe5d0,
longopts=0x14aea60,
get_one_option=0x779dc3 <get_one_perfschema_option(int, my_option
const*, char*)>)
at my_getopt.c:353
#8 0x0000000000784cc5 in main (argc=23, argv=0x24749c8) at mysqld.cc:4537
Another object used by vprint_msg_to_log():print_buffer_to_file() is
localtime_r() which may use a mutex (LOCK_localtime_r, in my_pthread.c)
which is initialized in my_thread_global_init(), too late too.
Still we want P_S option parsing failures to go to the logger, because
it's what prints to the Windows Events Log.
I suggest: for P_S options define a simple option_error_reporter() which
just copies the errors in a memory buffer (you can use a plain
sql_string.cc:String instance as recipient, which will automatically
grow as you append to it). If the handle_options(P_S) failed, skip
initialization of P_S (by doing "PSI_hook= NULL;"). After having done
my_init() and logger.init_base() (when log is thus ready), if there is
an error recorded in the String, pass it as a usual error message, to
the usual option_error_reporter() (which will print to the ready log).
Serg said that it's how errors generated by the Linux kernel before
syslog is up, are sent to syslog - they get buffered for the necessary time.
And add a .test testing the crash scenario above.
This logger problem was mentioned in the spec, by the way.
> +
> + /* Skip unknown options so that they may be processed later */
> + my_getopt_skip_unknown= TRUE;
GB Please add a comment here like "options not belonging to Performance
Schema are not processed here but later because some operate on objects
which are not yet initialized (like rpl_filter)".
> + if ((ho_error= handle_options(&argc, &argv, my_long_perfschema_options,
> + get_one_perfschema_option)))
> + return ho_error;
GB Please add this comment:
/** @todo parse --mutex-deadlock-detector here */
(that's not a todo for you, I'll create an unassigned bug report for it).
> + /* Add back the program name handle_options removes */
> + argc++;
> + argv--;
GB If you tweak the MY_INIT() call as explained below, you don't need to
add back anything, it will be ok to throw away the program's name.
> + /* Remove the performance schema options parsed. */
> + defaults_argc= argc;
> + defaults_argv= argv;
GB load_defaults() has allocated memory (wityh alloc_root()) and
defaults_argv has to stay unchanged after that, so that free_defaults()
has the proper pointer to free. This is explained in
init_server_components() (look for "We need to eat").
handle_options() automatically does the removing of parsed P_S options;
you don't need to do anything for it.
Could you please also add a ".test" file to verify that there are no
problems here? It would start mysqld with a ".cnf" file
(--defaults-file=$MYSQLTEST_VARDIR/tmp/something.cnf, see
mysqladmin.test for an example) containing P_S options and then other
options. It would check that all options from .cnf" are well seen. It
can be modelled on your previous cmdline_bad_*.test test.
> +
> + PSI_hook= initialize_performance_schema(& pfs_param);
> + }
<cut>
> + Other provider of the instrumentation interface should
> + initialize PSI_hook here:
> + - HAVE_PSI_INTERFACE is for the instrumentation interface
> + - HAVE_PERFORMANCE_SCHEMA is for one implementation of the interface,
> + but there could be alternate implementations, which is why
> + these two defines are kept separate.
> + TODO:
> + One possible alternate implementation is: enforce a mutex lock order,
> + not between mutex instances as in safe mutex, but between mutex classes,
> + and according to a dependency graph declared explicitly.
GB The TODO comment is probably at a wrong place.
> + */
> #endif /* HAVE_PERFORMANCE_SCHEMA */
>
> #ifdef HAVE_PSI_INTERFACE
> @@ -4560,13 +4584,13 @@ int main(int argc, char **argv)
> if (psi)
> PSI_server->set_thread(psi);
>
> -#ifndef HAVE_WL4876
> - /* TODO: remove with WL#4876: rework the out of order mysys initialization */
> - my_reinit_mutexes();
> -#endif /* HAVE_WL4876 */
GB Please add a comment here, like "recreate objects which were created
before initialization of Performance Schema, so that they are instrumented".
> + if (my_thread_basic_global_reinit())
> + return 1;
> }
> #endif /* HAVE_PSI_INTERFACE */
>
> + MY_INIT(orig_argv[0]); // init my_sys library &
> pthreads
GB Given that MY_INIT() happens late now, "my_progname= name;" (which it
contains) happens late too; as my_progname is sometimes used in error
messages, to be sure that it's present in error messages output by
earlier stages of main() I'd put "my_progname= name;" right after the
start of main(), and replace MY_INIT() above with my_init().
> @@ -7195,83 +7218,6 @@ The minimum value for this variable is 4
> (uchar**) &optimizer_switch_str, (uchar**) &optimizer_switch_str, 0,
> GET_STR, REQUIRED_ARG,
> /*OPTIMIZER_SWITCH_DEFAULT*/0,
> 0, 0, 0, 0, 0},
> -#ifdef HAVE_PERFORMANCE_SCHEMA
> - {"performance_schema", OPT_PFS_ENABLED,
> - "Enable the performance schema.",
> - (uchar**) &pfs_param.m_enabled,
> - (uchar**) &pfs_param.m_enabled,
<cut>
> +#ifdef HAVE_PERFORMANCE_SCHEMA
> +struct my_option my_long_perfschema_options[] =
> +{
> + {"performance_schema", OPT_PFS_ENABLED,
> + "Enable the performance schema.",
> + (uchar**) &pfs_param.m_enabled,
> + (uchar**) &pfs_param.m_enabled,
<cut>
> + {"performance_schema_max_thread_instruments", OPT_PFS_MAX_THREAD_INSTRUMENTS,
> + "Maximum number of thread instruments.",
> + (uchar**) &pfs_param.m_thread_class_sizing,
> + (uchar**) &pfs_param.m_thread_class_sizing,
> + NULL, GET_ULONG, OPT_ARG, PFS_MAX_THREAD_CLASS, 0, ULONG_MAX, 0, 1, NULL},
> + {0, 0, 0, 0, 0, 0, GET_NO_ARG, NO_ARG, 0, 0, 0, 0, 0, 0}
> +};
GB Please move P_S-specific options into ha_perfschema.cc, like for
other engines, with the canonical syntax (example: look for MYSQL_SYSVAR
and PLUGIN_VAR in ha_innodb.cc).
This way, my_print_help_inc_plugins() will pick them up naturally
(because it iterates through plugins) and so does not need to be
augmented with 'more_options'.
However, the invokation of handle_options() still needs to be done
early, as you have done (it cannot be done just like for other plugins,
which happens too late in init_server_components()).
Having P_S options in ha_perfschema.cc will cause handle_options() of
init_server_components():plugin_init():test_plugin_options() to be
called with them in parameter; handle_options() will search argv for P_S
options which it will not find (as we removed them early). This is
inefficient but harmless, and it keeps the code better organized (by
having a plugin's options located in this plugin's code and declared by
the canonical method MYSQL_SYSVAR_etc). We lose on one side but we reuse
more of the existing system like my_print_help_inc_plugins(). We make
P_S less of a special case.
GB A comment about this piece of main():
if (PSI_server)
{
/*
Now that we have parsed the command line arguments, and have
initialized
the performance schema itself, the next step is to register all the
server and mysys instruments.
*/
my_init_wt_psi_keys();
my_init_mysys_psi_keys();
init_server_psi_keys();
/* Instrument the main thread */
PSI_thread *psi= PSI_server->new_thread(key_thread_main, NULL, 0);
if (psi)
PSI_server->set_thread(psi);
if (my_thread_basic_global_reinit())
return 1;
}
#endif /* HAVE_PSI_INTERFACE */
MY_INIT(orig_argv[0]); // init my_sys library &
- my_init_wt_psi_keys() should be done inside wt_init(), it will be more
tidy (letting a module care for its internals and mysqld.cc stay
ignorant of them)
- my_init_mysys_psi_keys() should be done early in
my_thread_basic_global_reinit() (same reason)
- for init_server_psi_keys(), no change needed.
Thank you for looking into this.
--
Mr. Guilhem Bichot <guilhem@stripped>
Sun Microsystems / MySQL, Lead Software Engineer
Bordeaux, France
www.sun.com / www.mysql.com
