Hi, Sergei! I decide to answer before fixing everything, to be able to get your answer earlier. On 06.08.15 12:01, Sergei Golubchik wrote:
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. OK
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. OK
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 OK
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. OK
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 OK
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. Monty's arguments against was that we will not need to change test if we change default next time. I.e to keep environment for which test was created.
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 OK
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. Yes, it was in Monty's review also, but when I test the result alone it was not changed, I'll check how it is happened.
'#---------------------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? I discussed it, we found nothing, but I'll check. 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?
now result is sorted (and it should be because there is no order in the statement)
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?
The same - sorted result.
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?
The same - sorted result.
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? I'll check.
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 :( IMHO better to have both, but lets add SET...=AUTOSET when it will be requested.
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 YES
+ 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 ? MySQL/OLD way to get automatic value, lest for compatibility.
+ { + 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),
The same - sorted result. the problem was in how macro/constructor made, I did not found the way to do it in flags, I will recheck.
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
yes
"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.
The problem is that it called before processing, and it was bug before to check only numeric value.
+ 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