Hi Guilhem
Guilhem Bichot wrote:
> Hello Marc,
>
> Thanks for the prompt reply.
>
> Marc Alff a écrit, Le 04.06.2009 19:38:
>> Guilhem Bichot wrote:
>>> 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_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
>
>>> 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.
>
> Ok, please put this reason in a comment in my_thread_basic_global_init().
Done
>
>>>> === modified file 'sql/mysqld.cc'
>
>>> 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
>
> You are right, I see LOCK_error_log is created in
> init_thread_environment() called by init_common_variables() :-(
> So flushing the buffered error reporter needs to happen late enough.
> Bah, if there is an error in option parsing for P_S, we can just set
> PSI_server to NULL, then resume all the other non-P_S init code, until
> logger is finally ready (late enough) at which point we can check the
> buffered error and finally bail out. We can bail out as late as we like,
> for example just before start_signal_handler().
This was already implemented and pushed when I replied previously.
It seems some commit emails have been lost or delayed today, with some
internals systems down.
>
>> 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.
>
> Agree on both points.
>
>> I tested manually errors (--performance_schema_max) or warnings
>> (--performance_schema_max_cond=-1), but could not get that automated
>> with MTR.
>
> Why not do like those cmdline_bad tests you had?
The tests did work for a simple fprintf(stderr, "<message>").
Now that the real log is used, there are:
- time stamps
- other error messages from the server
- path to files
etc in the output, making this impossible to test with MTR.
In fact, I don't see *any* test in the test scripts that checks that the
*server* (not other things like mysql_admin) properly reads .cnf files,
and there is not even a MYSQLD variable defined in MTR itself to invoke
the server and test command line options.
>
> >>> + 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.
>
> You mean, bug is fixed by a new patch which hasn't yet been committed?
The patch was already committed when I replied.
>
>>>> + /* 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.
>
> And some test? After all, half of the reason for this WL#4876 is to have
> P_S options of my.cnf be honoured, so we should check that they are...
See my previous comments about MTR and the test suite.
>
>>>> + 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.
>
> The code looks like:
>
> #ifdef HAVE_PERFORMANCE_SCHEMA
> {
> int ho_error;
>
> my_getopt_register_get_addr(NULL);
> my_getopt_error_reporter= option_error_reporter;
>
> /* Skip unknown options so that they may be processed later */
> my_getopt_skip_unknown= TRUE;
>
> if ((ho_error= handle_options(&argc, &argv, my_long_perfschema_options,
> get_one_perfschema_option)))
> return ho_error;
> /* Add back the program name handle_options removes */
> argc++;
> argv--;
> /* Remove the performance schema options parsed. */
> defaults_argc= argc;
> defaults_argv= argv;
>
> PSI_hook= initialize_performance_schema(& pfs_param);
> }
> #else
> /*
> 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.
> */
> #endif /* HAVE_PERFORMANCE_SCHEMA */
>
> #ifdef HAVE_PSI_INTERFACE
> /*
> Obtain the current performance schema instrumentation interface,
> if available.
> */
> if (PSI_hook)
> PSI_server= (PSI*) PSI_hook->get_interface(PSI_CURRENT_VERSION);
>
> The comment starts with a description of HAVE_* symbols, i.e. about P_S,
> and changes to a TODO about safemutex deadlock detector; safemutex
> deadlock detector is unrelated to all lines (comment and code) around,
> which are about P_S. Someone who reads the code, will see only P_S code,
> and will wonder "one possible alternate implementation of P_S??" (he
> will not understand that this is about deadlock detector unless he knows
> WL#4876).
> The TODO comment should instead be somewhere around the mutex deadlock
> detector implementation.
>
>>>> +#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.
>
> I was suggesting that options are defined in ha_perfschema using the
> normal engine way (MYSQL_SYSVAR), exported, and mysqld.cc:main() calls
> handle_options() on them.
> That requires a transformation of MYSQL_SYSVAR objects to my_getopt
> objects so that handle_options() can be called on them; this
> transformation exists in test_plugin_options() so I guess that making a
> fake plugin object and call test_plugin_options() on it would do the
> trick. Unfortunately I tried to hack this, and it hits problem after
> problem: here's the code I had in the end:
> #ifdef HAVE_PERFORMANCE_SCHEMA
> {
> my_getopt_register_get_addr(NULL);
> my_getopt_error_reporter= option_error_reporter;
>
> /* Skip unknown options so that they may be processed later */
> my_getopt_skip_unknown= TRUE;
>
> {
> struct st_plugin_int tmp;
> struct st_mysql_plugin tmp_plugin;
> init_alloc_root(&tmp.mem_root, 4096, 4096);
> tmp.plugin= &tmp_plugin;
> tmp_plugin.name= tmp.name.str= "PERFORMANCE_SCHEMA";
> tmp.name.length= strlen(tmp.name.str);
> tmp.plugin->system_vars= pfs_system_variables;
> system_charset_info= &my_charset_utf8_general_ci;
> set_var_init();
> pfs_param.m_enabled= !test_plugin_options(&tmp.mem_root, &tmp,
> &argc, argv, TRUE);
> set_var_free();
> free_root(&tmp.mem_root, MYF(0));
> }
>
> PSI_hook= initialize_performance_schema(& pfs_param);
> }
> and even that didn't work. Too many objects to set up it seems.
> So, looks like doing this adds a bigger risk than the benefit of the
> requested review change :-( Doing things really cleanly would probably
> require more changes, too much.
> So please keep variables in mysqld.cc, and put a comment why they must
> be there.
> Keep my_print_help_inc_plugins() but change names of variables: from the
> point of view of this function, there aren't really main options and
> non-main ones, it treats them equal. So I'd use:
> void my_print_help_inc_plugins(my_option *options1, uint options1_size,
> my_option *options2, uint options2_size)
> or (if you like), to have it extendible to an infinite list of
> (my_option*,uint) pairs, use a vararg, or a 0-terminated array.
That was already done.
>
>>> 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_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.
>
> It cannot be executed earlier, but my_thread_basic_global_reinit()
> (which I suggested) precisely happens later (just a few lines after the
> place where you put my_init_mysys_psi_keys(), right after set_thread()).
Sorry, somehow I did read my_init() instead of
my_thread_basic_global_reinit().
Implemented then.
