Hello Marc,
Marc Alff a écrit, Le 08.10.2009 18:01:
> Hi Guilhem
>
> Guilhem Bichot wrote:
>> -------- Message original --------
>> Sujet: Re: bzr commit into mysql-6.0-perfschema branch (marc.alff:3160)
>> WL#4876
>> Date: Fri, 05 Jun 2009 14:31:50 +0200
>> De: Guilhem Bichot <guilhem@stripped>
>> Pour :: Marc Alff <Marc.Alff@stripped>
>> Copie à :: commits@stripped
>> Références: <20090604170229.47D0311378A@stripped>
>>
>> Hello Marc,
>>
>> Marc Alff a écrit, Le 04.06.2009 19:02:
>>> #At file:///home/malff/BZR-TREE/mysql-6.0-perfschema/ based on
>>> revid:marc.alff@stripped
>>>
>>> 3160 Marc Alff 2009-06-04
>>> WL#4876 Parse options before initializing mysys
>>> Implemented code review comments
>>> === modified file 'mysys/my_getopt.c'
>>> --- a/mysys/my_getopt.c 2009-05-14 08:56:34 +0000
>>> +++ b/mysys/my_getopt.c 2009-06-04 17:02:25 +0000
>>> @@ -417,9 +417,9 @@ invalid value '%s'",
>>> my_progname, optp->name, optend);
>>> continue;
>>> }
>>> - if (get_one_option(optp->id, optp,
>>> - *((my_bool*) value) ?
>>> - (char*) "1" : disabled_my_option))
>>> + if (get_one_option && get_one_option(optp->id, optp,
>>> + *((my_bool*) value) ?
>> Could you please add a function's comment to say that
>> get_one_option==NULL is now allowed and does nothing?
>
> Done.
Thanks. I see the function has a nice doxygen comment now.
>>> === modified file 'sql/mysqld.cc'
>>> --- a/sql/mysqld.cc 2009-06-03 17:55:20 +0000
>>> +++ b/sql/mysqld.cc 2009-06-04 17:02:25 +0000
>>> + /*
>>> + Some server components:
>>> + - the performance schema,
>>> + - the safe_mutex deadlock detector
>>> + 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;
>>> + my_getopt_register_get_addr(NULL);
>>> + my_getopt_error_reporter= buffered_option_error_reporter;
>>>
>>> - /* Skip unknown options so that they may be processed later */
>>> - my_getopt_skip_unknown= TRUE;
>>> + /* 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)))
>>> + if ((ho_error= handle_options(&argc, &argv,
> my_long_early_options,
>>> NULL)))
>>> + {
>>> + fprintf(stderr, "Failed to parse early options\n");
<cut>
>> Suggestion:
>> - if handle_options() above fails, don't exit main(); disable P_S by
>> doing PSI_server=NULL, don't print buffered errors. Record in
>> buffered_errors that mysqld should exit (so buffered_errors has to
>> become struct { String msg; bool should_exit;}
>
> I think you missed a point here.
> If handle_options() fails, there is no remaining_argc/argv to work with,
> so the server initialization can not proceed.
> There is no hope to execute all the code that is needed to reach the
> time when "the logger is finally available", so "bool should_exit" can
> not be implemented.
>
> I do agree however that the code can try to continue, but for a shorter
> time, to be in a position to be able to call sql_print_error() etc.
good
> With the patch just posted (mysql-trunk-perfschema, revno 2905),
> for failures, the logs will be properly formatted, in stderr and in the
> windows event log.
>
> I assume that by the "real error log", you mean the redirection of
> stderr done with freopen(), to the file pointed by --log-error.
Yes
> Note that the server never print errors in the *redirected* stderr log
> when handle_options() failed, since the code that redirects stderr (see
> "Setup logs" in init_server_components()) is not executed when the
> failure occurs, unless I missed something.
You didn't miss anything. Your best effort is good.
> For the record, I looked at what it would take to parse all the
> arguments needed to properly initialize the error log first.
>
> After several *days* of development, I had to give up on that path: the
> dependencies needed are just too big, and the code is just too unstable
> to be reordered without introducing bugs here.
> My preliminary analysis is that the following is impacted:
> - --help
> - --verbose may as well go with it
> - --console
> - --log-error
> - --pidfile_name
> - then gethostname is needed
> - then the windows socket init code is needed ...
> - --datadir
> - then all the code using datadir before of after changing the working
> directory is impacted ...
>
> If you still want to pursue that path, then this should be done as an
> independent refactoring task, because it affects the server alone.
Let's not pursue this path.
> It should not be a prerequisite of WL#2360, or a sub task of WL#2360.
Yes, it should rather be an unrelated task. Difficult to solve... the
real error log's name is passed as an option (--log-error=name), so if
we didn't manage to parse options, how to have an error log...
I'll now send minor comments on your patch.
