- developers - lists.mariadb.org

Re: [Maria-developers] [Commits] 686761f: MDEV-6066: Merge new defaults from 5.6 and 5.7 (part 1 (no hash serch) v2)
by Sergei Golubchik 07 Aug '15

07 Aug '15

Hi, Sanja! Summary: The implementation looks very much ok, all comments below are about details, nothing big. Well done! On Jul 14, sanja(a)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

2 2

Re: [Maria-developers] [Commits] 69704b6: MDEV-6887 Optimize sys_vars.sysvar_* tests
by Sergei Golubchik 06 Aug '15

06 Aug '15

Hi, Elena! On Aug 06, elenst(a)montyprogram.com wrote: > revision-id: 69704b6c6f895fa706f83bcd3bcefe267aaf8e57 > parent(s): afd59b575a75ebbc57f71ce2865fdff85e3e233b > committer: Elena Stepanova > branch nick: bb-10.1-elenst > timestamp: 2015-08-06 02:55:43 +0300 > message: > > MDEV-6887 Optimize sys_vars.sysvar_* tests > > diff --git a/mysql-test/suite/sys_vars/inc/sysvars_server.inc b/mysql-test/suite/sys_vars/inc/sysvars_server.inc > index cb06b40..98bffd4 100644 > --- a/mysql-test/suite/sys_vars/inc/sysvars_server.inc > +++ b/mysql-test/suite/sys_vars/inc/sysvars_server.inc > @@ -11,8 +12,9 @@ set sql_mode=ansi_quotes; > # global_value_origin=SQL > set global div_precision_increment=5; > > ---replace_regex /^\/\S+/PATH/ > +--replace_regex /^\/\S+/PATH/ /^[a-zA-Z]:[\\\/]\S+/PATH/ /^BIGINT\b/<INT or BIGINT>/ /^INT\b/<INT or BIGINT>/ /^(?:18446744073709551615\b|4294967295\b)/<32-bit or 64-bit MAX UNSIGNED>/ /^(?:9223372036854775807\b|2147483647\b)/<32-bit or 64-bit MAX SIGNED>/ /^(?:18446744073709547520\b|4294963200\b)/<32-bit or 64-bit MAX UNSIGNED ADJUSTED>/ /^(?:9223372036853727232\b|2146435072\b)/<32-bit or 64-bit MAX SIGNED ADJUSTED>/ Aren't you removing too much with these replacements? It's kinda good to get rid of 32bit diff files (as they aren't easy to update), but I wouldn't replace every INT or BIGINT with <INT or BIGINT>. May be, move them to a separate select, like let type=BIGINT; if (32-bit) { let type=INT; } --replace $type <INT-or-BIGINT> select * from information_schema.system_variables where variable_name in (list of ulong variables); and something similar for other replacements. In fact, there's obscure feature of let $repl=/foo/bar/ /abc/def; replace_regex $repl; so you can have sysvar32bitrepl.inc that will set up $replace_regexes variable: let type=BIGINT; let max=18446744073709551615; if (32-bit) { let type=INT; let max=4294967295; } let replace_regexes=/$type/INT or BIGINT/ /$max/<32-bit or 64-bit MAX UNSIGNED>/; Then in every sysvar*.test file you can do, like --source sysvar32bitrepl --replace_regex $replace_regexes select * from information_schema.system_variables where variable_name in (list of ulong variables); Regards, Sergei

2 2

Re: [Maria-developers] MDEV-8389 patch review
by Sergey Petrunia 06 Aug '15

06 Aug '15

