21 Oct
2014
21 Oct
'14
5:40 p.m.
Hi, Oleksandr!
On Oct 21, Oleksandr Byelkin wrote:
> >
> > * add tests with @@default_engine. For example:
> >
> > SET @@default_engine=MyISAM;
> > SET STATEMENT @@default_engine=MEMORY CREATE TABLE t1 (a int);
> > SHOW CREATE TABLE t1; -- verify the engine
> > SET STATEMENT @@default_engine=MyISAM CREATE TABLE t2 (a int);
> Done. I think that to test other & current engine is enough because we
> test variable not engines and using 2 engine which always present makes
> the test simpler.
The point was to test plugin locking. When you change a default engine,
old engine plugin is unlocked, new engine plugin is locked.
In the debug mode there's a code to assert that locks and unlocks are
correctly paired.
It doesn't matter what two engines you use, so your tests are ok.
> >> +'#------------------ STATEMENT Test 4 -----------------------#'
> >> +'# set initial variable value, make prepared statement
> >> +SET SESSION myisam_sort_buffer_size=500000, myisam_repair_threads=1;
> >> +''
> >> +'# Pre-STATEMENT variable value'
> >> +SHOW SESSION VARIABLES LIKE 'myisam_sort_buffer_size';
> >> +Variable_name Value
> >> +myisam_sort_buffer_size 500000
> >> +SHOW SESSION VARIABLES LIKE 'myisam_repair_threads';
> >> +Variable_name Value
> >> +myisam_repair_threads 1
> >> +''
> >> +SET STATEMENT myisam_sort_buffer_size=800000,
> >> +myisam_repair_threads=2 FOR OPTIMIZE TABLE t1;
> > 1. this is not "make prepared statement" as the comment says
> Yes, probably mistake of test creator (cut'n'paste?)
> > 2. what does it actually test? you don't verify whether
> > new values had any effect.
> I have no idea how to check the effect, but maybe it is testing crash...
I think it's just one of those useless tests that doesn't test anything.
There were many of them in the original patch.
> >> +SET STATEMENT sort_buffer_size=150000 FOR SELECT * FROM t2;
> >> +ERROR 42S02: Table 'test.t2' doesn't exist
> >> +''
> >> +'# Post-STATEMENT variable value'
> >> +SHOW SESSION VARIABLES LIKE 'sort_buffer_size';
> >> +Variable_name Value
> >> +sort_buffer_size 150000
> > Looks wrong. The value is not restored in case of an error.
> fixed. It was problem that sql_set_variables was not working correctly
> is you enter it with already happened error.
But the test was good, I hope you kept it.
> >> +SET STATEMENT myisam_sort_buffer_size=400000,
> >> +myisam_repair_threads=2,
> >> +sort_buffer_size=200000,
> >> +binlog_format=row,
> >> +keep_files_on_create=OFF,
> >> +max_join_size=4444440000000 FOR
> >> +DROP FUNCTION myProc;
> > How does this verify that SET STATEMENT had any effect?
> I have no idea what is verified here (crash, syntax, or just test suite
> was strange from the beginning?)
Again, one of many useless tests in the original patch, I suppose.
> >> diff --git a/sql/set_var.cc b/sql/set_var.cc
> >> index bae6511..20e3040 100644
> >> --- a/sql/set_var.cc
> >> +++ b/sql/set_var.cc
> >> @@ -147,7 +147,7 @@ void sys_var_end()
> >> flags(flags_arg), show_val_type(show_val_type_arg),
> >> guard(lock), offset(off), on_check(on_check_func), on_update(on_update_func),
> >> deprecation_substitute(substitute),
> >> - is_os_charset(FALSE)
> >> + is_os_charset(FALSE), default_val(TRUE)
> > I've asked below why is that needed.
> For some variables (like timestamp) default value is not a certain value
> but a special state (timestamp in such state returns current time
> instead of fixed value).
> >> + DBUG_ASSERT(var->is_system());
> >> + set_var *o= NULL, *v= (set_var*)var;
> >> + if (v->var->is_default())
> >> + o= new set_var(v->type, v->var, &v->base, NULL);
> > Why do you treat default values differently?
> because default value is not just a value for some variables, but
> special state.
And here's the review:
> diff --git a/mysql-test/r/events_bugs.result b/mysql-test/r/events_bugs.result
> index e359921..95fc690 100644
> --- a/mysql-test/r/events_bugs.result
> +++ b/mysql-test/r/events_bugs.result
> @@ -218,13 +218,13 @@ drop event events_test.mysqltest_user1;
> drop user mysqltest_user1@localhost;
> drop database mysqltest_db1;
> create event e_53 on schedule at (select s1 from ttx) do drop table t;
> -ERROR 42000: This version of MariaDB doesn't yet support 'Usage of subqueries or stored function calls as part of this statement'
> +ERROR HY000: CREATE/ALTER EVENT does not support subqueries or stored functions.
SQLSTATE 42000 looks fine, please change your error message to use it
> create event e_53 on schedule every (select s1 from ttx) second do drop table t;
> -ERROR 42000: This version of MariaDB doesn't yet support 'Usage of subqueries or stored function calls as part of this statement'
> +ERROR HY000: CREATE/ALTER EVENT does not support subqueries or stored functions.
> create event e_53 on schedule every 5 second starts (select s1 from ttx) do drop table t;
> -ERROR 42000: This version of MariaDB doesn't yet support 'Usage of subqueries or stored function calls as part of this statement'
> +ERROR HY000: CREATE/ALTER EVENT does not support subqueries or stored functions.
> create event e_53 on schedule every 5 second ends (select s1 from ttx) do drop table t;
> -ERROR 42000: This version of MariaDB doesn't yet support 'Usage of subqueries or stored function calls as part of this statement'
> +ERROR HY000: CREATE/ALTER EVENT does not support subqueries or stored functions.
> drop event if exists e_16;
> drop procedure if exists p_16;
> create event e_16 on schedule every 1 second do set @a=5;
> diff --git a/mysql-test/t/set_statement.test b/mysql-test/t/set_statement.test
> index 3ea9235..9a2e577 100644
> --- a/mysql-test/t/set_statement.test
> +++ b/mysql-test/t/set_statement.test
> @@ -661,10 +610,92 @@ SELECT @@sql_mode;
> --echo ''
> SET STATEMENT sql_mode='ansi' FOR PREPARE stmt FROM 'SELECT "t1".* FROM t1';
> execute stmt;
> +ALTER TABLE t1 ADD COLUMN v3 int;
> +# repreparation with other mode cause an error
> +--error ER_PARSE_ERROR
> +execute stmt;
> +ALTER TABLE t1 drop COLUMN v3;
> deallocate prepare stmt;
> --echo ''
> --echo '# Post-STATEMENT
> SELECT @@sql_mode;
> +--echo check the same behaviour in normal set
> +SET sql_mode='ansi';
> +PREPARE stmt FROM 'SELECT "t1".* FROM t1';
> +SET sql_mode=default;
> +execute stmt;
> +ALTER TABLE t1 ADD COLUMN v3 int;
> +# repreparation with other mode cause an error
> +--error ER_PARSE_ERROR
> +execute stmt;
> +ALTER TABLE t1 drop COLUMN v3;
> +deallocate prepare stmt;
> +# the above test about SP
> +SELECT @@sql_mode;
> +SET sql_mode='ansi';
> +SELECT @@sql_mode;
> +DELIMITER |;
> + CREATE PROCEDURE p6() BEGIN
> + SELECT @@sql_mode;
> + SELECT "t1".* FROM t1;
> + END|
> +DELIMITER ;|
> +SET sql_mode=default;
> +call p6;
> +ALTER TABLE t1 ADD COLUMN v3 int;
> +--echo # no reparsing for now
You can still force re-parsing by somehow flushing the
SP cache. E.g. you can start a new connection and run the SP there, try
that, please.
> +call p6;
> +ALTER TABLE t1 drop COLUMN v3;
> +drop procedure p6;
> +
> +
> +SELECT @@sql_mode;
> +DELIMITER |;
> +--echo # SET and the statement parsed as one unit before the SET takes effect
> +--error ER_PARSE_ERROR
> +SET STATEMENT sql_mode='ansi' FOR
> + CREATE PROCEDURE p6() BEGIN
> + SELECT @@sql_mode;
> + SELECT "t1".* FROM t1;
> + END|
> +DELIMITER ;|
> +#call p1;
> +#ALTER TABLE t1 ADD COLUMN v3 int;
> +#--echo # no reparsing for now
> +#call p1;
> +#ALTER TABLE t1 drop COLUMN v3;
> +#drop procedure p1;
> +
> +
> +# the above test about compaund statement
compOund, not compAund
> +SELECT @@sql_mode;
> +SET sql_mode='ansi';
> +SELECT @@sql_mode;
> +DELIMITER |;
> +BEGIN NOT ATOMIC
> + SELECT @@sql_mode;
> + SELECT "t1".* FROM t1;
> +END|
> +DELIMITER ;|
> +SET sql_mode=default;
> +
> +
> +SELECT @@sql_mode;
> +DELIMITER |;
> +--echo # SET and the statement parsed as one unit before the SET takes effect
> +--error ER_PARSE_ERROR
> +SET STATEMENT sql_mode='ansi' FOR
> +BEGIN NOT ATOMIC
> + SELECT @@sql_mode;
> + SELECT "t1".* FROM t1;
> +END|
> +SET STATEMENT sql_mode='ansi' FOR
> +BEGIN NOT ATOMIC
> + SELECT @@sql_mode;
> + SELECT * FROM t1;
> + SELECT @@sql_mode;
> +END|
> +DELIMITER ;|
> --echo ''
> --echo ''
> --echo '#------------------Test 17-----------------------#'
> diff --git a/sql/set_var.cc b/sql/set_var.cc
> index 20e3040..1dad360 100644
> --- a/sql/set_var.cc
> +++ b/sql/set_var.cc
> @@ -687,7 +688,7 @@ int sql_set_variables(THD *thd, List<set_var_base> *var_list, bool free)
> if ((error= var->check(thd)))
> goto err;
> }
> - if (!(error= MY_TEST(thd->is_error())))
> + if (was_error || !(error= MY_TEST(thd->is_error())))
ok, this is that bug with restoring values after an error...
> {
> it.rewind();
> while ((var= it++))
> diff --git a/sql/share/errmsg-utf8.txt b/sql/share/errmsg-utf8.txt
> index 4072af4..e3f0de7 100644
> --- a/sql/share/errmsg-utf8.txt
> +++ b/sql/share/errmsg-utf8.txt
> @@ -7110,5 +7110,5 @@ ER_SLAVE_SKIP_NOT_IN_GTID
> eng "When using GTID, @@sql_slave_skip_counter can not be used. Instead, setting @@gtid_slave_pos explicitly can be used to skip to after a given GTID position."
> ER_STATEMENT_TIMEOUT 70100
> eng "Query execution was interrupted (max_statement_time exceeded)"
> -ER_SET_STATEMENT_TABLES_IS_NOT_SUPPORTED
> - eng "SET STATEMENT does not support using tables in the assignment variables expressions"
> +ER_SUBQUERIES_NOT_SUPPORTED
Here use SQLSTATE 42000
> + eng "%s does not support subqueries or stored functions."
> diff --git a/sql/sql_parse.cc b/sql/sql_parse.cc
> index eff0e08..0fa3b15 100644
> --- a/sql/sql_parse.cc
> +++ b/sql/sql_parse.cc
> @@ -2648,6 +2648,11 @@ static bool do_execute_sp(THD *thd, sp_head *sp)
> {
> DBUG_ASSERT(var->is_system());
> set_var *o= NULL, *v= (set_var*)var;
> + if (v->value->walk(&Item::is_sp_processor, FALSE, NULL))
> + {
> + my_error(ER_SUBQUERIES_NOT_SUPPORTED, MYF(0), "SET STATEMENT");
> + goto error;
> + }
couldn't you rather check lex->table_or_sp_used() in the parser
(like events or KILL are doing) instead of walking the item tree?
> if (v->var->is_default())
> o= new set_var(v->type, v->var, &v->base, NULL);
> else
> @@ -2727,6 +2732,11 @@ static bool do_execute_sp(THD *thd, sp_head *sp)
> my_error(ER_WRONG_ARGUMENTS, MYF(0), "SET");
> goto error;
> }
> + if (lex->sql_command == SQLCOM_COMPOUND)
> + {
> + /* mode might be changed by SET STATEMENT */
> + lex->sphead->m_sql_mode= thd->variables.sql_mode;
> + }
In fact, it might be that you don't need that and you can remove my fix for
MDEV-6609 and simply put this assignment into 'case SQLCOM_COMPOUND:'
> }
>
> /*
Regards,
Sergei