Hi Nirbhay! On Fri, Jan 20, 2017 at 7:09 AM, Nirbhay Choubey 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 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