Hi, Sanja! Summary: The implementation looks very much ok, all comments below are about details, nothing big. Well done! On Jul 14, sanja@mariadb.com wrote:
revision-id: 686761fbb0fc36e4fa781b68aba24af21599822d parent(s): 302bf7c4664b904482ecc133476e822d497b114d committer: Oleksandr Byelkin branch nick: server
could you update your post-commit trigger? thanks.
timestamp: 2015-07-14 00:11:25 +0200 message:
MDEV-6066: Merge new defaults from 5.6 and 5.7 (part 1 (no hash serch) v2)
diff --git a/mysql-test/extra/binlog_tests/mysqlbinlog_row_engine.inc b/mysql-test/extra/binlog_tests/mysqlbinlog_row_engine.inc index 95440ab..c072d50 100644 --- a/mysql-test/extra/binlog_tests/mysqlbinlog_row_engine.inc +++ b/mysql-test/extra/binlog_tests/mysqlbinlog_row_engine.inc @@ -16,6 +16,7 @@ #
--source include/have_log_bin.inc +set sql_mode="";
please split this big commit in 'new defaults' and 'autoset' commits.
SET NAMES 'utf8'; #SHOW VARIABLES LIKE 'character_set%'; diff --git a/mysql-test/r/analyze_stmt_privileges.result b/mysql-test/r/analyze_stmt_privileges.result index d382b0b..05a35e7 100644 --- a/mysql-test/r/analyze_stmt_privileges.result +++ b/mysql-test/r/analyze_stmt_privileges.result @@ -7,7 +7,7 @@ use db; create table t1 (i int, c varchar(8)); insert into t1 values (1,'foo'),(2,'bar'),(3,'baz'),(4,'qux'); create view v1 as select * from t1 where i > 1; -grant ALL on db.v1 to u1@localhost; +set statement sql_mode="" for grant ALL on db.v1 to u1@localhost;
heh. I think it would've been less confusing to put here an explicit CREATE USER statement
connect con1,localhost,u1,,; select * from db.t1; ERROR 42000: SELECT command denied to user 'u1'@'localhost' for table 't1' diff --git a/mysql-test/r/blackhole_plugin.result b/mysql-test/r/blackhole_plugin.result index 4ef9fa0..ab106f8 100644 --- a/mysql-test/r/blackhole_plugin.result +++ b/mysql-test/r/blackhole_plugin.result @@ -1,7 +1,9 @@ +set sql_mode="";
you could've put --error 1286 instead.
CREATE TABLE t1(a int) ENGINE=BLACKHOLE; Warnings: Warning 1286 Unknown storage engine 'BLACKHOLE' Warning 1266 Using storage engine MyISAM for table 't1' +set sql_mode=default; DROP TABLE t1; INSTALL PLUGIN blackhole SONAME 'ha_blackhole.so'; INSTALL PLUGIN BLACKHOLE SONAME 'ha_blackhole.so'; diff --git a/mysql-test/r/change_user.result b/mysql-test/r/change_user.result index 18c53a5..bd87398 100644 --- a/mysql-test/r/change_user.result +++ b/mysql-test/r/change_user.result @@ -1,3 +1,5 @@ +set sql_mode=""; +set global secure_auth=0; grant select on test.* to test_nopw; grant select on test.* to test_oldpw identified by password "09301740536db389"; grant select on test.* to test_newpw identified by "newpw";
you could've used "create user" instead of "grant select on test.* to" and that would work without resetting sql_mode
diff --git a/mysql-test/r/concurrent_innodb_safelog.result b/mysql-test/r/concurrent_innodb_safelog.result index 24a84af..03d20f4 100644 --- a/mysql-test/r/concurrent_innodb_safelog.result +++ b/mysql-test/r/concurrent_innodb_safelog.result @@ -1,4 +1,5 @@ SET GLOBAL TRANSACTION ISOLATION LEVEL REPEATABLE READ; +SET SQL_MODE="";
same. basically I'd suggest to fix tests instead of setting sql_mode="". i suspect it's mostly adding create user or replacing grant with create user. sometimes it's about removing engine=xxx and you're doing that already.
SELECT @@global.tx_isolation; @@global.tx_isolation REPEATABLE-READ diff --git a/mysql-test/r/func_compress.result b/mysql-test/r/func_compress.result index 9fde006..4763d87 100644 --- a/mysql-test/r/func_compress.result +++ b/mysql-test/r/func_compress.result @@ -1,3 +1,4 @@ +set global max_allowed_packet=1048576;
again, I'd rather fix the test
select @test_compress_string:='string for test compress function aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa '; @test_compress_string:='string for test compress function aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa ' string for test compress function aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa diff --git a/mysql-test/r/host_cache_size_functionality.result b/mysql-test/r/host_cache_size_functionality.result index e7f9e09..ffa8d71 100644 --- a/mysql-test/r/host_cache_size_functionality.result +++ b/mysql-test/r/host_cache_size_functionality.result @@ -9,7 +9,7 @@ SELECT COUNT(@@GLOBAL.Host_Cache_Size) set @Default_host_cache_size=128; select @@global.Host_Cache_Size=@Default_host_cache_size; @@global.Host_Cache_Size=@Default_host_cache_size -1 +0 1 Expected
Now that looks weird.
'#---------------------WL6372_VAR_6_02----------------------#' # Restart server with Host_Cache_Size 1 diff --git a/mysql-test/r/index_intersect.result b/mysql-test/r/index_intersect.result index 1337c3f..edc52db 100644 --- a/mysql-test/r/index_intersect.result +++ b/mysql-test/r/index_intersect.result @@ -1035,7 +1032,7 @@ EXPLAIN SELECT * FROM t1 WHERE (f1 < 535 OR f1 > 985) AND ( f4='r' OR f4 LIKE 'a%' ) ; id select_type table type possible_keys key key_len ref rows Extra -1 SIMPLE t1 range PRIMARY,f4 f4 35 NULL # Using index condition; Using where +1 SIMPLE t1 index_merge PRIMARY,f4 PRIMARY,f4 4,39 NULL # Using sort_intersect(PRIMARY,f4); Using where
why?
SELECT * FROM t1 WHERE (f1 < 535 OR f1 > 985) AND ( f4='r' OR f4 LIKE 'a%' ) ; f1 f4 f5 diff --git a/mysql-test/r/mysqld--help.result b/mysql-test/r/mysqld--help.result index fc22af6..1f2a8cf 100644 --- a/mysql-test/r/mysqld--help.result +++ b/mysql-test/r/mysqld--help.result @@ -890,6 +890,7 @@ The following options may be given as the first argument: write privileges to the mysql.user table. --secure-auth Disallow authentication for accounts that have old (pre-4.1) passwords + (Defaults to on; use --skip-secure-auth to disable.)
You know what, it would be nice if the user would know what options support the --autoset prefix. I think here's a good place for it, the help for back_log can end with gets very many connection requests in a very short time (Automatically configured unless set explicitly) Of course, this line should be automatically generated by my_print_help(), not manually added to the help text of every autoset variable.
--secure-file-priv=name Limit LOAD DATA, SELECT ... OUTFILE, and LOAD_FILE() to files within specified directory diff --git a/mysql-test/r/selectivity.result b/mysql-test/r/selectivity.result index 620bdc6..4238359 100644 --- a/mysql-test/r/selectivity.result +++ b/mysql-test/r/selectivity.result @@ -1298,10 +1298,10 @@ Note 1003 select `test`.`t1`.`a` AS `a`,`test`.`t1`.`b` AS `b`,`test`.`t2`.`c` A select * from t1, t2, t1 as t3 where t1.b=t2.c and t2.d=t3.a and t3.b<5 and t1.a < 2000; a b c d a b -1495 89366 89366 28296 28296 3 -961 24512 24512 85239 85239 4 1063 89366 89366 28296 28296 3 +1495 89366 89366 28296 28296 3 221 56120 56120 28296 28296 3 +961 24512 24512 85239 85239 4
why?
set optimizer_use_condition_selectivity=3; explain extended select * from t1, t2, t1 as t3 @@ -1315,10 +1315,10 @@ Note 1003 select `test`.`t1`.`a` AS `a`,`test`.`t1`.`b` AS `b`,`test`.`t2`.`c` A select * from t1, t2, t1 as t3 where t1.b=t2.c and t2.d=t3.a and t3.b<5 and t1.a < 2000; a b c d a b -961 24512 24512 85239 85239 4 -1495 89366 89366 28296 28296 3 1063 89366 89366 28296 28296 3 +1495 89366 89366 28296 28296 3 221 56120 56120 28296 28296 3 +961 24512 24512 85239 85239 4
why?
set optimizer_use_condition_selectivity=@save_optimizer_use_condition_selectivity; drop table t1,t2; set histogram_type=@save_histogram_type; diff --git a/mysql-test/r/selectivity_innodb.result b/mysql-test/r/selectivity_innodb.result index 0acbb46..daf2807 100644 --- a/mysql-test/r/selectivity_innodb.result +++ b/mysql-test/r/selectivity_innodb.result @@ -1308,10 +1308,10 @@ Note 1003 select `test`.`t1`.`a` AS `a`,`test`.`t1`.`b` AS `b`,`test`.`t2`.`c` A select * from t1, t2, t1 as t3 where t1.b=t2.c and t2.d=t3.a and t3.b<5 and t1.a < 2000; a b c d a b -1495 89366 89366 28296 28296 3 -961 24512 24512 85239 85239 4 1063 89366 89366 28296 28296 3 +1495 89366 89366 28296 28296 3 221 56120 56120 28296 28296 3 +961 24512 24512 85239 85239 4
why?
set optimizer_use_condition_selectivity=3; explain extended select * from t1, t2, t1 as t3 @@ -1325,10 +1325,10 @@ Note 1003 select `test`.`t1`.`a` AS `a`,`test`.`t1`.`b` AS `b`,`test`.`t2`.`c` A select * from t1, t2, t1 as t3 where t1.b=t2.c and t2.d=t3.a and t3.b<5 and t1.a < 2000; a b c d a b -961 24512 24512 85239 85239 4 -1495 89366 89366 28296 28296 3 1063 89366 89366 28296 28296 3 +1495 89366 89366 28296 28296 3 221 56120 56120 28296 28296 3 +961 24512 24512 85239 85239 4
why?
set optimizer_use_condition_selectivity=@save_optimizer_use_condition_selectivity; drop table t1,t2; set histogram_type=@save_histogram_type; diff --git a/mysql-test/suite/funcs_1/r/is_columns_is_embedded.result b/mysql-test/suite/funcs_1/r/is_columns_is_embedded.result index c314165..bd49492 100644 --- a/mysql-test/suite/funcs_1/r/is_columns_is_embedded.result +++ b/mysql-test/suite/funcs_1/r/is_columns_is_embedded.result @@ -167,9 +167,9 @@ def information_schema GEOMETRY_COLUMNS MAX_PPR 12 0 NO tinyint NULL NULL 3 0 NU def information_schema GEOMETRY_COLUMNS SRID 13 0 NO smallint NULL NULL 5 0 NULL NULL NULL smallint(5) def information_schema GEOMETRY_COLUMNS STORAGE_TYPE 9 0 NO tinyint NULL NULL 3 0 NULL NULL NULL tinyint(2) def information_schema GLOBAL_STATUS VARIABLE_NAME 1 NO varchar 64 192 NULL NULL NULL utf8 utf8_general_ci varchar(64) -def information_schema GLOBAL_STATUS VARIABLE_VALUE 2 NO varchar 1024 3072 NULL NULL NULL utf8 utf8_general_ci varchar(1024) +def information_schema GLOBAL_STATUS VARIABLE_VALUE 2 NO varchar 2048 6144 NULL NULL NULL utf8 utf8_general_ci varchar(2048) def information_schema GLOBAL_VARIABLES VARIABLE_NAME 1 NO varchar 64 192 NULL NULL NULL utf8 utf8_general_ci varchar(64) -def information_schema GLOBAL_VARIABLES VARIABLE_VALUE 2 NO varchar 1024 3072 NULL NULL NULL utf8 utf8_general_ci varchar(1024) +def information_schema GLOBAL_VARIABLES VARIABLE_VALUE 2 NO varchar 2048 6144 NULL NULL NULL utf8 utf8_general_ci varchar(2048)
why?
def information_schema INDEX_STATISTICS INDEX_NAME 3 NO varchar 192 576 NULL NULL NULL utf8 utf8_general_ci varchar(192) def information_schema INDEX_STATISTICS ROWS_READ 4 0 NO bigint NULL NULL 19 0 NULL NULL NULL bigint(21) def information_schema INDEX_STATISTICS TABLE_NAME 2 NO varchar 192 576 NULL NULL NULL utf8 utf8_general_ci varchar(192) diff --git a/mysql-test/suite/perfschema/r/stage_mdl_global.result b/mysql-test/suite/perfschema/r/stage_mdl_global.result index 1a6f51a..de5df8f 100644 --- a/mysql-test/suite/perfschema/r/stage_mdl_global.result +++ b/mysql-test/suite/perfschema/r/stage_mdl_global.result @@ -6,6 +6,8 @@ user1 statement/sql/flush flush tables with read lock username event_name nesting_event_type username event_name nesting_event_type user1 stage/sql/init STATEMENT +user1 stage/sql/Waiting for query cache lock STATEMENT +user1 stage/sql/init STATEMENT
why?
user1 stage/sql/query end STATEMENT user1 stage/sql/closing tables STATEMENT user1 stage/sql/freeing items STATEMENT diff --git a/mysys/my_getopt.c b/mysys/my_getopt.c index 88fa3d4..1d32833 100644 --- a/mysys/my_getopt.c +++ b/mysys/my_getopt.c @@ -47,14 +47,15 @@ static char *check_struct_option(char *cur_arg, char *key_name); order of their arguments must correspond to each other. */ static const char *special_opt_prefix[]= -{"skip", "disable", "enable", "maximum", "loose", 0}; +{"skip", "disable", "enable", "maximum", "loose", "autoset", 0};
So, I'm not very happy with the --autoset- prefix. Because it cannot be used from the SQL. In SQL we have to use SET variable=AUTO; or something like that. And it would be good to use the same approach on the command line and from SQL. On the other hand, =AUTO has its problems too - it conflicts with the AUTO value of the enum/set variables. I don't have a good answer to this :(
static const uint special_opt_prefix_lengths[]= -{ 4, 7, 6, 7, 5, 0}; +{ 4, 7, 6, 7, 5, 7, 0}; enum enum_special_opt -{ OPT_SKIP, OPT_DISABLE, OPT_ENABLE, OPT_MAXIMUM, OPT_LOOSE}; +{ OPT_SKIP, OPT_DISABLE, OPT_ENABLE, OPT_MAXIMUM, OPT_LOOSE, OPT_AUTOSET};
char *disabled_my_option= (char*) "0"; char *enabled_my_option= (char*) "1"; +char *autoset_my_option= (char*) "";
/* This is a flag that can be set in client programs. 0 means that @@ -458,6 +462,36 @@ int handle_options(int *argc, char ***argv, } argument= optend; } + else if (option_is_autoset) + { + if (optend) + { + my_getopt_error_reporter(ERROR_LEVEL, + "%s: automatic setup request of " + "option '--%s' cannot take an argument", + my_progname, optp->name);
heh, another argument agains --autoset- prefix. With the =AUTO value one cannot possibly have this error. So, perhaps, I'm *slighty* leaning towards the =AUTO vs --autoset.
+ + DBUG_RETURN(EXIT_NO_ARGUMENT_ALLOWED); + } + /* + We support automatic setup only via get_one_option and only for + marked options. + */ + if (!get_one_option || + !(optp->var_type & GET_AUTO)) + { + my_getopt_error_reporter(option_is_loose ? + WARNING_LEVEL : ERROR_LEVEL, + "%s: automatic setup request is " + "unsupported by option '--%s'", + my_progname, optp->name); + if (!option_is_loose) + return EXIT_ARGUMENT_INVALID; + continue; + } + else + argument= autoset_my_option; + } else if (optp->arg_type == REQUIRED_ARG && !optend) { /* Check if there are more arguments after this one, diff --git a/sql/set_var.cc b/sql/set_var.cc index c65ca3d..2251c36 100644 --- a/sql/set_var.cc +++ b/sql/set_var.cc @@ -1075,7 +1076,7 @@ int fill_sysvars(THD *thd, TABLE_LIST *tables, COND *cond) // NUMERIC_MAX_VALUE // NUMERIC_BLOCK_SIZE bool is_unsigned= true; - switch (var->option.var_type) + switch (vartype)
ah, yes. thanks.
{ case GET_INT: case GET_LONG: @@ -1176,3 +1177,20 @@ void mark_sys_var_value_origin(void *ptr, enum sys_var::where here) DBUG_ASSERT(found); // variable must have been found }
+enum sys_var::where get_sys_var_value_origin(void *ptr) +{ + DBUG_ASSERT(!mysqld_server_started); // only to be used during startup + + for (uint i= 0; i < system_variable_hash.records; i++) + { + sys_var *var= (sys_var*) my_hash_element(&system_variable_hash, i); + if (var->option.value == ptr) + { + return var->value_origin; //first match + } + } + + DBUG_ASSERT(1); // variable must have been found
you mean DBUG_ASSERT(0); here
+ return sys_var::CONFIG; +} + diff --git a/sql/mysqld.cc b/sql/mysqld.cc index 43a3f0e..21a1198 100644 --- a/sql/mysqld.cc +++ b/sql/mysqld.cc @@ -4316,6 +4316,27 @@ static int init_common_variables() } #endif /* HAVE_SOLARIS_LARGE_PAGES */
+ + /* Fix host_cache_size. */ + if (IS_SYSVAR_AUTOSIZE(&host_cache_size)) + { + if (max_connections <= 628 - 128) + SYSVAR_AUTOSIZE(host_cache_size, 128 + max_connections); + else if (max_connections <= ((ulong)(2000 - 628)) * 20 + 500) + SYSVAR_AUTOSIZE(host_cache_size, 628 + ((max_connections - 500) / 20)); + else + SYSVAR_AUTOSIZE(host_cache_size, 2000); + } + + /* Fix back_log */ + if (back_log == 0 || IS_SYSVAR_AUTOSIZE(&back_log))
why back_log==0 ?
+ { + if ((900 - 50) * 5 >= max_connections) + SYSVAR_AUTOSIZE(back_log, (50 + max_connections / 5)); + else + SYSVAR_AUTOSIZE(back_log, 900); + } + /* connections and databases needs lots of files */ { uint files, wanted_files, max_open_files; diff --git a/sql/sys_vars.ic b/sql/sys_vars.ic index 7c0359a..b04bdb5 100644 --- a/sql/sys_vars.ic +++ b/sql/sys_vars.ic @@ -226,6 +227,7 @@ class Sys_var_integer: public sys_var typedef Sys_var_integer<int, GET_INT, SHOW_SINT> Sys_var_int; typedef Sys_var_integer<uint, GET_UINT, SHOW_UINT> Sys_var_uint; typedef Sys_var_integer<ulong, GET_ULONG, SHOW_ULONG> Sys_var_ulong; +typedef Sys_var_integer<ulong, (GET_ULONG|GET_AUTO), SHOW_ULONG> Sys_var_aulong;
Hmm, why did you make it a new type? I think the *type* of the variable doesn't depend on the auto-set behavior. The type defines storage size, how to print the value, etc. While auto-set is just a behavior modifier. I'd rather put it in flags: - READ_ONLY GLOBAL_VAR(back_log), CMD_LINE(REQUIRED_ARG), + AUTO_SET READ_ONLY GLOBAL_VAR(back_log), CMD_LINE(REQUIRED_ARG),
typedef Sys_var_integer<ha_rows, GET_HA_ROWS, SHOW_HA_ROWS> Sys_var_harows; typedef Sys_var_integer<ulonglong, GET_ULL, SHOW_ULONGLONG> Sys_var_ulonglong; typedef Sys_var_integer<long, GET_LONG, SHOW_SLONG> Sys_var_long; diff --git a/sql/sys_vars.cc b/sql/sys_vars.cc index 7a7b9d1..6d90305 100644 --- a/sql/sys_vars.cc +++ b/sql/sys_vars.cc @@ -360,12 +360,12 @@ static Sys_var_mybool Sys_automatic_sp_privileges( GLOBAL_VAR(sp_automatic_privileges), CMD_LINE(OPT_ARG), DEFAULT(TRUE));
-static Sys_var_ulong Sys_back_log( +static Sys_var_aulong Sys_back_log( "back_log", "The number of outstanding connection requests " "MySQL can have. This comes into play when the main MySQL thread "
s/MySQL/MariaDB/ I suppose
"gets very many connection requests in a very short time", READ_ONLY GLOBAL_VAR(back_log), CMD_LINE(REQUIRED_ARG), - VALID_RANGE(1, 65535), DEFAULT(150), BLOCK_SIZE(1)); + VALID_RANGE(0, 65535), DEFAULT(150), BLOCK_SIZE(1));
static Sys_var_charptr Sys_basedir( "basedir", "Path to installation directory. All paths are " @@ -2751,16 +2743,38 @@ static bool check_query_cache_type(sys_var *self, THD *thd, set_var *var) my_error(ER_QUERY_CACHE_IS_DISABLED, MYF(0)); return true; } - if (var->type != OPT_GLOBAL && - global_system_variables.query_cache_type == 0 && - var->value->val_int() != 0) + + if (var->type != OPT_GLOBAL && global_system_variables.query_cache_type == 0) { - my_error(ER_QUERY_CACHE_IS_GLOBALY_DISABLED, MYF(0)); - return true; - } + uint value= 0 /*default is off*/; + if (var->value) + { + if (var->value->result_type() == INT_RESULT) + value= var->value->val_int(); + else + { + char buff[STRING_BUFFER_USUAL_SIZE]; + String str(buff, sizeof(buff), system_charset_info), *res; + if (!(res=var->value->val_str(&str))) + return true; + if (res->length() != 3 || + my_toupper(res->charset(), res->ptr()[0]) != 'O' || + my_toupper(res->charset(), res->ptr()[1]) != 'F' || + my_toupper(res->charset(), res->ptr()[2]) != 'F')
eh? query_cache_type is ENUM variable, there is no need to compare string values manually.
+ value= 1; // set to something not 0 + } + }
+ if (value != 0) + { + my_error(ER_QUERY_CACHE_IS_GLOBALY_DISABLED, MYF(0)); + return true; + } + } return false; } + + static bool fix_query_cache_type(sys_var *self, THD *thd, enum_var_type type) { if (type != OPT_GLOBAL) diff --git a/unittest/mysys/CMakeLists.txt b/unittest/mysys/CMakeLists.txt index 8c09a46..6564e09 100644 --- a/unittest/mysys/CMakeLists.txt +++ b/unittest/mysys/CMakeLists.txt @@ -13,7 +13,7 @@ # along with this program; if not, write to the Free Software # Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
-MY_ADD_TESTS(bitmap base64 my_vsnprintf my_atomic my_rdtsc lf my_malloc +MY_ADD_TESTS(bitmap base64 my_vsnprintf my_atomic my_rdtsc lf my_malloc my_getopt
Ah, that's great, thanks! Long overdue.
LINK_LIBRARIES mysys)
MY_ADD_TESTS(ma_dyncol
Regards, Sergei