maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Oleksandr!

On Aug 07, Oleksandr Byelkin wrote:
> >> diff --git a/mysql-test/r/mysqld--help.result b/mysql-test/r/mysqld--help.result
> >> index fc22af6..1f2a8cf 100644
> >> --- a/mysql-test/r/mysqld--help.result
> >> +++ b/mysql-test/r/mysqld--help.result
> >> @@ -890,6 +890,7 @@ The following options may be given as the first argument:
> >>    write privileges to the mysql.user table.
> >>    --secure-auth       Disallow authentication for accounts that have old
> >>    (pre-4.1) passwords
> >> + (Defaults to on; use --skip-secure-auth to disable.)
> > You know what, it would be nice if the user would know what options
> > support the --autoset prefix. I think here's a good place for it,
> > the help for back_log can end with
> >
> >        gets very many connection requests in a very short time
> >        (Automatically configured unless set explicitly)
> >
> > Of course, this line should be automatically generated by my_print_help(),
> > not manually added to the help text of every autoset variable.

OK?

> >>   1063	89366	89366	28296	28296	3
> >> +1495	89366	89366	28296	28296	3
> >>   221	56120	56120	28296	28296	3
> >> +961	24512	24512	85239	85239	4
> > why?
> now result is sorted (and it should be because there is no order in the 
> statement)

ok, sorry. I've missed that you added --sorted_result

> >> diff --git a/mysys/my_getopt.c b/mysys/my_getopt.c
> >> index 88fa3d4..1d32833 100644
> >> --- a/mysys/my_getopt.c
> >> +++ b/mysys/my_getopt.c
> >> @@ -47,14 +47,15 @@ static char *check_struct_option(char *cur_arg, char *key_name);
> >>     order of their arguments must correspond to each other.
> >>   */
> >>   static const char *special_opt_prefix[]=
> >> -{"skip", "disable", "enable", "maximum", "loose", 0};
> >> +{"skip", "disable", "enable", "maximum", "loose", "autoset", 0};
> > So, I'm not very happy with the --autoset- prefix. Because
> > it cannot be used from the SQL. In SQL we have to use
> >      SET variable=AUTO;
> > or something like that. And it would be good to use the same approach
> > on the command line and from SQL. On the other hand, =AUTO has its problems
> > too - it conflicts with the AUTO value of the enum/set variables.
> >
> > I don't have a good answer to this :(
> IMHO better to have both, but lets add SET...=AUTO when it will be 
> requested.

No, please, not both. Not --autoset-variable=AUTO.
Either =AUTO or --autoset. Because of the following:

> >> +	else if (option_is_autoset)
> >> +	{
> >> +	  if (optend)
> >> +	  {
> >> +	    my_getopt_error_reporter(ERROR_LEVEL,
> >> +                                     "%s: automatic setup request of "
> >> +                                     "option '--%s' cannot take an argument",
> >> +                                     my_progname, optp->name);
> > heh, another argument agains --autoset- prefix. With the =AUTO value
> > one cannot possibly have this error.
> >
> > So, perhaps, I'm *slighty* leaning towards the =AUTO vs --autoset.

I've started to prefer =AUTO. It's also future-proof (we can later to
=AUTO from SQL).

> >> diff --git a/sql/mysqld.cc b/sql/mysqld.cc
> >> index 43a3f0e..21a1198 100644
> >> --- a/sql/mysqld.cc
> >> +++ b/sql/mysqld.cc
> >> @@ -4316,6 +4316,27 @@ static int init_common_variables()
> >>     }
> >>   #endif /* HAVE_SOLARIS_LARGE_PAGES */
> >>   
> >> +
> >> +  /* Fix host_cache_size. */
> >> +  if (IS_SYSVAR_AUTOSIZE(&host_cache_size))
> >> +  {
> >> +    if (max_connections <= 628 - 128)
> >> +      SYSVAR_AUTOSIZE(host_cache_size, 128 + max_connections);
> >> +    else if (max_connections <= ((ulong)(2000 - 628)) * 20 + 500)
> >> +      SYSVAR_AUTOSIZE(host_cache_size, 628 + ((max_connections - 500) / 20));
> >> +    else
> >> +      SYSVAR_AUTOSIZE(host_cache_size, 2000);
> >> +  }
> >> +
> >> +  /* Fix back_log */
> >> +  if (back_log == 0 || IS_SYSVAR_AUTOSIZE(&back_log))
> > why back_log==0 ?
> MySQL/OLD way to get automatic value, lest for compatibility.

Certainly not "OLD" way, because lowest allowed back_log value was 1,
you've changed it to be 0 in this patch. May be it's for 5.6
compatibility?

> >> +typedef Sys_var_integer<ulong, (GET_ULONG|GET_AUTO), SHOW_ULONG> Sys_var_aulong;
> > Hmm, why did you make it a new type? I think the *type* of the variable
> > doesn't depend on the auto-set behavior. The type defines storage
> > size, how to print the value, etc. While auto-set is just a behavior
> > modifier. I'd rather put it in flags:
> >
> > -      READ_ONLY GLOBAL_VAR(back_log), CMD_LINE(REQUIRED_ARG),
> > +      AUTO_SET READ_ONLY GLOBAL_VAR(back_log), CMD_LINE(REQUIRED_ARG),
> the problem was in how macro/constructor made, I did not found the way 
> to do it in flags, I will recheck.

Like

  if (flags & AUTO_SET)
    option.var_type|= GET_AUTO;

> >> @@ -2751,16 +2743,38 @@ static bool check_query_cache_type(sys_var *self, THD *thd, set_var *var)
> >> +  if (var->type != OPT_GLOBAL && global_system_variables.query_cache_type == 0)
> >>     {
> >> -    my_error(ER_QUERY_CACHE_IS_GLOBALY_DISABLED, MYF(0));
> >> -    return true;
> >> -  }
> >> +    uint value= 0 /*default is off*/;
> >> +    if (var->value)
> >> +    {
> >> +      if (var->value->result_type() == INT_RESULT)
> >> +        value= var->value->val_int();
> >> +      else
> >> +      {
> >> +        char buff[STRING_BUFFER_USUAL_SIZE];
> >> +        String str(buff, sizeof(buff), system_charset_info), *res;
> >> +        if (!(res=var->value->val_str(&str)))
> >> +          return true;
> >> +        if (res->length() != 3 ||
> >> +            my_toupper(res->charset(), res->ptr()[0]) != 'O' ||
> >> +            my_toupper(res->charset(), res->ptr()[1]) != 'F' ||
> >> +            my_toupper(res->charset(), res->ptr()[2]) != 'F')
> > eh? query_cache_type is ENUM variable, there is no need to compare
> > string values manually.
> The problem is that it called before processing, and it was bug before 
> to check only numeric value.

Shouldn't be. See sys_var::check():

    if ((var->value && do_check(thd, var))
       || (on_check && on_check(this, thd, var)))

First it calls do_check(), then on_check(). on_check() is your
ON_CHECK callback, that is check_query_cache_type(). do_check() is
Sys_var_typelib::do_check(). It converts the value from the string to a
number of the enum element.

Regards,
Sergei