maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

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