maria-developers team mailing list archive

[Sachin Setiya <sachin.setiya@xxxxxxxxxxx>]

Hi Nirbhay!

On Fri, Jan 20, 2017 at 7:09 AM, Nirbhay Choubey <nirbhay@xxxxxxxxxxx>
wrote:

> Hi Sachin,
>
> The overall patch looks ok. I, however, have a few minor comments inline.
>
>
> On Thu, Jan 19, 2017 at 1:21 AM, SachinSetiya <sachin.setiya@xxxxxxxxxxx>
> wrote:
>
>> revision-id: 98b2a9c967a5eaa1f99bb3ef229ff2af62018ffe
>> (mariadb-10.0.28-34-g98b2a9c)
>> parent(s): 9bf92706d19761722b46d66a671734466cb6e98e
>> author: Sachin Setiya
>> committer: Sachin Setiya
>> timestamp: 2017-01-19 11:50:59 +0530
>> message:
>>
>> MDEV-4774 Strangeness with max_binlog_stmt_cache_size Settings
>>
>> Problem:- When setting max_binlog_stmt_cache_size=18446744073709547520
>> from either command line or .cnf file, server fails to start.
>>
>> Solution:- Added one more function eval_num_suffix_ull , which uses
>> strtoull to get unsigned ulonglong from string. And getopt_ull calles this
>>
>
> Typo s/calles/calls/
>
Changed.

>
>
>> function instead of eval_num_suffix. Also changed previous
>> eval_num_suffix to
>> eval_num_suffix_ll to remain consistent.
>>
>> ---
>>  .../r/binlog_max_binlog_stmt_cache_size.result     | 14 +++++
>>  .../binlog/t/binlog_max_binlog_stmt_cache_size.opt |  1 +
>>  .../t/binlog_max_binlog_stmt_cache_size.test       | 11 ++++
>>  mysys/my_getopt.c                                  | 66
>> ++++++++++++++++++----
>>  4 files changed, 81 insertions(+), 11 deletions(-)
>>
>> diff --git a/mysql-test/suite/binlog/r/binlog_max_binlog_stmt_cache_size.result
>> b/mysql-test/suite/binlog/r/binlog_max_binlog_stmt_cache_size.result
>> new file mode 100644
>> index 0000000..6fbec90
>> --- /dev/null
>> +++ b/mysql-test/suite/binlog/r/binlog_max_binlog_stmt_cache_size.result
>> @@ -0,0 +1,14 @@
>> +select @@max_binlog_stmt_cache_size;
>> +@@max_binlog_stmt_cache_size
>> +18446744073709547520
>> +set global max_binlog_stmt_cache_size= 18446744073709547520;
>> +select @@max_binlog_stmt_cache_size;
>> +@@max_binlog_stmt_cache_size
>> +18446744073709547520
>> +set global max_binlog_stmt_cache_size= 18446744073709547519;
>> +Warnings:
>> +Warning        1292    Truncated incorrect max_binlog_stmt_cache_size
>> value: '18446744073709547519'
>> +select @@max_binlog_stmt_cache_size;
>> +@@max_binlog_stmt_cache_size
>> +18446744073709543424
>> +set global max_binlog_stmt_cache_size= 18446744073709547520;
>> diff --git a/mysql-test/suite/binlog/t/binlog_max_binlog_stmt_cache_size.opt
>> b/mysql-test/suite/binlog/t/binlog_max_binlog_stmt_cache_size.opt
>> new file mode 100644
>> index 0000000..c52ef14
>> --- /dev/null
>> +++ b/mysql-test/suite/binlog/t/binlog_max_binlog_stmt_cache_size.opt
>> @@ -0,0 +1 @@
>> +--max_binlog_stmt_cache_size=18446744073709547520
>> diff --git a/mysql-test/suite/binlog/t/binlog_max_binlog_stmt_cache_size.test
>> b/mysql-test/suite/binlog/t/binlog_max_binlog_stmt_cache_size.test
>> new file mode 100644
>> index 0000000..bc30b48
>> --- /dev/null
>> +++ b/mysql-test/suite/binlog/t/binlog_max_binlog_stmt_cache_size.test
>> @@ -0,0 +1,11 @@
>> +source include/have_log_bin.inc;
>> +select @@max_binlog_stmt_cache_size;
>> +
>> +--let $cache_size=`select @@max_binlog_stmt_cache_size;`
>> +
>> +set global max_binlog_stmt_cache_size= 18446744073709547520;
>> +select @@max_binlog_stmt_cache_size;
>> +
>> +set global max_binlog_stmt_cache_size= 18446744073709547519;
>> +select @@max_binlog_stmt_cache_size;
>>
>
> I would also add tests for ULLONG_MAX and ULLONG_MAX +/- 1.
>
> Added the test for ULLONG_MAX+1, I am already testing for ULLONG_MAX and
ULLONG_MAX-1.