Hi Diwas, On Wed, Aug 05, 2015 at 08:47:28PM +0530, Diwas Joshi wrote: > I have removed the errors from previous patch, I only got those errors when > i compiled with valgrind. Everything seems to be working fine now. Also I > have fixed the test case for select * from mysql.proc. Also added a > function that removes the tables after they have been used. I have added > the errors that we discussed about and also test cases corresponding to > these errors. Please review. > The big problems in the patch are: 1. check_and_process_table_functions() is at the wrong place. I debug a query, "SELECT * FROM tbl" , where 'tbl' is a regular table. I see check_and_process_table_functions() invoke sp_find_routine() for "test.tbl". Why does this happen? The query makes it clear that the table is not a table function. This is not acceptable. 2. Temporary table 2.1 The temporary table that is created uses InnoDB storage engine. This doesn't follow any other kind of temporary table. When execution engine creates temporary tables (e.g. for VIEWs, or GROUP BY), these temporary tables are either Aria or MyISAM tables, depending on the server settings. We should follow that. 2.2 Temporary table is created by building the "CREATE TEMPORARY TABLE (...)" string and passing it to the parser. Did you try looking into creating the temp. table directly from the data strucutres you have? No other code has to build "CREATE TABLE" statements and then parse them back. I suspect Table Functions don't need to do this either. The rest of the feedback is below: > diff --git a/mysql-test/t/sp-error.test b/mysql-test/t/sp-error.test > index ccf5b98..2ad4144 100644 > --- a/mysql-test/t/sp-error.test > +++ b/mysql-test/t/sp-error.test > @@ -3846,7 +3846,23 @@ DROP FUNCTION f1; > DROP FUNCTION f2; > DROP TABLE t1; > > ---error 1235 > -SELECT * FROM f1('xyz'); > ---error 1064 > -SELECT * FROM t1, f1('xyz',t1.a); > +delimiter |; > +CREATE FUNCTION f1() RETURNS INTEGER > +BEGIN > + RETURN 1; > +END| > +delimiter ;| > +--error 1979 Please use error names, not error numbers. you can write --error ER_SP_SCALAR_VALUE_EXPECTED. > +SELECT * FROM f1(); > +DROP FUNCTION f1; > + > +delimiter |; > +CREATE FUNCTION f1(a INT, b VARCHAR(11)) > +RETURNS TABLE t1(id INT, name VARCHAR(11)) > +BEGIN > +INSERT INTO t1 SELECT id, name FROM t2 WHERE id = a; > +END| > +delimiter ;| > +--error 1978 Same as above. > +SELECT f1(1,'AA'); > +DROP FUNCTION f1; > diff --git a/mysql-test/t/sp.test b/mysql-test/t/sp.test > index 5c42df0..45db6a3 100644 > --- a/mysql-test/t/sp.test > +++ b/mysql-test/t/sp.test > @@ -9398,6 +9398,6 @@ END| > DELIMITER ;| > > --vertical_results > -SELECT * FROM mysql.proc WHERE db = database() and name = 'f1'; > - > +SELECT db, name, type, specific_name, language, sql_data_access, is_deterministic, security_type, param_list, returns, body, definer, sql_mode, comment, character_set_client, collation_connection, db_collation, body_utf8 FROM mysql.proc WHERE db = database() and name = 'f1'; The change is ok. Please also format the query so that its width is limited 80 chars. > +SELECT * FROM f1(10,'XYZ'); > DROP FUNCTION f1; > diff --git a/sql/item_func.cc b/sql/item_func.cc > index 3e78be8..ad18e2e 100644 > --- a/sql/item_func.cc > +++ b/sql/item_func.cc > @@ -6574,7 +6574,12 @@ void my_missing_function_error(const LEX_STRING &token, const char *func_name) > context->process_error(thd); > DBUG_RETURN(TRUE); > } > - > + > + if (m_sp->m_type == TYPE_ENUM_TABLE){ > + my_error(ER_SP_SCALAR_VALUE_EXPECTED, MYF(0)); > + context->process_error(thd); > + DBUG_RETURN(TRUE); > + } > /* > A Field need to be attached to a Table. > Below we "create" a dummy table by initializing > @@ -6588,7 +6593,7 @@ void my_missing_function_error(const LEX_STRING &token, const char *func_name) > dummy_table->copy_blobs= TRUE; > share->table_cache_key = empty_name; > share->table_name = empty_name; > - > + Please don't make dummy changes like this. > if (!(sp_result_field= m_sp->create_result_field(max_length, name, > dummy_table))) > { > diff --git a/sql/sql_parse.cc b/sql/sql_parse.cc > index 2998972..6f94720 100644 > --- a/sql/sql_parse.cc > +++ b/sql/sql_parse.cc > @@ -2467,6 +2467,8 @@ static bool do_execute_sp(THD *thd, sp_head *sp) > /* should be assigned after making first tables same */ > all_tables= lex->query_tables; > /* set context for commands which do not use setup_tables */ > + if (lex->sql_command == SQLCOM_SELECT) > + check_and_process_table_functions(thd, all_tables); Why limit table function uses to SELECTs ? A user can write an INSERT.. SELECT statement INSERT INTO table SELECT * FROM table_func('argument'); or a multi-table UPDATE statement: UPDATE regular_table, table_func('arg') SET regular_table.col= ...; In general, there should not be a limit. I think, it's ok to introduce the limit within the scope of this MDEV entry. In that case, you need to add a comment with 'TODO-DiwasJoshi:...' saying that the limitation is temporary. > select_lex-> > context.resolve_in_table_list_only(select_lex-> > table_list.first); > @@ -5697,7 +5699,8 @@ static bool do_execute_sp(THD *thd, sp_head *sp) > thd->mdl_context.release_transactional_locks(); > } > #endif /* WITH_WSREP */ > - > + if (lex->sql_command == SQLCOM_SELECT) > + clear_table_functions(thd, all_tables); This looks like an odd place to do cleanups. Let's get back to this once other issues are resolved. > DBUG_RETURN(res || thd->is_error()); > } > > @@ -9222,3 +9225,185 @@ CHARSET_INFO *find_bin_collation(CHARSET_INFO *cs) > } > return cs; > } > + > +/* > + @brief > + Check for table functions and create corresponding tables to be used in query. > + > + @param thd Thread handle > + @param table_list A list of tables being processed in the query. > + > + @return > + False on success > + @retval > + TRUE on error > +*/ > +bool check_and_process_table_functions(THD *thd, TABLE_LIST *table_list) > +{ > + TABLE_LIST *tables = table_list; > + int err= 0; > + while(tables) > + { > + if (tables->table_name == NULL) > + { > + tables = tables->next_local; > + continue; > + } > + sp_head *sp; > + sp_name *name; > + LEX_STRING lex_db; > + LEX_STRING lex_name; > + lex_db.length= strlen(tables->db); > + lex_name.length= strlen(tables->table_name); > + lex_db.str= thd->strmake(tables->db, lex_db.length); > + lex_name.str= thd->strmake(tables->table_name, lex_name.length); > + name= new sp_name(lex_db, lex_name, true); > + name->init_qname(thd); > + int err=0; > + > + if (!err && (sp = sp_find_routine(thd,TYPE_ENUM_FUNCTION, name, > + &thd->sp_proc_cache, FALSE))) //check if the table is a table-valued function. > + { > + if (sp->m_type == TYPE_ENUM_FUNCTION) > + my_error(ER_SP_TABLE_VALUE_EXPECTED, MYF(0)); > + LEX *old_lex= thd->lex, newlex; > + thd->lex= &newlex; > + ulonglong sql_mode= 0 , saved_mode= thd->variables.sql_mode; > + String defstr; //query in defstr will be parsed to create temporary table. > + defstr.append(STRING_WITH_LEN("CREATE TEMPORARY TABLE ")); > + defstr.append(tables->table_name); > + defstr.append(STRING_WITH_LEN(" (")); > + List_iterator_fast<Create_field> it(sp->m_cols_list); > + > + TABLE table; > + TABLE_SHARE share; > + bzero((char*) &table, sizeof(table)); > + bzero((char*) &share, sizeof(share)); > + table.in_use= thd; > + table.s = &share; > + Create_field *cols_field; > + cols_field= it++; > + while (cols_field) > + { > + uint unused1= 0; > + sp_prepare_create_field(thd, cols_field); > + prepare_create_field(cols_field, &unused1, HA_CAN_GEOMETRY); > + uint field_length; > + Field *field; > + String tmp_result(512); > + field_length= cols_field->length; > + cols_field->flags= 0; > + field= make_field(table.s, /* TABLE_SHARE ptr */ > + (uchar*) 0, /* field ptr */ > + field_length, /* field [max] length */ > + (uchar*) "", /* null ptr */ > + 0, /* null bit */ > + cols_field->pack_flag, > + cols_field->sql_type, > + cols_field->charset, > + cols_field->geom_type, cols_field->srid, > + Field::NONE, /* unreg check */ > + cols_field->interval, > + cols_field->field_name); > + field->vcol_info= cols_field->vcol_info; > + field->stored_in_db= cols_field->stored_in_db; > + if (field) > + field->init(&table); > + defstr.append(cols_field->field_name); > + defstr.append(STRING_WITH_LEN(" ")); > + field->sql_type(tmp_result); > + defstr.append(tmp_result); > + cols_field= it++; > + if(cols_field) > + defstr.append(STRING_WITH_LEN(", ")); > + delete field; > + } > + defstr.append(STRING_WITH_LEN(")")); > + newlex.create_info.DDL_options_st::init(); > + newlex.current_select= NULL; > + Parser_state parser_state; > + thd->variables.sql_mode= sql_mode; > + err= parser_state.init(thd, defstr.c_ptr_safe(), defstr.length()); > + if (!err) { > + lex_start(thd); > + err= parse_sql(thd, &parser_state, NULL, TRUE) || thd->lex == NULL; > + } > + if (!err) > + { > + Table_specification_st create_info(thd->lex->create_info); > + Alter_info alter_info(thd->lex->alter_info, thd->mem_root); > + thd->lex->create_info.default_table_charset= NULL; > + SELECT_LEX *select_lex= &thd->lex->select_lex; > + TABLE_LIST *create_table= select_lex->table_list.first; > + if (append_file_to_dir(thd, &create_info.data_file_name, > + create_table->table_name) || > + append_file_to_dir(thd, &create_info.index_file_name, > + create_table->table_name)) > + { > + err= 1; > + } > + if (!(create_info.used_fields & HA_CREATE_USED_ENGINE)) > + create_info.use_default_db_type(thd); > + > + err= mysql_create_table(thd, create_table, &create_info, &alter_info); > + thd->variables.option_bits|= OPTION_KEEP_LOG; > + } > + thd->variables.sql_mode= saved_mode; > + lex_end(thd->lex); > + thd->lex= old_lex; > + } > + tables = tables->next_local; > + } > + if (!err) > + close_thread_tables(thd); > + return err; > +} > + > +/* > + @brief > + Drop tables created for table functions. > + > + @param thd Thread handle > + @param table_list A list of tables being processed in the query. > + > + @return > + False on success > + @retval > + TRUE on error > +*/ > +bool clear_table_functions(THD *thd, TABLE_LIST *table_list) > +{ > + TABLE_LIST *tables = table_list; > + int err= 0; > + while(tables) > + { > + if (tables->table_name == NULL) > + { > + tables = tables->next_local; > + continue; > + } > + sp_head *sp; > + sp_name *name; > + LEX_STRING lex_db; > + LEX_STRING lex_name; > + lex_db.length= strlen(tables->db); > + lex_name.length= strlen(tables->table_name); > + lex_db.str= thd->strmake(tables->db, lex_db.length); > + lex_name.str= thd->strmake(tables->table_name, lex_name.length); > + name= new sp_name(lex_db, lex_name, true); > + name->init_qname(thd); > + int err=0; > + if (!err && (sp = sp_find_routine(thd,TYPE_ENUM_FUNCTION, name, > + &thd->sp_proc_cache, FALSE)) && This function is supposed to just close the temporary tables. Why does it need to search for SPs? > + sp->m_type== TYPE_ENUM_TABLE) //check if the table is a table-valued function. > + { > + if(tables->table) > + close_temporary_table(thd,tables->table,TRUE,TRUE); > + if (thd->is_error()) > + err= 1; > + tables->table= NULL; > + } > + tables= tables->next_local; > + } > + return err; > +} BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://s.petrunia.net/blog

1 0

[Maria-developers] Best way of starting multi-source connections with active slave.
by Giuseppe Maxia 06 Aug '15

06 Aug '15

Hi, I am setting up multi-source replication with four nodes, where nodes 1, 2, and 3 are masters and node 4 is the slave of the first 3. The slave is *active*, meaning that it has a binary log and runs transactions of its own. All is well if I run 'reset master' in all nodes before setting up replication. CHANGE MASTER 'node1' TO master_host='host1', master_port=3306, master_user='some_slave', master_password='some_slave', master_use_gtid=current_pos; CHANGE MASTER 'node2' TO master_host='host2', master_port=3306, master_user='some_slave', master_password='some_slave', master_use_gtid=current_pos; CHANGE MASTER 'node3' TO master_host='host3', master_port=3306, master_user='some_slave', master_password='some_slave', master_use_gtid=current_pos; start all slaves; However, if host 4 has a binary log and has logged one or more transactions before starting replication, we get a conflict. Got fatal error 1236 from master when reading data from binary log: 'Error: connecting slave requested to start from GTID 0-104-1, which is not in the master's binlog. Since the master's binlog contains GTIDs with higher sequence numbers, it probably means that the slave has diverged due to executing extra erroneous transactions' I get the same result if I use 'slave_pos' instead of 'current_pos'. Notice that the changes in the slave do not conflict with anything that happens in the masters. For example: #1 node4 (fan-in slave): create user distribution_user identified by 'something'; #2 node4 CHANGE MASTER ... #3 node4 start all slaves #4 node1 (master): create table t1 #5 node2 (master): create table t2 #6 node3 (master): create table t3 This sequence of events fails. If I execute event #1 after #2 and #3, then all works as expected. Or it all works if I say master_use_gtid=no, but that would defeat the purpose of having GTIDs, or am I missing something? Is there a way of enabling multi-source replication with the slave having non-conflicting transactions already logged? Thanks Giuseppe

3 3

Re: [Maria-developers] 3eceb51: MDEV-7990: ERROR 1526 when procedure executed for second time ALTER TABLE partition ... pMAX values less than MAXVALUE
by Sergei Golubchik 05 Aug '15

05 Aug '15

Hi, Sanja! On May 06, sanja(a)mariadb.com wrote: > revision-id: 3eceb51e697d5a514e70c374a06e7eef6b5a37a0 > parent(s): aa5095627e2619bdad7916d33d1016802a84a9e1 > committer: Oleksandr Byelkin > branch nick: server > timestamp: 2015-05-06 13:19:22 +0200 > message: > > MDEV-7990: ERROR 1526 when procedure executed for second time ALTER > TABLE partition ... pMAX values less than MAXVALUE > > Added cleaning partition data which are saved in LEX structure to make > possible correct re-execution of a SP or PS using the code. No, this is wrong. See prep_alter_part_table(). The first thing it does it to copy thd->lex->part_info to thd->work_part_info. It tries to do a deep copy to be able to modify it freely, but apparently the copy isn't deep enough. I see two options 1. Do a deeper copy. Duplicate all value lists. 2. Make part_column_list_val elements immutable. That would be preferred, of course, but I'm not sure it's possible. Regards, Sergei

2 2

Re: [Maria-developers] MDEV-8389 patch review
by Sergey Petrunia 03 Aug '15

03 Aug '15

On Sun, Aug 02, 2015 at 08:51:44PM +0530, Diwas Joshi wrote: > So what would be the expected result of query select f1(1,'aa'). since it > will be a table shouldn't we use select * from f1(1,'aa'). I know it > shouldn't crash but what should be the output of that query? > My first guess is that since we are storing this is as functions too(as we > decided earlier) it is crashing so we'll have to check for table functions > in the execution cycle of functions Agree. When a table function is invoked in the context where a scalar value is expected, the statement should fail with an error (Please look in sql/share/errmsg.txt and pick an error code. If none works, please add a new error code/message). However, this is not what I wanted to complain about (I pasted the wrong example). Let's consider a testcase where a table function is used in the scalar context (and should return an empty table): == example starts == delimiter | CREATE FUNCTION f1(param1 VARCHAR(11)) RETURNS TABLE t1(name VARCHAR(11)) deterministic BEGIN INSERT INTO t1 SELECT a FROM t2 WHERE id = param1; END| DELIMITER ; select 1 from f1('aaa'); == example ends == This crashes for me, too. The crash is in your function: Program received signal SIGSEGV, Segmentation fault. [Switching to Thread 0x7ffff7eb8700 (LWP 12653)] 0x0000555555b4c56b in Object_creation_ctx::set_n_backup (this=0xb, thd=0x55555ac4aeb0) at /home/psergey/dev-git/10.1-gsoc-repo/sql/table.cc:83 (gdb) wher #0 0x0000555555b4c56b in Object_creation_ctx::set_n_backup (this=0xb, thd=0x55555ac4aeb0) at /home/psergey/dev-git/10.1-gsoc-repo/sql/table.cc:83 #1 0x0000555555a6d280 in parse_sql (thd=0x55555ac4aeb0, parser_state=0x7ffff7eb4580, creation_ctx=0xb, do_pfs_digest=false) at /home/psergey/dev-git/10.1-gsoc-repo/sql/sql_parse.cc:9100 #2 0x0000555555a6dcbb in check_and_process_table_functions (thd=0x55555ac4aeb0, table_list=0x7fff5c007630) at /home/psergey/dev-git/10.1-gsoc-repo/sql/sql_parse.cc:9336 #3 0x0000555555a5ab77 in mysql_execute_command (thd=0x55555ac4aeb0) at /home/psergey/dev-git/10.1-gsoc-repo/sql/sql_parse.cc:2471 #4 0x0000555555a6942e in mysql_parse (thd=0x55555ac4aeb0, rawbuf=0x7fff5c007408 "select 1 from f1('aaa')", length=23, parser_state=0x7ffff7eb7100) at /home/psergey/dev-git/10.1-gsoc-repo/sql/sql_parse.cc:7174 #5 0x0000555555a5838b in dispatch_command (command=COM_QUERY, thd=0x55555ac4aeb0, packet=0x55555ac51f71 "select 1 from f1('aaa')", packet_length=23) at /home/psergey/dev-git/10.1-gsoc-repo/sql/sql_parse.cc:1470 #6 0x0000555555a570a5 in do_command (thd=0x55555ac4aeb0) at /home/psergey/dev-git/10.1-gsoc-repo/sql/sql_parse.cc:1093 #7 0x0000555555b95929 in do_handle_one_connection (thd_arg=0x55555ac4aeb0) at /home/psergey/dev-git/10.1-gsoc-repo/sql/sql_connect.cc:1347 #8 0x0000555555b9566e in handle_one_connection (arg=0x55555ac4aeb0) at /home/psergey/dev-git/10.1-gsoc-repo/sql/sql_connect.cc:1258 #9 0x00007ffff691de9a in start_thread (arg=0x7ffff7eb8700) at pthread_create.c:308 #10 0x00007ffff604e3fd in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:112 #11 0x0000000000000000 in ?? () Please check what's going on. (If you don't get the crash, compile and run under valgrind). > Also i removed the test case SELECT * FROM mysql.proc WHERE db = database() > and name = 'f1' because it was failing as it is writing time to result file > so the test passes if we use --record since it writes new file but fails > for ./mysql-test-run sp.test with differences in reject and result file Good point. However, the solution should be not to remove the testcase, but rather make the testcase output not depend on the date/time when the testcase was run. Why did .result file have time/date. Let's look: -SELECT * FROM mysql.proc WHERE db = database() and name = 'f1'; ... -definer root@localhost -created 2015-07-03 16:36:36 -modified 2015-07-03 16:36:36 -sql_mode -comment -character_set_client latin1 ... It did so, because the SELECT selects 'created' and 'modified' columns from mysql.proc. The testcase doesn't really need these columns, please make the SELECT not to select them. BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://s.petrunia.net/blog

1 1

Re: [Maria-developers] MDEV-8389 patch review
by Sergey Petrunia 02 Aug '15

02 Aug '15

Hi Diwas, I've got a crash when trying the first example. The example was: CREATE FUNCTION f1(a INT, b VARCHAR(11)) RETURNS TABLE t1(id INT, name VARCHAR(11)) DETERMINISTIC BEGIN INSERT INTO t1 SELECT id, name FROM t2 WHERE id = a; END|; delimiter ; select f1(1,'aa'); The crash was: Program received signal SIGSEGV, Segmentation fault. [Switching to Thread 0x7ffff7eb8700 (LWP 24655)] 0x0000555555c6a82a in Field_str::Field_str (this=0x7fff5c007d30, ptr_arg=0x0, len_arg=4294967295, null_ptr_arg=0x55555644299c "", null_bit_arg=1 '\001', unireg_check_arg=Field::NONE, field_name_arg=0x7fff5c006780 "f1(1,'aa')", charset_arg=0x0) at /home/psergey/dev-git/10.1-gsoc-repo/sql/field.cc:1845 (gdb) wher #0 0x0000555555c6a82a in Field_str::Field_str (this=0x7fff5c007d30, ptr_arg=0x0, len_arg=4294967295, null_ptr_arg=0x55555644299c "", null_bit_arg=1 '\001', unireg_check_arg=Field::NONE, field_name_arg=0x7fff5c006780 "f1(1,'aa')", charset_arg=0x0) at /home/psergey/dev-git/10.1-gsoc-repo/sql/field.cc:1845 #1 0x0000555555ae6820 in Field_longstr::Field_longstr (this=0x7fff5c007d30, ptr_arg=0x0, len_arg=4294967295, null_ptr_arg=0x55555644299c "", null_bit_arg=1 '\001', unireg_check_arg=Field::NONE, field_name_arg=0x7fff5c006780 "f1(1,'aa')", charset_arg=0x0) at /home/psergey/dev-git/10.1-gsoc-repo/sql/field.h:1200 #2 0x0000555555c7d85c in Field_blob::Field_blob (this=0x7fff5c007d30, ptr_arg=0x0, null_ptr_arg=0x55555644299c "", null_bit_arg=1 '\001', unireg_check_arg=Field::NONE, field_name_arg=0x7fff5c006780 "f1(1,'aa')", share=0x7fff5c0062a0, blob_pack_length=4, cs=0x0) at /home/psergey/dev-git/10.1-gsoc-repo/sql/field.cc:7415 #3 0x0000555555c85725 in make_field (share=0x7fff5c0062a0, ptr=0x0, field_length=2779096485, null_pos=0x55555644299c "", null_bit=1 '\001', pack_flag=2779096485, field_type=MYSQL_TYPE_NULL, field_charset=0x0, geom_type=Field::GEOM_GEOMETRY, srid=0, unireg_check=Field::NONE, interval=0x0, field_name=0x7fff5c006780 "f1(1,'aa')") at /home/psergey/dev-git/10.1-gsoc-repo/sql/field.cc:9861 #4 0x0000555555db4d35 in sp_head::create_result_field (this=0x7fff5c01c398, field_max_length=0, field_name=0x7fff5c006780 "f1(1,'aa')", table=0x7fff5c005710) at /home/psergey/dev-git/10.1-gsoc-repo/sql/sp_head.cc:885 #5 0x0000555555d0d2b3 in Item_func_sp::init_result_field (this=0x7fff5c0055e0, thd=0x55555acf2800) at /home/psergey/dev-git/10.1-gsoc-repo/sql/item_func.cc:6593 #6 0x0000555555d0dc99 in Item_func_sp::fix_fields (this=0x7fff5c0055e0, thd=0x55555acf2800, ref=0x7fff5c006778) at /home/psergey/dev-git/10.1-gsoc-repo/sql/item_func.cc:6844 #7 0x00005555559fd1ed in setup_fields (thd=0x55555acf2800, ref_pointer_array=0x7fff5c007c90, fields=..., mark_used_columns=MARK_COLUMNS_READ, sum_func_list=0x7fff5c007a80, allow_sum_func=true) at /home/psergey/dev-git/10.1-gsoc-repo/sql/sql_base.cc:7925 #8 0x0000555555aa6a83 in JOIN::prepare (this=0x7fff5c007750, rref_pointer_array=0x55555acf6b28, tables_init=0x0, wild_num=0, conds_init=0x0, og_num=0, order_init=0x0, skip_order_by=false, group_init=0x0, having_init=0x0, proc_param_init=0x0, select_lex_arg=0x55555acf68b0, unit_arg=0x55555acf61c0) at /home/psergey/dev-git/10.1-gsoc-repo/sql/sql_select.cc:782 #9 0x0000555555aafa67 in mysql_select (thd=0x55555acf2800, rref_pointer_array=0x55555acf6b28, tables=0x0, wild_num=0, fields=..., conds=0x0, og_num=0, order=0x0, group=0x0, having=0x0, proc_param=0x0, select_options=2147748608, result=0x7fff5c007730, unit=0x55555acf61c0, select_lex=0x55555acf68b0) at /home/psergey/dev-git/10.1-gsoc-repo/sql/sql_select.cc:3301 #10 0x0000555555aa5d93 in handle_select (thd=0x55555acf2800, lex=0x55555acf60f8, result=0x7fff5c007730, setup_tables_done_option=0) at /home/psergey/dev-git/10.1-gsoc-repo/sql/sql_select.cc:371 #11 0x0000555555a65e98 in execute_sqlcom_select (thd=0x55555acf2800, all_tables=0x0) at /home/psergey/dev-git/10.1-gsoc-repo/sql/sql_parse.cc:5798 #12 0x0000555555a5c151 in mysql_execute_command (thd=0x55555acf2800) at /home/psergey/dev-git/10.1-gsoc-repo/sql/sql_parse.cc:2939 #13 0x0000555555a6942e in mysql_parse (thd=0x55555acf2800, rawbuf=0x7fff5c0051b8 "select f1(1,'aa')", length=17, parser_state=0x7ffff7eb7100) at /home/psergey/dev-git/10.1-gsoc-repo/sql/sql_parse.cc:7174 #14 0x0000555555a5838b in dispatch_command (command=COM_QUERY, thd=0x55555acf2800, packet=0x55555acf9b41 "select f1(1,'aa')", packet_length=17) at /home/psergey/dev-git/10.1-gsoc-repo/sql/sql_parse.cc:1470 #15 0x0000555555a570a5 in do_command (thd=0x55555acf2800) at /home/psergey/dev-git/10.1-gsoc-repo/sql/sql_parse.cc:1093 (gdb) list 1840 const char *field_name_arg, CHARSET_INFO *charset_arg) 1841 :Field(ptr_arg, len_arg, null_ptr_arg, null_bit_arg, 1842 unireg_check_arg, field_name_arg) 1843 { 1844 field_charset= charset_arg; 1845 if (charset_arg->state & MY_CS_BINSORT) 1846 flags|=BINARY_FLAG; 1847 field_derivation= DERIVATION_IMPLICIT; 1848 } 1849 (gdb) p charset_arg $10 = (CHARSET_INFO *) 0x0 Another piece of feedback: Please don't change old tests into new tests. Tests that were made for previous MDEVs should stay. New tasks should add new tests. I'll continue to look at the patch. On Sun, Aug 02, 2015 at 06:51:26PM +0530, Diwas Joshi wrote: > I am attaching the patch for MDEV-8389. As discussed earlier, I am creating > tables in the query corresponding to table functions before they are being > accessed. I am checking for each table if its a table-function using > sp_find_routine. If it is then we make a table for it. Going by the same > mechanism used in sp_find_routine to parse the function, i parse a create > table query which initializes create_info and alter_info which i later use > in mysql_create_table() to create temporary tables. I also added a test > case for it. Seems to be working fine. > > I guess we'll also need a function that will delete the tables create after > use. I am working on it right now, but i guess that won't be needed in this > patch. > Please review the patch. > > regards > Diwas Joshi > diff --git a/mysql-test/r/sp-error.result b/mysql-test/r/sp-error.result > index cacfeea..4373925 100644 > --- a/mysql-test/r/sp-error.result > +++ b/mysql-test/r/sp-error.result > @@ -2866,7 +2866,3 @@ SELECT @msg; > DROP FUNCTION f1; > DROP FUNCTION f2; > DROP TABLE t1; > -SELECT * FROM f1('xyz'); > -ERROR 42000: This version of MariaDB doesn't yet support 'Queries using Table Functions' > -SELECT * FROM t1, f1('xyz',t1.a); > -ERROR 42000: You have an error in your SQL syntax; check the manual that corresponds to your MariaDB server version for the right syntax to use near '' at line 1 > diff --git a/mysql-test/r/sp.result b/mysql-test/r/sp.result > index f06939b..d0f6578 100644 > --- a/mysql-test/r/sp.result > +++ b/mysql-test/r/sp.result > @@ -7943,29 +7943,7 @@ RETURNS TABLE t1(id INT, name VARCHAR(11)) > BEGIN > INSERT INTO t1 SELECT id, name FROM t2 WHERE id = a; > END| > -SELECT * FROM mysql.proc WHERE db = database() and name = 'f1'; > -db test > -name f1 > -type FUNCTION > -specific_name f1 > -language SQL > -sql_data_access CONTAINS_SQL > -is_deterministic NO > -security_type DEFINER > -param_list a INT, b VARCHAR(11) > -returns TABLE t1(id int(11), name varchar(11)) > -body BEGIN > -INSERT INTO t1 SELECT id, name FROM t2 WHERE id = a; > -END > -definer root@localhost > -created 2015-07-03 16:36:36 > -modified 2015-07-03 16:36:36 > -sql_mode > -comment > -character_set_client latin1 > -collation_connection latin1_swedish_ci > -db_collation latin1_swedish_ci > -body_utf8 BEGIN > -INSERT INTO t1 SELECT id, name FROM t2 WHERE id = a; > -END > +SELECT * FROM f1(10,'XYZ'); > +id name > +DROP TABLE f1; > DROP FUNCTION f1; > diff --git a/mysql-test/t/sp-error.test b/mysql-test/t/sp-error.test > index ccf5b98..c1a21c8 100644 > --- a/mysql-test/t/sp-error.test > +++ b/mysql-test/t/sp-error.test > @@ -3845,8 +3845,3 @@ SELECT @msg; > DROP FUNCTION f1; > DROP FUNCTION f2; > DROP TABLE t1; > - > ---error 1235 > -SELECT * FROM f1('xyz'); > ---error 1064 > -SELECT * FROM t1, f1('xyz',t1.a); > diff --git a/mysql-test/t/sp.test b/mysql-test/t/sp.test > index 5c42df0..906c91c 100644 > --- a/mysql-test/t/sp.test > +++ b/mysql-test/t/sp.test > @@ -9397,7 +9397,6 @@ END| > > DELIMITER ;| > > ---vertical_results > -SELECT * FROM mysql.proc WHERE db = database() and name = 'f1'; > - > +SELECT * FROM f1(10,'XYZ'); > +DROP TABLE f1; > DROP FUNCTION f1; > diff --git a/sql/sql_parse.cc b/sql/sql_parse.cc > index 2998972..dd16c36 100644 > --- a/sql/sql_parse.cc > +++ b/sql/sql_parse.cc > @@ -2467,6 +2467,8 @@ static bool do_execute_sp(THD *thd, sp_head *sp) > /* should be assigned after making first tables same */ > all_tables= lex->query_tables; > /* set context for commands which do not use setup_tables */ > + if (lex->sql_command == SQLCOM_SELECT) > + check_and_process_table_functions(thd, all_tables); > select_lex-> > context.resolve_in_table_list_only(select_lex-> > table_list.first); > @@ -9222,3 +9224,142 @@ CHARSET_INFO *find_bin_collation(CHARSET_INFO *cs) > } > return cs; > } > + > +/* > + @brief > + Check for table functions and create corresponding tables to be used in query. > + > + @param thd Thread handle > + @param table_list A list of tables being processed in the query. > + > + @return > + False on success > + @retval > + TRUE on error > +*/ > +bool check_and_process_table_functions(THD *thd, TABLE_LIST *table_list) > +{ > + TABLE_LIST *tables = table_list; > + int err= 0; > + while(tables) > + { > + if (tables->table_name == NULL) > + { > + tables = tables->next_local; > + continue; > + } > + sp_head *sp; > + sp_name *name; > + LEX_STRING lex_db; > + LEX_STRING lex_name; > + lex_db.length= strlen(tables->db); > + lex_name.length= strlen(tables->table_name); > + lex_db.str= thd->strmake(tables->db, lex_db.length); > + lex_name.str= thd->strmake(tables->table_name, lex_name.length); > + name= new sp_name(lex_db, lex_name, true); > + name->init_qname(thd); > + Object_creation_ctx *creation_ctx; > + int err=0; > + > + if (!err && (sp = sp_find_routine(thd,TYPE_ENUM_FUNCTION, name, > + &thd->sp_proc_cache, FALSE)) && > + sp->m_type== TYPE_ENUM_TABLE) //check if the table is a table-valued function. > + { > + LEX *old_lex= thd->lex, newlex; > + thd->lex= &newlex; > + ulonglong sql_mode= 0 , saved_mode= thd->variables.sql_mode; > + String defstr; //query in defstr will be parsed to create temporary table. > + defstr.append(STRING_WITH_LEN("CREATE TEMPORARY TABLE ")); > + defstr.append(tables->table_name); > + defstr.append(STRING_WITH_LEN(" (")); > + List_iterator_fast<Create_field> it(sp->m_cols_list); > + > + TABLE table; > + TABLE_SHARE share; > + bzero((char*) &table, sizeof(table)); > + bzero((char*) &share, sizeof(share)); > + table.in_use= thd; > + table.s = &share; > + Create_field *cols_field; > + cols_field= it++; > + while (cols_field) > + { > + uint unused1= 0; > + sp_prepare_create_field(thd, cols_field); > + prepare_create_field(cols_field, &unused1, HA_CAN_GEOMETRY); > + uint field_length; > + Field *field; > + String tmp_result(512); > + field_length= cols_field->length; > + cols_field->flags= 0; > + field= make_field(table.s, /* TABLE_SHARE ptr */ > + (uchar*) 0, /* field ptr */ > + field_length, /* field [max] length */ > + (uchar*) "", /* null ptr */ > + 0, /* null bit */ > + cols_field->pack_flag, > + cols_field->sql_type, > + cols_field->charset, > + cols_field->geom_type, cols_field->srid, > + Field::NONE, /* unreg check */ > + cols_field->interval, > + cols_field->field_name); > + field->vcol_info= cols_field->vcol_info; > + field->stored_in_db= cols_field->stored_in_db; > + if (field) > + field->init(&table); > + defstr.append(cols_field->field_name); > + defstr.append(STRING_WITH_LEN(" ")); > + field->sql_type(tmp_result); > + defstr.append(tmp_result); > + cols_field= it++; > + if(cols_field) > + defstr.append(STRING_WITH_LEN(", ")); > + delete field; > + } > + defstr.append(STRING_WITH_LEN(")")); > + char saved_cur_db_name_buf[SAFE_NAME_LEN+1]; > + LEX_STRING saved_cur_db_name= { saved_cur_db_name_buf, > + sizeof(saved_cur_db_name_buf) }; > + newlex.create_info.DDL_options_st::init(); > + Stored_program_creation_ctx *creation_ctx; > + char definer_user_name_holder[USERNAME_LENGTH + 1]; > + LEX_STRING definer_user_name= { definer_user_name_holder, USERNAME_LENGTH }; > + char definer_host_name_holder[HOSTNAME_LENGTH + 1]; > + LEX_STRING definer_host_name= { definer_host_name_holder, HOSTNAME_LENGTH }; > + newlex.current_select= NULL; > + Parser_state parser_state; > + thd->variables.sql_mode= sql_mode; > + err= parser_state.init(thd, defstr.c_ptr_safe(), defstr.length()); > + if (!err) { > + lex_start(thd); > + err= parse_sql(thd, &parser_state, creation_ctx) || thd->lex == NULL; > + } > + if (!err) > + { > + Table_specification_st create_info(thd->lex->create_info); > + Alter_info alter_info(thd->lex->alter_info, thd->mem_root); > + thd->lex->create_info.default_table_charset= NULL; > + SELECT_LEX *select_lex= &thd->lex->select_lex; > + TABLE_LIST *create_table= select_lex->table_list.first; > + if (append_file_to_dir(thd, &create_info.data_file_name, > + create_table->table_name) || > + append_file_to_dir(thd, &create_info.index_file_name, > + create_table->table_name)) > + { > + err= 1; > + } > + if (!(create_info.used_fields & HA_CREATE_USED_ENGINE)) > + create_info.use_default_db_type(thd); > + > + err= mysql_create_table(thd, create_table, &create_info, &alter_info); > + thd->variables.option_bits|= OPTION_KEEP_LOG; > + } > + thd->variables.sql_mode= saved_mode; > + lex_end(thd->lex); > + thd->lex= old_lex; > + } > + tables = tables->next_local; > + } > + return err; > +} > \ No newline at end of file > diff --git a/sql/sql_parse.h b/sql/sql_parse.h > index 76e1b7a..6b89b35 100644 > --- a/sql/sql_parse.h > +++ b/sql/sql_parse.h > @@ -121,7 +121,7 @@ bool push_new_name_resolution_context(THD *thd, > Item *normalize_cond(Item *cond); > Item *negate_expression(THD *thd, Item *expr); > bool check_stack_overrun(THD *thd, long margin, uchar *dummy); > - > +bool check_and_process_table_functions(THD *thd, TABLE_LIST *table_list); > /* Variables */ > > extern const char* any_db; > diff --git a/sql/sql_yacc.yy b/sql/sql_yacc.yy > index d7d7d7b..dd1f9f6 100644 > --- a/sql/sql_yacc.yy > +++ b/sql/sql_yacc.yy > @@ -10802,7 +10802,6 @@ table_factor: > Yacc_state *state= & thd->m_parser_state->m_yacc; > if (state->table_factor_part_type== 2) > { > - mysql_init_select(lex); > List_iterator_fast<Item> it(lex->value_list); > Item *item; > while ((item = it++)) > @@ -10817,9 +10816,6 @@ table_factor: > MYSQL_YYABORT; > sel->add_joined_table($$); > lex->pop_context(); > - > - my_error(ER_NOT_SUPPORTED_YET, MYF(0), > - "Queries using Table Functions"); //to be removed later > } > else > { -- BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://s.petrunia.net/blog

1 0

Re: [Maria-developers] [Commits] 2430d1d: MDEV-7821 - Server crashes in Item_func_group_concat::fix_fields on 2nd
by Oleksandr Byelkin 31 Jul '15

31 Jul '15

Hi! On 30.07.15 16:59, Sergey Vojtovich wrote: > revision-id: 2430d1d7ccfcf429e45dbfe655a24d57a79ebf6c (mariadb-5.5.44-14-g2430d1d) > parent(s): 392df76bc3a40a5dd1956b12628dd6489a37be36 > committer: Sergey Vojtovich > timestamp: 2015-07-30 18:51:44 +0400 > message: > > MDEV-7821 - Server crashes in Item_func_group_concat::fix_fields on 2nd > execution of PS > > GROUP_CONCAT() with ORDER BY column position may crash server on PS reexecution. > > The problem was that arguments array of GROUP_CONCAT() was adjusted to point to > temporary elements (resolved ORDER BY fields) during first execution. > > This patch expands rev. 08763096cb to restore original arguments array as well. OK to push! > > --- > mysql-test/r/func_gconcat.result | 16 ++++++++++++++++ > mysql-test/t/func_gconcat.test | 11 +++++++++++ > sql/item_sum.cc | 3 +++ > 3 files changed, 30 insertions(+) > > diff --git a/mysql-test/r/func_gconcat.result b/mysql-test/r/func_gconcat.result > index f12a0c1..0bc31a5 100644 > --- a/mysql-test/r/func_gconcat.result > +++ b/mysql-test/r/func_gconcat.result > @@ -1103,3 +1103,19 @@ ORDER BY field; > field > c,c > drop table t3, t2, t1; > +# > +# MDEV-7821 - Server crashes in Item_func_group_concat::fix_fields on 2nd > +# execution of PS > +# > +CREATE TABLE t1(a INT); > +INSERT INTO t1 VALUES(1),(2); > +PREPARE stmt FROM "SELECT GROUP_CONCAT(t1a.a ORDER BY 1, t1a.a=0) FROM t1 AS t1a, t1 AS t1b GROUP BY t1a.a"; > +EXECUTE stmt; > +GROUP_CONCAT(t1a.a ORDER BY 1, t1a.a=0) > +1,1 > +2,2 > +EXECUTE stmt; > +GROUP_CONCAT(t1a.a ORDER BY 1, t1a.a=0) > +1,1 > +2,2 > +DROP TABLE t1; > diff --git a/mysql-test/t/func_gconcat.test b/mysql-test/t/func_gconcat.test > index 42a3076..5550eeb 100644 > --- a/mysql-test/t/func_gconcat.test > +++ b/mysql-test/t/func_gconcat.test > @@ -821,3 +821,14 @@ FROM ( SELECT * FROM t2 ) AS sq2, t3 > ORDER BY field; > > drop table t3, t2, t1; > + > +--echo # > +--echo # MDEV-7821 - Server crashes in Item_func_group_concat::fix_fields on 2nd > +--echo # execution of PS > +--echo # > +CREATE TABLE t1(a INT); > +INSERT INTO t1 VALUES(1),(2); > +PREPARE stmt FROM "SELECT GROUP_CONCAT(t1a.a ORDER BY 1, t1a.a=0) FROM t1 AS t1a, t1 AS t1b GROUP BY t1a.a"; > +EXECUTE stmt; > +EXECUTE stmt; > +DROP TABLE t1; > diff --git a/sql/item_sum.cc b/sql/item_sum.cc > index d8970ca..a24307b 100644 > --- a/sql/item_sum.cc > +++ b/sql/item_sum.cc > @@ -3300,6 +3300,8 @@ void Item_func_group_concat::cleanup() > from Item_func_group_concat::setup() to point to runtime > created objects, we need to reset them back to the original > arguments of the function. > + > + The very same applies to args array. > */ > ORDER **order_ptr= order; > for (uint i= 0; i < arg_count_order; i++) > @@ -3307,6 +3309,7 @@ void Item_func_group_concat::cleanup() > (*order_ptr)->item= &args[arg_count_field + i]; > order_ptr++; > } > + memcpy(args, orig_args, sizeof(Item *) * arg_count); > DBUG_VOID_RETURN; > } > > _______________________________________________ > commits mailing list > commits(a)mariadb.org > https://lists.askmonty.org/cgi-bin/mailman/listinfo/commits

1 0

[Maria-developers] [MDEV-8554] Weird behavior of multi-table UPDATE and THD::save_prep_leaf_list
by Sergey Petrunia 30 Jul '15

30 Jul '15

Hello Igor, I was debugging https://mariadb.atlassian.net/browse/MDEV-8554 The patch is about save_prep_leaf_list is not being cleaned up by a statement. I committed a patch that makes that bug go away. However, the way that statements work with THD::save_prep_leaf_list look weird to me. Since it was you who added save_prep_leaf_list, I wanted to ask you: Consider an example: CREATE TABLE t1 (a INT) ENGINE=MyISAM; INSERT INTO t1 VALUES (1),(2); # Not necessary, the table can be empty CREATE TABLE t2 (b INT) ENGINE=MyISAM; INSERT INTO t2 VALUES (3),(4); # Not necessary, the table can be empty CREATE TABLE t3 (c INT) ENGINE=MyISAM; INSERT INTO t3 VALUES (5),(6); # Not necessary, the table can be empty CREATE OR REPLACE ALGORITHM=MERGE VIEW v3 AS SELECT * FROM t3; Now, let's debug how THD::save_prep_leaf_list changes and is used: UPDATE t1, t2 SET a = 1 WHERE a IN ( SELECT 0 FROM v3 ); We see: 1. Breakpoint 4, THD::THD (this=0x59ff6f0) at /home/psergey/dev-git/10.0/sql/sql_class.cc:1062 save_prep_leaf_list= FALSE; 2. Breakpoint 1, THD::reset_for_next_command (this=0x59ff6f0) at /home/psergey/dev-git/10.0/sql/sql_parse.cc:6272 thd->save_prep_leaf_list= false; ^^^ I have added this line in my fix 3. Breakpoint 5, LEX::save_prep_leaf_tables (this=0x5a02f80) at /home/psergey/dev-git/10.0/sql/sql_lex.cc:4132 bool LEX::save_prep_leaf_tables() { if (!thd->save_prep_leaf_list) return FALSE; ^^^ This is where save_prep_leaf_list is used. 4. Breakpoint 2, mysql_derived_merge (thd=0x59ff6f0, lex=0x5a02f80, derived=0x7fff68008618) at /home/psergey/dev-git/10.0/sql/sql_derived.cc:379 if (thd->lex->sql_command == SQLCOM_UPDATE_MULTI || thd->lex->sql_command == SQLCOM_DELETE_MULTI) thd->save_prep_leaf_list= TRUE; ^^^ this is where save_prep_leaf_list is changed by this statement. 5. (no break). The statement finished. That is, the statement first checks save_prep_leaf_list, and then uses it. This looks weird. Any thoughts? BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://s.petrunia.net/blog

1 0

Re: [Maria-developers] 9f8a458: MDEV-7846: Server crashes in Item_subselect::fix_fields or fails with Thread stack overrun
by Sergei Golubchik 30 Jul '15

30 Jul '15

Hi, Sanja! On Apr 23, sanja(a)mariadb.com wrote: > revision-id: 9f8a458fb2d07298810bb5d9824ce728017bfb32 > parent(s): e540d023e2ec6f37efc9ab695ccdfd4a6744ad64 > committer: Oleksandr Byelkin > branch nick: server > timestamp: 2015-04-23 20:08:57 +0200 > message: > > MDEV-7846: Server crashes in Item_subselect::fix_fields or fails with > Thread stack overrun > > Substitute into transformed subselects original left expression and > than register its change in case it was substituted. Looks mostly ok to me. But please: 1. Add comments (the patch is attached) 2. Add a test case with row items 3. In your test case - if you remove GROUP BY you get a crash again. This might be a different bug, though. Regards, Sergei

1 0