Re: bzr commit into mysql-6.0-perfschema branch (marc.alff:3156) WL#4876

[Guilhem Bichot]

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().

>>> === 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().

> 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?

  >>> +    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?

>>> +    /* 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...

>>> +    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.

>> 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()).

-- 
Mr. Guilhem Bichot <guilhem@stripped>
Sun Microsystems / MySQL, Lead Software Engineer
Bordeaux, France
www.sun.com / www.mysql.com