> +--eval set global max_binlog_stmt_cache_size= $cache_size
>> diff --git a/mysys/my_getopt.c b/mysys/my_getopt.c
>> index 2a45571..0934a50 100644
>> --- a/mysys/my_getopt.c
>> +++ b/mysys/my_getopt.c
>> @@ -895,11 +895,32 @@ my_bool getopt_compare_strings(register const char
>> *s, register const char *t,
>>  /*
>>    function: eval_num_suffix
>>
>> +  Transforms suffix like k/m/g to there real value.
>>
>
> Typo : s/there/their/
>
Changed.

>
>
>> +*/
>> +static inline long eval_num_suffix(char *suffix, int *error)
>> +{
>> +  long num= 1;
>> +  if (*suffix == 'k' || *suffix == 'K')
>> +    num*= 1024L;
>> +  else if (*suffix == 'm' || *suffix == 'M')
>> +    num*= 1024L * 1024L;
>> +  else if (*suffix == 'g' || *suffix == 'G')
>> +    num*= 1024L * 1024L * 1024L;
>> +  else if (*suffix)
>> +  {
>> +    *error= 1;
>> +    return 0;
>> +  }
>> + return num;
>> +}
>>
>
> Please add a new line here (as a separator).
>
Added.

>
>
>> +/*
>> +  function: eval_num_suffix_ll
>> +
>>    Transforms a number with a suffix to real number. Suffix can
>>    be k|K for kilo, m|M for mega or g|G for giga.
>>  */
>>
>> -static longlong eval_num_suffix(char *argument, int *error, char
>> *option_name)
>> +static longlong eval_num_suffix_ll(char *argument, int *error, char
>> *option_name)
>>  {
>>    char *endchar;
>>    longlong num;
>>
>
>   DBUG_ENTER("eval_num_suffix");
>
> You should also update the function name.
>
Changed.

>
>
>> @@ -916,23 +937,46 @@ static longlong eval_num_suffix(char *argument, int
>> *error, char *option_name)
>>      *error= 1;
>>      DBUG_RETURN(0);
>>    }
>> -  if (*endchar == 'k' || *endchar == 'K')
>> -    num*= 1024L;
>> -  else if (*endchar == 'm' || *endchar == 'M')
>> -    num*= 1024L * 1024L;
>> -  else if (*endchar == 'g' || *endchar == 'G')
>> -    num*= 1024L * 1024L * 1024L;
>> -  else if (*endchar)
>> -  {
>> +  num*= eval_num_suffix(endchar, error);
>> +  if (*error)
>>      fprintf(stderr,
>>             "Unknown suffix '%c' used for variable '%s' (value '%s')\n",
>>             *endchar, option_name, argument);
>> +  DBUG_RETURN(num);
>> +}
>> +
>> +/*
>> +  function: eval_num_suffix_ull
>> +
>> +  Transforms a number with a suffix to positive Integer. Suffix can
>> +  be k|K for kilo, m|M for mega or g|G for giga.
>> +*/
>> +
>> +static ulonglong eval_num_suffix_ull(char *argument, int *error, char
>> *option_name)
>>
>
> The above line >80 chars, please break it into 2 lines.
>
>
Updated.

> +{
>> +  char *endchar;
>> +  ulonglong num;
>> +  DBUG_ENTER("eval_num_suffix");
>>
>
> Update the function name.
>
Updated.

>
>
>> +
>> +  *error= 0;
>> +  errno= 0;
>> +  num= strtoull(argument, &endchar, 10);
>> +  if (errno == ERANGE)
>> +  {
>> +    my_getopt_error_reporter(ERROR_LEVEL,
>> +                             "Incorrect integer value: '%s'", argument);
>>      *error= 1;
>>      DBUG_RETURN(0);
>>    }
>> +  num*= eval_num_suffix(endchar, error);
>> +  if (*error)
>> +    fprintf(stderr,
>> +           "Unknown suffix '%c' used for variable '%s' (value '%s')\n",
>> +           *endchar, option_name, argument);
>>    DBUG_RETURN(num);
>>  }
>>
>> +
>>  /*
>>    function: getopt_ll
>>
>> @@ -946,7 +990,7 @@ static longlong eval_num_suffix(char *argument, int
>> *error, char *option_name)
>>
>>  static longlong getopt_ll(char *arg, const struct my_option *optp, int
>> *err)
>>  {
>> -  longlong num=eval_num_suffix(arg, err, (char*) optp->name);
>> +  longlong num=eval_num_suffix_ll(arg, err, (char*) optp->name);
>>    return getopt_ll_limit_value(num, optp, NULL);
>>  }
>>
>> @@ -1023,7 +1067,7 @@ longlong getopt_ll_limit_value(longlong num, const
>> struct my_option *optp,
>>
>>  static ulonglong getopt_ull(char *arg, const struct my_option *optp, int
>> *err)
>>  {
>> -  ulonglong num= eval_num_suffix(arg, err, (char*) optp->name);
>> +  ulonglong num= eval_num_suffix_ull(arg, err, (char*) optp->name);
>>    return getopt_ull_limit_value(num, optp, NULL);
>>  }
>>
>>
> Best,
> Nirbhay
>
http://lists.askmonty.org/pipermail/commits/2017-January/010470.html
Regards
sachin