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
Nirbhay Choubey
Sachin Setiya