Hi, Kristofer!
On May 05, Kristofer Pettersson wrote:
> Sergei Golubchik wrote:
>
>>> + opt->flags|= (PLUGIN_VAR_READONLY);
>> I think this looks weird. Why the parentheses ?
>
> It removed a warning from GCC.
what warning (just curious) ?
>>> sql_print_warning("Server variable %s of plugin %s was forced
>>> "
>>> "to be read-only: string variable without "
>>> "update_func and PLUGIN_VAR_MEMALLOC flag",
>>> @@ -3208,64 +3241,80 @@ static int test_plugin_options(MEM_ROOT
>>> - if (!*enabled && !can_disable)
>>> + /*
>>> + The 'MyISAM' and 'Memory' storage engines currently can't be disabled.
>>> + TODO It really isn't possible to select --myisam=OFF as this option
> name
>>> + isn't registered.
>> why is it not registered ?
>
> MyISAM is not part of the plugins which we iterate over in plugin_init().
It is. We iterate over mysqld_builtins[] array:
for (builtins= mysqld_builtins; *builtins; builtins++)
and myisam is there, at least in my tree (sql_builtin.cc):
struct st_mysql_plugin *mysqld_builtins[]=
{
builtin_binlog_plugin, builtin_myisam_plugin, builtin_partition_plugin,
builtin_csv_plugin, builtin_falcon_plugin, builtin_heap_plugin, builtin_maria_plugin,
builtin_myisammrg_plugin, builtin_ndbcluster_plugin,(struct st_mysql_plugin *)0
};
>>> + */
>>> + can_disable=
>>> + my_strcasecmp(&my_charset_latin1, tmp->name.str, "MyISAM")
> &&
>>> + my_strcasecmp(&my_charset_latin1, tmp->name.str, "MEMORY");
>>> +
>>> + tmp->is_mandatory= (plugin_load_policy == PLUGIN_FORCE);
>>> +
>>> + if (disable_plugin && !can_disable)
>>> {
>>> sql_print_warning("Plugin '%s' cannot be disabled", tmp->name.str);
>>> - *enabled= TRUE;
>>> + disable_plugin= FALSE;
>>> }
>> I like the old behavior, when --help message reflected the fact that the
>> engine is mandatory. Perhaps, again, either pass can_disable down to
>> construct_options, or fix the array afterwards:
>> if (!can_disable)
>> opts[0].comment= 0; // hide disabling option from --help
>
> I think I disagree. Please help me understand what you mean.
>
> Strictly speaking all plugins can now be mandatory, that's why I've writing
> this patch (I think. :))
>
> How are help texts affected by comment== 0? The comment in
> my_print_help_inc_plugins() says "Only options with a non-NULL comment are
> displayed in help text". To me this means that if there is a comment, it
> should be displayed. I don't see an explicit intention to restrict help
> text to "mandatory plugins". Further more the comment to
> the caller usage() states:
> "/* Print out all the options including plugin supplied options */"
>
> Again "all" and "including".
I mean, if a plugin is MYSQL_PLUGIN_MANDATORY() in plug.in it cannot be
disabled. --disable-XXX will not disable a mandatory plugin.
That's why I suggest not to print --XXX option in the --help output for
plugins where this option doesn't work. For example, I suggest not to
print (let's assume there's MYSQL_PLUGIN_MANDATORY(maria) line in
plug.in):
--maria Enables Maria Storage Engine. Use --disable-maria to
disable.
in the --help output, because Maria is enabled by default anyway and
--disable-maria won't disable it.
Note that "plugin supplied options" (like --maria-block-size) will
still be shown. I only suggest to remove --maria option (for plugins
that cannot be disabled).
>>> - error= 1;
>>> -
>>> - if (*enabled)
>>> + /*
>>> + If the plugin is disabled it should not be initialized.
>>> + */
>>> + if (disable_plugin)
>> you print the warning even if global_system_variables.log_warnings is 0,
>> old code wasn't. Why the change ?
> I made the change because I strongly believe that loose and implicit
> connection between variable states are the root of all evil. If I see a
> predicate I want to as quickly as possible deduce its plausibility instead
> of having to reverse engineer the code to find all possible states.
>
> However, I perhaps wasn't careful when I made this change. Can you explain
> to me what the connection is between global_system_variables.log_warnings
> and disabled plugins?
log_warnings specifies the verbosity level. When it's 1 MySQL emits
more warnings in the error log than when it's 0.
As far as I can see, "Plugin XXX is disabled" was considered to be a
non-critical warning, more like an informational message (indeed, it's
printed using sql_print_information, not sql_print_warning), and as such
it should not be printed at low verbosity level.
> Personally I think it makes much sense to always print some kind of message
> when a plugin is loaded, initialized, enabled, disabled or uninitialized.
>
>>> {
>>> + sql_print_information("Plugin '%s' is disabled.",
>>> + tmp->name.str);
>>> + DBUG_RETURN(1);
>>
>> by returning early you skip my_cleanup_options() call, which can cause
>> memoty leaks.
>
> Could you help me see where the memory leak happens?
> The assumption is that the options are allocated on a temporary memory
> root. It also seems like my_cleanup_options() is a wrapper around
> init_variables() which sets the default values for the options. What has
> that to do with memory clean up?
Not exactly. my_cleanup_options() calls init_variables() and passes
fini_one_value() as a second argument. fini_one_value() calls my_free()
for all options of GET_STR_ALLOC type.
Regards / Mit vielen Grüßen,
Sergei
--
__ ___ ___ ____ __
/ |/ /_ __/ __/ __ \/ / Sergei Golubchik <serg@stripped>
/ /|_/ / // /\ \/ /_/ / /__ Principal Software Engineer/Server Architect
/_/ /_/\_, /___/\___\_\___/ Sun Microsystems GmbH, HRB München 161028
<___/ Sonnenallee 1, 85551 Kirchheim-Heimstetten
Geschäftsführer: Thomas Schroeder, Wolfgang Engels, Wolf Frenkel
Vorsitzender des Aufsichtsrates: Martin Häring
