maria-developers team mailing list archive

[Nirbhay Choubey <nirbhay@xxxxxxxxxxx>]

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/


> 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.

+--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/


> +*/
> +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).


> +/*
> +  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.


> @@ -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.


> +{
> +  char *endchar;
> +  ulonglong num;
> +  DBUG_ENTER("eval_num_suffix");
>

Update the function name.


> +
> +  *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