maria-discuss team mailing list archive

[Michael Widenius <michael.widenius@xxxxxxxxx>]

Hi!

On Wed, Mar 21, 2018 at 2:12 PM, Sergey Vojtovich <svoj@xxxxxxxxxxx> wrote:
> Hi Monty,
>
>> diff --git a/mysql-test/mysql-test-run.pl b/mysql-test/mysql-test-run.pl
>> index 55543e529a6..54dc0aa17cc 100755
>> --- a/mysql-test/mysql-test-run.pl
>> +++ b/mysql-test/mysql-test-run.pl
>> @@ -4469,6 +4469,7 @@ sub extract_warning_lines ($$) {
>>       qr/InnoDB: See also */,
>>       qr/InnoDB: Cannot open .*ib_buffer_pool.* for reading: No such file or directory*/,
>>       qr/InnoDB: Table .*mysql.*innodb_table_stats.* not found./,
>> +     qr/Changed limits: max_open_files/,
> I wonder how did you get this warning?

You almost always gets this warning when running mtr as we have to
adjust the values
as many system only allows you to use 1024 file descriptors by default.

>>       qr/InnoDB: User stopword table .* does not exist./
>>
>>      );
>> diff --git a/sql/mysqld.cc b/sql/mysqld.cc
>> index b21d50a20fe..b3414279ac9 100644
>> --- a/sql/mysqld.cc
>> +++ b/sql/mysqld.cc
>> @@ -4446,11 +4446,18 @@ static int init_common_variables()
>>
>>    /* connections and databases needs lots of files */
>>    {
>> -    uint files, wanted_files, max_open_files;
>> +    uint files, wanted_files, max_open_files, min_tc_size, extra_files,
>> +      min_connections;
>> +    ulong org_max_connections= max_connections, org_tc_size= tc_size;
>>
>> +    /* Number of files reserved for temporary files */
>> +    extra_files= 30;
>> +    min_connections= 10;
>>      /* MyISAM requires two file handles per table. */
>> -    wanted_files= (10 + max_connections + extra_max_connections +
>> +    wanted_files= (extra_files + max_connections + extra_max_connections +
>>                     tc_size * 2);
>> +    min_tc_size= MY_MIN(tc_size, TABLE_OPEN_CACHE_MIN);
>> +
>>      /*
>>        We are trying to allocate no less than max_connections*5 file
>>        handles (i.e. we are trying to set the limit so that they will
> I believe the following is easier to read:
>
>     ulong org_max_connections= max_connections;
>     ulong org_tc_size= tc_size;
>     ulong min_tc_size= MY_MIN(tc_size, TABLE_OPEN_CACHE_MIN);
>     /* Number of files reserved for temporary files */
>     uint extra_files= 30;
>     uint min_connections= 10;
>     /* MyISAM requires two file handles per table. */
>     uint wanted_files= extra_files + max_connections + extra_max_connections +
>                        tc_size * 2;

I strongly prefer to have the declaration separated:
- All declaration in one place show more clearly how much memory is allocated
- Having declaration and assignment on same line, makes it harder to see
  the important part, which is the assignment not the declaration.

I have now moved the org_... values away from the declarations, just
to make this part more clear.

<cut>
 /*
>> +      Decrease tc_size according to max_connections, but
>> +      not below min_tc_size.  Outer MY_MIN() ensures that we
>> +      never increase tc_size automatically (that could
>> +      happen if max_connections is decreased above).
>> +    */
>> +    SYSVAR_AUTOSIZE(tc_size,
>> +                    (ulong) MY_MIN(MY_MAX((files - extra_files -
>> +                                           max_connections) / 2, min_tc_size),
>> +                                   tc_size));
>> +    DBUG_PRINT("warning",
>> +               ("Current limits: max_open_files: %u  max_connections: %ld  table_cache: %ld",
>> +                files, max_connections, tc_size));
>> +    if (global_system_variables.log_warnings > 1 &&
>> +        (max_connections < org_max_connections ||
>> +         tc_size < org_tc_size))
>> +      sql_print_warning("Changed limits: max_open_files: %u  max_connections: %lu (%lu)  table_cache: %lu (%lu)",
> I'd say this format is quite confusing. Probably add something like "was %lu"
> in brackets?

I can add was, even if I think it's obvious and we do the same in
other places as far as I can remember.


>> +                     files, max_connections, org_max_connections,
>> +                        tc_size, org_tc_size);
> I think the above code must be executed under "if (files < wanted_files)".
> We don't really want to "autosize" max_connections and tc_size if we got
> enough file descriptors.

We don't do that with the current code. Max connections is set with:

    SYSVAR_AUTOSIZE(max_connections,
                    (ulong) MY_MAX(MY_MIN(files- extra_files- min_tc_size*2,
                                          max_connections), min_connections));

Which ensures that if files is big enough, it's limited to the current
value of max_connections.
Same for tc_size.


> Also you missed to "autosize" open_files_limit. I think it should be done
> "if (open_files_limit != files) SYSVAR_AUTOSIZE(open_files_limit, files);".

ok

>>    }
>>
>>    /*
>> -    Max_connections is now set.
>> +    Max_connections and tc_cache is now set.
> s/is/are/
>
>>      Now we can fix other variables depending on this variable.
>>    */
>>
>> diff --git a/sql/sql_const.h b/sql/sql_const.h
>> index 65742235bee..e28a0649f04 100644
>> --- a/sql/sql_const.h
>> +++ b/sql/sql_const.h
>> @@ -127,7 +127,7 @@
>>  #define MAX_FIELDS_BEFORE_HASH       32
>>  #define USER_VARS_HASH_SIZE     16
>>  #define SEQUENCES_HASH_SIZE     16
>> -#define TABLE_OPEN_CACHE_MIN    400
>> +#define TABLE_OPEN_CACHE_MIN    200
>>  #define TABLE_OPEN_CACHE_DEFAULT 2000
>>  #define TABLE_DEF_CACHE_DEFAULT 400
>>  /**
>> diff --git a/sql/sys_vars.cc b/sql/sys_vars.cc
>> index de9806ab289..41317a5c259 100644
>> --- a/sql/sys_vars.cc
>> +++ b/sql/sys_vars.cc
>> @@ -3477,7 +3477,7 @@ static bool fix_table_open_cache(sys_var *, THD *, enum_var_type)
>>  static Sys_var_ulong Sys_table_cache_size(
>>         "table_open_cache", "The number of cached open tables",
>>         GLOBAL_VAR(tc_size), CMD_LINE(REQUIRED_ARG),
>> -       VALID_RANGE(1, 1024*1024), DEFAULT(TABLE_OPEN_CACHE_DEFAULT),
>> +       VALID_RANGE(10, 1024*1024), DEFAULT(TABLE_OPEN_CACHE_DEFAULT),
> Why? There're at least a few test cases that do table_open_cache=1:
> partition_open_files_limit, sp, myisam.

I don't really see a point to have 1, just for a few tests;  It's more
likely that a user gets things wrong
by accidently setting this value too low.  Would be better to adjust the tests.

I have now set it to 1, just for the test, but think that should be
fixed eventually.

Regards,
Monty