Re: [Maria-developers] [Commits] 98b2a9c: MDEV-4774 Strangeness with max_binlog_stmt_cache_size Settings
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@mariadb.com> 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
Hi Nirbhay! On Fri, Jan 20, 2017 at 7:09 AM, Nirbhay Choubey <nirbhay@mariadb.com> 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@mariadb.com> 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
Hi Sachin, On Fri, Jan 20, 2017 at 3:04 AM, Sachin Setiya <sachin.setiya@mariadb.com> wrote:
Hi Nirbhay!
On Fri, Jan 20, 2017 at 7:09 AM, Nirbhay Choubey <nirbhay@mariadb.com> 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@mariadb.com> 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
.. cut ..
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.
Not exactly. What you currently test is the maximum allowed value '18446744073709547520' for this variable (which is good). But, I would adittionally like to have it ULL_MAX (& +/1) which is 18446744073709551615. Considering this change, I have no more remarks. Best, Nirbhay
Hi Nirbhay! On Fri, Jan 20, 2017 at 6:39 PM, Nirbhay Choubey <nirbhay@mariadb.com> wrote:
Hi Sachin,
On Fri, Jan 20, 2017 at 3:04 AM, Sachin Setiya <sachin.setiya@mariadb.com> wrote:
Hi Nirbhay!
On Fri, Jan 20, 2017 at 7:09 AM, Nirbhay Choubey <nirbhay@mariadb.com> 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@mariadb.com
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
.. cut ..
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.
Not exactly. What you currently test is the maximum allowed value '18446744073709547520' for this variable (which is good). But, I would adittionally like to have it ULL_MAX (& +/1) which is 18446744073709551615.
Sorry Got it. Changed.
Considering this change, I have no more remarks.
Best, Nirbhay
-- Regards sachin
participants (2)
-
Nirbhay Choubey -
Sachin Setiya