Hi Nirbhay! On Fri, Jan 20, 2017 at 7:09 AM, Nirbhay Choubey <[email protected]> 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 <[email protected]> > 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
_______________________________________________ Mailing list: https://launchpad.net/~maria-developers Post to : [email protected] Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
