Hi Guilhem
Guilhem Bichot wrote:
> 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.
>
Fixed
>> {
>> - 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.
I agree, fixed.
>
>> + 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.
>
Fixed
>> +{
>> + 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()?
>
The thread local storage needs to be initialized, in particular for
assignments to my_errno.
>> +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.
Fixed
>
>> + 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.
>
Changed
>> === 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().
>
Fixed handle_option to allow NULL instead.
Removed 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".
Done
>
>> + 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."
>
Done
>> + 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.
>
Implemented a buffered error reporter, to keep errors until the logger
is available.
For the logger, I don't think logger.init_base() is sufficient:
- the logger needs more setup, and in particular command line options
affecting the logger behavior have not been parsed yet
- mutexes like LOCK_error_log are initialized later
Refactoring the command line options parsing for the logger is not part
of the scope of this patch, but I think every option affecting logging
eventually should be parsed earlier in the server, as well.
I tested manually errors (--performance_schema_max) or warnings
(--performance_schema_max_cond=-1), but could not get that automated
with MTR.
>> +
>> + /* 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)".
Added comments in the options arrays.
>
>> + 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).
>
It's implemented.
>> + /* 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.
No change
>
>> + /* 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.
>
Fixed the free_defaults() problem, added clarity in the code.
>> +
>> + 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.
>
No change, unless there is a better suggestion.
>> + */
>> #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".
>
Done
>> + 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().
>
I agree, fixed.
>> @@ -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.
>
Please clarify how:
- the performance schema options implemented in the engine plugin
- parsing the performance schema options early at server startup
is supposed to work then.
> 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)
This one I agree with.
It was reported earlier, but I never got to fix it.
Implemented.
> - my_init_mysys_psi_keys() should be done early in
> my_thread_basic_global_reinit() (same reason)
No.
The code needs a PSI_server != NULL, and therefore can not be executed
earlier.
> - for init_server_psi_keys(), no change needed.
>
> Thank you for looking into this.
>
Thanks for your comments.
Regards,
-- Marc
