- developers - lists.mariadb.org

Re: [Maria-developers] MDEV-19429: Wrong query result with EXISTS and LIMIT 0
by Sergey Petrunia 19 Jul '19

19 Jul '19

Hi Sanja, Ok to push after the below input is addressed. > commit ab5fa406b4b314705cb87ffd74111a518b549ff4 > Author: Oleksandr Byelkin <sanja(a)mariadb.com> > Date: Wed Jul 17 12:31:45 2019 +0200 > > MDEV-19429: Wrong query result with EXISTS and LIMIT 0 > > Check EXISTS LIMIT before rewriting. > > diff --git a/sql/item_subselect.cc b/sql/item_subselect.cc > index afc42dc08d5..85d91181337 100644 > --- a/sql/item_subselect.cc > +++ b/sql/item_subselect.cc > @@ -1432,12 +1432,18 @@ void Item_exists_subselect::fix_length_and_dec() > { > DBUG_ENTER("Item_exists_subselect::fix_length_and_dec"); > init_length_and_dec(); > - /* > - We need only 1 row to determine existence (i.e. any EXISTS that is not > - an IN always requires LIMIT 1) > - */ > - thd->change_item_tree(&unit->global_parameters->select_limit, > - new Item_int((int32) 1)); > + // If limit is not set or it is constant more than 1 > + if (!unit->global_parameters->select_limit || > + (unit->global_parameters->select_limit->basic_const_item() && > + unit->global_parameters->select_limit->val_int() > 1)) > + { > + /* > + We need only 1 row to determine existence (i.e. any EXISTS that is not > + an IN always requires LIMIT 1) > + */ > + thd->change_item_tree(&unit->global_parameters->select_limit, > + new Item_int((int32) 1)); Please fix identation ^ > + } > DBUG_PRINT("info", ("Set limit to 1"));a Please move the DBUG_PRINT into the if () {...} . Because right now it will print "set limit to 1" even when it didn't set it. > DBUG_VOID_RETURN; > } I also observe that LIMIT clause is not printed into EXPLAIN EXTENDED output: mysql> explain extended select * from t10 where exists (select * from one_k where a >55 order by a limit 100 offset 50); +------+-------------+-------+------+---------------+------+---------+------+------+----------+-----------------------------+ | id | select_type | table | type | possible_keys | key | key_len | ref | rows | filtered | Extra | +------+-------------+-------+------+---------------+------+---------+------+------+----------+-----------------------------+ | 1 | PRIMARY | t10 | ALL | NULL | NULL | NULL | NULL | 3 | 100.00 | | | 2 | SUBQUERY | one_k | ALL | NULL | NULL | NULL | NULL | 1342 | 100.00 | Using where; Using filesort | +------+-------------+-------+------+---------------+------+---------+------+------+----------+-----------------------------+ 2 rows in set, 1 warning (6.34 sec) mysql> show warnings\G *************************** 1. row *************************** Level: Note Code: 1003 Message: select `test`.`t10`.`a` AS `a` from `test`.`t10` where exists(select 1 from `test`.`one_k` where (`test`.`one_k`.`a` > 55) order by `test`.`one_k`.`a`) 1 row in set (0.00 sec) ^^^ Note the lack of LIMIT above. It's missing only for EXISTS subqueries, for other kinds of subqueries it is there. I guess this is outside of scope of this MDEV and should be filed separately. BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://s.petrunia.net/blog

1 0

Re: [Maria-developers] Interaction between rpl_slave_state and rpl_binlog_state
by Kristian Nielsen 18 Jul '19

18 Jul '19

Andrei Elkin <andrei.elkin(a)mariadb.com> writes: > I would also raise another but relevant topic of maintaining > > gtid_"executed"_pos > > which is an union of all GTID executed regardless of their arrival > method. E.g some of foreign (to the recipient server) domains gtid:s may > The master potentially could be (made) interested in such table > should create a replication mode without necessary binlogging on the > server. Sorry, I don't follow. Do you mean to create a new table of GTID positions, which will be updated by all transactions, whether originating locally or replicated by a slave thread? Or do you mean to have the mysql.gtid_slave_pos table updated also by locally originated transactions? Or do you mean to have a new system variable @@gtid_executed_pos which is constructed from the existing binlog and mysql.gtid_slave_pos_table, similar to @@gtid_current_pos, but maybe including more GTIDs somehow? > This is not really something new as it's exactly how mysql implemented > gtid bookkeeping. I think MySQL originally kept track of GTIDs only in the binlog, right? And binlog was required to be enabled for GTID to work? I do remember seeing something that relaxed this requirement, but I haven't followed the details. - Kristian.

2 2

Re: [Maria-developers] [Commits] fb619954867: MDEV-15010: Wrong Seconds_Behind_Master when only starting the SQL_Thread
by andrei.elkin＠pp.inet.fi 16 Jul '19

16 Jul '19

Sujatha, howdy. Thanks for a good piece of work! The fact that it's actually an upstream contribution only pleases more :-), and to that regard we need a commit id to refer to (and to the people behind that work :-)). > revision-id: fb619954867232b3a733213a2703b2a425dff8e9 (mariadb-5.5.64-19-gfb619954867) > parent(s): 7a7d9904e12335ee8b1eea9671138b3c469a3829 > author: Sujatha > committer: Sujatha > timestamp: 2019-06-19 14:48:18 +0530 > message: > > MDEV-15010: Wrong Seconds_Behind_Master when only starting the SQL_Thread > > Problem: > ======= > When I run replication normally, I get a value in "Seconds_Behind_Master" from > "SHOW SLAVE STATUS". However, when I run only the SQL Thread (on local relay > logs that have been downloaded previously), I have "NULL" in > "Seconds_Behind_Master". I would expect to have a numeric value. > > Fix: > === > Implemented following changes. > case 1: "Seconds_Behind_Master" shows 0, when SQL thread is in sync with IO > thread and IO thread is running. > case 2: "Seconds_Behind_Master" reports NULL, when SQL thread is in sync with > IO thread and IO thread is stopped. > case 3: "Seconds_Behind_Master" reports NULL, when SQL thread is stopped while > IO thread is up and running. > case 4: "Seconds_Behind_Master" reports a valid numerical value when IO thread > is stopped and SQL thread is consuming existing relay log. I have a question to the case 4 in the tests actually. > > --- > .../suite/rpl/r/rpl_seconds_behind_master.result | 40 ++++++++++ > .../suite/rpl/t/rpl_seconds_behind_master.test | 92 ++++++++++++++++++++++ > sql/slave.cc | 82 ++++++++++++------- > 3 files changed, 187 insertions(+), 27 deletions(-) > > diff --git a/mysql-test/suite/rpl/r/rpl_seconds_behind_master.result b/mysql-test/suite/rpl/r/rpl_seconds_behind_master.result > new file mode 100644 > index 00000000000..1c0ce7c1b19 > --- /dev/null > +++ b/mysql-test/suite/rpl/r/rpl_seconds_behind_master.result > @@ -0,0 +1,40 @@ > +include/master-slave.inc > +[connection master] > +CREATE TABLE t1 (a INT PRIMARY KEY, b INT); > +****************************************************************** > +* Case1: SQL thread is in sync with IO thread and IO thread is UP > +* Seconds_Behind_Master should be '0'. > +****************************************************************** > +include/assert.inc [Seconds_Behind_Master should be 0] > +****************************************************************** > +* Case2: SQL thread is in sync with IO thread and IO thread is > +* stopped. > +* Seconds_Behind_Master should be 'NULL'. > +****************************************************************** > +include/stop_slave_io.inc > +include/assert.inc [Seconds_Behind_Master should be NULL] > +****************************************************************** > +* Case3: SQL thread is stopped while IO thread is up and running. > +* Seconds_Behind_Master should be 'NULL'. > +****************************************************************** > +START SLAVE IO_THREAD; > +include/wait_for_slave_io_to_start.inc > +include/stop_slave_sql.inc > +include/assert.inc [Seconds_Behind_Master should be NULL] > +****************************************************************** > +* Case4: IO thread is stopped. SQL thread is consuming existing relay > +* log. Hence the Seconds_Behind_Master should have a numeric > +* value > 0 > +****************************************************************** > +INSERT INTO t1 VALUES (1,sleep(2)); > +INSERT INTO t1 VALUES (2,sleep(2)); > +include/sync_slave_io_with_master.inc > +include/stop_slave_io.inc > +START SLAVE SQL_THREAD; > +include/wait_for_slave_sql_to_start.inc > +include/assert.inc [Seconds_Behind_Master should be > 0] > +START SLAVE IO_THREAD; > +include/wait_for_slave_io_to_start.inc > +******* CLEANUP ********* > +DROP TABLE t1; > +include/rpl_end.inc > diff --git a/mysql-test/suite/rpl/t/rpl_seconds_behind_master.test b/mysql-test/suite/rpl/t/rpl_seconds_behind_master.test > new file mode 100644 > index 00000000000..6f9ed2939e1 > --- /dev/null > +++ b/mysql-test/suite/rpl/t/rpl_seconds_behind_master.test > @@ -0,0 +1,92 @@ > +# ==== Purpose ==== > +# > +# Test verifies that Seconds_Behind_Master(SBM) values are as expected in various > +# scenario listed below. > +# > +# case 1: SBM=0 if SQL thread is in sync with IO thread and IO thread is UP > +# case 2: SBM=NULL if SQL thread is in sync with IO thread and IO thread is > +# stopped. > +# case 3: SBM=NULL if SQL thread is stopped while IO thread is up and running. > +# case 4: SBM > 0 and SBM != NULL if IO thread is stopped. SQL thread is > +# consuming existing relay log. > +# > +# ==== Implementation ==== > +# > +# Steps: > +# 1 - Create a table and sync it with slave and verify SBM value. > +# 2 - Now stop IO thread and verify SBM > +# 3 - Start IO thread, Stop SQL thread and verify SBM > +# 4 - Execute slow INSERT statements with SLEEP(2) on master. Ensure that > +# the IO thread has read these INSERT statements and then bring it down. > +# 5 - Start the SQL thread and verify that while it is trying to consume > +# these slow INSERT statements in relay log the SBM value is > 0. > +# > +# ==== References ==== > +# > +# MDEV-15010: Wrong Seconds_Behind_Master when only starting the SQL_Thread. > + > +--source include/have_binlog_format_statement.inc > +--source include/master-slave.inc > + > +--disable_query_log > +CALL mtr.add_suppression("Unsafe statement written to the binary log using statement format since BINLOG_FORMAT = STATEMENT"); > +--enable_query_log > + > +CREATE TABLE t1 (a INT PRIMARY KEY, b INT); > +--sync_slave_with_master > + > +-- echo ****************************************************************** > +-- echo * Case1: SQL thread is in sync with IO thread and IO thread is UP > +-- echo * Seconds_Behind_Master should be '0'. > +-- echo ****************************************************************** > +--let $assert_cond= [SHOW SLAVE STATUS, Seconds_Behind_Master, 1] = 0 > +--let $assert_text= Seconds_Behind_Master should be 0 > +--source include/assert.inc > + > +-- echo ****************************************************************** > +-- echo * Case2: SQL thread is in sync with IO thread and IO thread is > +-- echo * stopped. > +-- echo * Seconds_Behind_Master should be 'NULL'. > +-- echo ****************************************************************** > +--source include/stop_slave_io.inc > +--let $assert_cond= "[SHOW SLAVE STATUS, Seconds_Behind_Master, 1]" = "NULL" > +--let $assert_text= Seconds_Behind_Master should be NULL > +--source include/assert.inc > + > +-- echo ****************************************************************** > +-- echo * Case3: SQL thread is stopped while IO thread is up and running. > +-- echo * Seconds_Behind_Master should be 'NULL'. > +-- echo ****************************************************************** > +START SLAVE IO_THREAD; > +--source include/wait_for_slave_io_to_start.inc > +--source include/stop_slave_sql.inc > +--let $assert_cond= "[SHOW SLAVE STATUS, Seconds_Behind_Master, 1]" = "NULL" > +--let $assert_text= Seconds_Behind_Master should be NULL > +--source include/assert.inc > + > +-- echo ****************************************************************** > +-- echo * Case4: IO thread is stopped. SQL thread is consuming existing relay > +-- echo * log. Hence the Seconds_Behind_Master should have a numeric > +-- echo * value > 0 > +-- echo ****************************************************************** > +--connection master > +--disable_warnings > +INSERT INTO t1 VALUES (1,sleep(2)); > +INSERT INTO t1 VALUES (2,sleep(2)); > +--enable_warnings > +--source include/sync_slave_io_with_master.inc > +--source include/stop_slave_io.inc > + > +START SLAVE SQL_THREAD; > +--source include/wait_for_slave_sql_to_start.inc There's a theoretical (and even though I think it would hardly ever be practical) chance for the SQL thread to catch up while for some concurrency reason the waiting would take longer that it's naturally expected. How about to + LOCK TABLES t1 WRITE before START SLAVE SQL_THREAD and unlock > +--let $assert_cond= [SHOW SLAVE STATUS, Seconds_Behind_Master, 1] > 0 > +--let $assert_text= Seconds_Behind_Master should be > 0 > +--source include/assert.inc after the assert check? This measure would speak rather explicitly that the check is done just before any first event has been processed by the SQL thread, and as you prepare 2, then no synchronization is guaranteed. Cheers, Andrei > +START SLAVE IO_THREAD; > +--source include/wait_for_slave_io_to_start.inc > + > +--echo ******* CLEANUP ********* > +--connection master > +DROP TABLE t1; > + > +--source include/rpl_end.inc > diff --git a/sql/slave.cc b/sql/slave.cc > index a8946c69d18..7b4b7bd857e 100644 > --- a/sql/slave.cc > +++ b/sql/slave.cc > @@ -2192,38 +2192,66 @@ bool show_master_info(THD* thd, Master_info* mi) > protocol->store(mi->ssl_cert, &my_charset_bin); > protocol->store(mi->ssl_cipher, &my_charset_bin); > protocol->store(mi->ssl_key, &my_charset_bin); > - > /* > - Seconds_Behind_Master: if SQL thread is running and I/O thread is > - connected, we can compute it otherwise show NULL (i.e. unknown). > + The pseudo code to compute Seconds_Behind_Master: > + if (SQL thread is running) > + { > + if (SQL thread processed all the available relay log) > + { > + if (IO thread is running) > + print 0; > + else > + print NULL; > + } > + else > + compute Seconds_Behind_Master; > + } > + else > + print NULL; > */ > - if ((mi->slave_running == MYSQL_SLAVE_RUN_CONNECT) && > - mi->rli.slave_running) > + if (mi->rli.slave_running) > { > - long time_diff= ((long)(time(0) - mi->rli.last_master_timestamp) > - - mi->clock_diff_with_master); > /* > - Apparently on some systems time_diff can be <0. Here are possible > - reasons related to MySQL: > - - the master is itself a slave of another master whose time is ahead. > - - somebody used an explicit SET TIMESTAMP on the master. > - Possible reason related to granularity-to-second of time functions > - (nothing to do with MySQL), which can explain a value of -1: > - assume the master's and slave's time are perfectly synchronized, and > - that at slave's connection time, when the master's timestamp is read, > - it is at the very end of second 1, and (a very short time later) when > - the slave's timestamp is read it is at the very beginning of second > - 2. Then the recorded value for master is 1 and the recorded value for > - slave is 2. At SHOW SLAVE STATUS time, assume that the difference > - between timestamp of slave and rli->last_master_timestamp is 0 > - (i.e. they are in the same second), then we get 0-(2-1)=-1 as a result. > - This confuses users, so we don't go below 0: hence the max(). > - > - last_master_timestamp == 0 (an "impossible" timestamp 1970) is a > - special marker to say "consider we have caught up". > + Check if SQL thread is at the end of relay log > + Checking should be done using two conditions > + condition1: compare the log positions and > + condition2: compare the file names (to handle rotation case) > */ > - protocol->store((longlong)(mi->rli.last_master_timestamp ? > - max(0, time_diff) : 0)); > + if ((mi->master_log_pos == mi->rli.group_master_log_pos) && > + (!strcmp(mi->master_log_name, mi->rli.group_master_log_name))) > + { > + if (mi->slave_running == MYSQL_SLAVE_RUN_CONNECT) > + protocol->store(0LL); > + else > + protocol->store_null(); > + } > + else > + { > + long time_diff = ((long)(time(0) - mi->rli.last_master_timestamp) > + - mi->clock_diff_with_master); > + /* > + Apparently on some systems time_diff can be <0. Here are possible > + reasons related to MySQL: > + - the master is itself a slave of another master whose time is ahead. > + - somebody used an explicit SET TIMESTAMP on the master. > + Possible reason related to granularity-to-second of time functions > + (nothing to do with MySQL), which can explain a value of -1: > + assume the master's and slave's time are perfectly synchronized, and > + that at slave's connection time, when the master's timestamp is read, > + it is at the very end of second 1, and (a very short time later) when > + the slave's timestamp is read it is at the very beginning of second > + 2. Then the recorded value for master is 1 and the recorded value for > + slave is 2. At SHOW SLAVE STATUS time, assume that the difference > + between timestamp of slave and rli->last_master_timestamp is 0 > + (i.e. they are in the same second), then we get 0-(2-1)=-1 as a result. > + This confuses users, so we don't go below 0: hence the max(). > + > + last_master_timestamp == 0 (an "impossible" timestamp 1970) is a > + special marker to say "consider we have caught up". > + */ > + protocol->store((longlong)(mi->rli.last_master_timestamp ? > + max(0, time_diff) : 0)); > + } > } > else > { > _______________________________________________ > commits mailing list > commits(a)mariadb.org > https://lists.askmonty.org/cgi-bin/mailman/listinfo/commits

1 0

[Maria-developers] MDEV-8306: more input
by Sergey Petrunia 16 Jul '19

16 Jul '19

Hi Varun, Please find more input, I'm commenting on the second patch (the review is probably incomplete, I've made just one pass so might have missed something) > diff -urpN '--exclude=.*' 10.5-mdev8306-orig/sql/opt_subselect.cc 10.5-mdev8306/sql/opt_subselect.cc > --- 10.5-mdev8306-orig/sql/opt_subselect.cc 2019-07-10 11:30:41.151696943 +0300 > +++ 10.5-mdev8306/sql/opt_subselect.cc 2019-07-16 13:07:13.948467315 +0300 > @@ -5517,6 +5517,31 @@ enum_nested_loop_state join_tab_executio > sjm->materialized= TRUE; > } > } > + /* > + This should be the place to add the sub_select call for the > + prefix of the join order > + */ > + > + else if (tab->is_order_nest) > + { > + enum_nested_loop_state rc; > + JOIN *join= tab->join; > + NEST_INFO *nest_info= join->order_nest_info; > + > + if (!nest_info->materialized) > + { > + JOIN_TAB *join_tab= join->join_tab + join->const_tables; > + JOIN_TAB *save_return_tab= join->return_tab; > + if ((rc= sub_select(join, join_tab, FALSE)) < 0 || > + (rc= sub_select(join, join_tab, TRUE)) < 0) > + { > + join->return_tab= save_return_tab; > + DBUG_RETURN(rc); > + } > + join->return_tab= save_return_tab; > + nest_info->materialized= TRUE; > + } Looks a bit repetitive with the SJM nest code above, but this is not a big deal. A question: if the JOIN_TABs for the tables inside the sort nest are still in the join->join_tab array, where is the code to "jump over" them in regular join execution? > + } > > DBUG_RETURN(NESTED_LOOP_OK); > } ... > diff -urpN '--exclude=.*' 10.5-mdev8306-orig/sql/sql_class.h 10.5-mdev8306/sql/sql_class.h > --- 10.5-mdev8306-orig/sql/sql_class.h 2019-07-16 13:10:05.448491384 +0300 > +++ 10.5-mdev8306/sql/sql_class.h 2019-07-16 13:07:13.948467315 +0300 > @@ -6037,6 +6037,24 @@ public: > Copy_field *copy_field; /* Needed for SJ_Materialization scan */ > }; > > +class NEST_INFO : public Sql_alloc > +{ > +public: > + ~NEST_INFO() > + { > + nest_tab= NULL; > + table= NULL; > + } What is this for? Object members should not be used after destructor is called. If you're concerned about accidental access, please use TRASH_XXX macros to fill the variables with "already-freed" marker. > + TMP_TABLE_PARAM tmp_table_param; > + List<Item> nest_base_table_cols; > + List<Item> nest_temp_table_cols; > + TABLE *table; > + st_join_table *nest_tab; > + uint n_tables; > + bool materialized; /* TRUE <=> materialization already performed */ > + table_map nest_tables_map; > +}; > + > > /* Structs used when sorting */ > struct SORT_FIELD_ATTR ... > diff -urpN '--exclude=.*' 10.5-mdev8306-orig/sql/sql_select.cc 10.5-mdev8306/sql/sql_select.cc > --- 10.5-mdev8306-orig/sql/sql_select.cc 2019-07-16 13:10:05.452491478 +0300 > +++ 10.5-mdev8306/sql/sql_select.cc 2019-07-16 13:07:13.952467409 +0300 > return retval; > } > > +void substitute_base_to_nest_items(JOIN *join) > +{ > + NEST_INFO *order_nest_info= join->order_nest_info; > + if (!order_nest_info) > + return; > + REPLACE_NEST_FIELD_ARG arg= {join}; > + join->conds= join->conds->transform(join->thd, &Item::replace_with_nest_items, > + (uchar *) &arg); > + > + List_iterator<Item> it(join->fields_list); > + Item *item, *new_item; > + uint i=0; > + while ((item= it++)) > + { > + if ((new_item= item->transform(join->thd, > + &Item::replace_with_nest_items, > + (uchar *) &arg)) != item) > + it.replace(new_item); > + } Ok, so you are processing the select list, and the WHERE condition. What about the ON expressions, and the right sides of ref access? Also, how do we solve the issue of the same Item_field object being used in two conditions at once: - one inside the sort nest - one outside the sort nest (remember the example with " (inner_tbl_cond1 AND outer_tbl_con1) OR (inner_tbl_cond2 AND outer_tbl_cond2) " > +} > + > > /** ... > @@ -12136,6 +12314,35 @@ end_sj_materialize(JOIN *join, JOIN_TAB > } > > > +enum_nested_loop_state > +end_nest_materialization(JOIN *join, JOIN_TAB *join_tab, bool end_of_records) > +{ > + int error; > + THD *thd= join->thd; > + NEST_INFO *nest_info= join->order_nest_info; > + DBUG_ENTER("end_sj_materialize"); Please change to match the function name. > + if (!end_of_records) > + { > + TABLE *table= nest_info->table; > + fill_record(thd, table, table->field, > + nest_info->nest_base_table_cols, TRUE, FALSE); > + > + if (unlikely(thd->is_error())) > + DBUG_RETURN(NESTED_LOOP_ERROR); /* purecov: inspected */ > + if (unlikely((error= table->file->ha_write_tmp_row(table->record[0])))) > + { > + /* create_myisam_from_heap will generate error if needed */ > + if (table->file->is_fatal_error(error, HA_CHECK_DUP) && Is this check necessary? The table doesn't have any unique keys, why do we ignore duplicates? > + create_internal_tmp_table_from_heap(thd, table, > + nest_info->tmp_table_param.start_recinfo, > + &nest_info->tmp_table_param.recinfo, > + error, 1, NULL)) > + DBUG_RETURN(NESTED_LOOP_ERROR); /* purecov: inspected */ > + } > + } > + DBUG_RETURN(NESTED_LOOP_OK); > +} > + > /* > Check whether a join buffer can be used to join the specified table > BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://s.petrunia.net/blog

1 0

Re: [Maria-developers] d7b274fa257: MDEV-16932: ASAN heap-use-after-free in my_charlen_utf8 / my_well_formed_char_length_utf8 on 2nd execution of SP with ALTER trying to add bad CHECK
by Sergei Golubchik 16 Jul '19

16 Jul '19

Hi, Oleksandr! A couple of minor comments, see below On Jul 15, Oleksandr Byelkin wrote: > revision-id: d7b274fa257 (mariadb-10.2.25-63-gd7b274fa257) > parent(s): 64900e3d7c3 > author: Oleksandr Byelkin <sanja(a)mariadb.com> > committer: Oleksandr Byelkin <sanja(a)mariadb.com> > timestamp: 2019-07-11 13:29:51 +0200 > message: > > MDEV-16932: ASAN heap-use-after-free in my_charlen_utf8 / my_well_formed_char_length_utf8 on 2nd execution of SP with ALTER trying to add bad CHECK > > Make automatic name generation during execution (not prepare). > > Check result of memory allocation operation. > > diff --git a/sql/sql_table.cc b/sql/sql_table.cc > index 043cfaaaaaa..636fb9f427c 100644 > --- a/sql/sql_table.cc > +++ b/sql/sql_table.cc > @@ -6205,6 +6210,10 @@ handle_if_exists_options(THD *thd, TABLE *table, Alter_info *alter_info) > Virtual_column_info *check; > TABLE_SHARE *share= table->s; > uint c; > + > + if (fix_constraints_names(thd, &alter_info->check_constraint_list)) > + DBUG_RETURN(TRUE); It's not very logical, I think, to generate constraint names in handle_if_exists_options(), I'd rather move it out and put directly in mysql_alter_table(). > + > while ((check=it++)) > { > if (!(check->flags & Alter_info::CHECK_CONSTRAINT_IF_NOT_EXISTS) && > @@ -6232,10 +6241,44 @@ handle_if_exists_options(THD *thd, TABLE *table, Alter_info *alter_info) > } > } > > - DBUG_VOID_RETURN; > + DBUG_RETURN(FALSE); > } > > > +static bool fix_constraints_names(THD *thd, List<Virtual_column_info> > + *check_constraint_list) > +{ > + List_iterator<Virtual_column_info> it((*check_constraint_list)); > + Virtual_column_info *check; > + uint nr= 1; > + DBUG_ENTER("fix_constraints_names"); > + if (!check_constraint_list) > + DBUG_RETURN(FALSE); > + // Prevent accessing freed memory during generating unique names > + while ((check=it++)) > + { > + if (check->automatic_name) > + { > + check->name.str= NULL; > + check->name.length= 0; > + } > + } > + it.rewind(); > + // Grnerate unique names if needed typo > + while ((check=it++)) > + { > + if (!check->name.length) > + { > + check->automatic_name= TRUE; > + if (make_unique_constraint_name(thd, &check->name, > + check_constraint_list, > + &nr)) > + DBUG_RETURN(TRUE); > + } > + } > + DBUG_RETURN(FALSE); I would remove the first while() at all and just do the second one till the end: bool ret= FALSE; while ((check=it++)) if (check->automatic_name) ret |= make_unique_constraint_name(...); return ret; Because thd->strmake() basically never fails, it doesn't make much sense to optimize for a case when it does. This code above assumes you set automatic_name=TRUE in mysql_prepare_create_table(). > +} > + > /** > Get Create_field object for newly created table by field index. Regards, Sergei

2 1

Re: [Maria-developers] Interaction between rpl_slave_state and rpl_binlog_state
by Kristian Nielsen 15 Jul '19

15 Jul '19

Hi Andrei! The @@gtid_current_pos exists for one sole purpose. This is to let the user promote a slave as the new master and attach the old master as a slave to the new master. By using master_use_gtid=current_pos, the exact same command can be used to attach a slave to the new master, regardless of whether that slave was previously a slave or a master: CHANGE MASTER TO master_host=new_promoted_master If not using gtid_current_pos (ie master_use_gtid=slave_pos), then to let the old master become a slave of the new master, the old master's position must explicitly be set: SET GLOBAL gtid_slave_pos=@@gtid_binlog_pos This is because for efficiency reasons, the master doesn't update the mysql.gtid_slave_pos in each commit. So now we can see why only GTIDs with the servers own server_id should contribute to @@gtid_current_pos. If a GTID was replicated from another server, that GTID will appear in the @@gtid_slave_pos. If the GTID originated on this server, it will appear in @@gtid_binlog_pos. The @@gtid_current_pos is the @@gtid_slave_pos extended with GTIDs originating on this server, hence only GTIDs with our own server id. Normally, every GTID in the binlog with a different server id than our own will already be in the @@gtid_slave_pos as well - since it originated on another server and was replicated to this server. Thus, in the normal case, where user did not play tricks with the binlog and slave state, extending the @@gtid_current_pos as suggested in MDEV-18404 has no effect - the GTIDs are already in the @@gtid_slave_pos, so @@gtid_current_pos is unaffected. And in case the user deliberately modified the state, it should be up to the user to decide what goes into @@gtid_slave_state and @@gtid_binlog_state. For example, the MDEV-18404 change would make it impossible on a server to remove a replicated GTID from the @@gtid_current_pos if --log-slave-updates (without the drastic RESET MASTER). Another problem is that the server cannot reliably compare GTIDs with distinct server ids to decide which one is the most recent. There is no guarantee that sequence numbers are monotonic across different server ids. Thus the MDEV-18404 method could create completely invalid positions in some setups where @@gtid_strict_mode=0 and replication domains are not strictly maintained. I don't see the value in MDEV-18404. If the user is updating @@gtid_binlog_state (itself a very drastic operation), and wants a specific GTID to go into the slave position - just update the @@gtid_slave_pos with the desired GTID, don't leave the server with an inconsistent replication state. And finally, let me reiterate: I consider the @@gtid_current_pos a design mistake. Better to just transfer the @@gtid_binlog_pos to the @@gtid_slave_pos _only_ at the point where an old master is turned into a slave. This can be done manually already, and it would be simple to implement automatic support for this with an extra option for CHANGE MASTER. Hope this helps, - Kristian. Andrei Elkin <andrei.elkin(a)mariadb.com> writes: > Kristian, howdy! > > I thought it's good to refer to a previous discussion that touched > 'current_pos' feature now when we need to resolve MDEV-18404 which I > look at under a specific angle, narrated here. > >> Finally, experience has shown that a _lot_ of users get problems when >> locally done transactions on a slave influence the slave's GTID position. In >> retrospect, I have realised that CHANGE MASTER TO >> master_use_gtid=current_pos was a mistake, only slave_pos should be used. >> Similarly, if a local transaction in a slave's binlog can cause transactions >> from the master to be silently ignored, it will cause a lot of grief for >> users. > > > To the definition of > > gtid_current_pos > > the docs https://mariadb.com/kb/en/library/gtid/#gtid_current_pos first introduce an intuition > with > > This variable is the GTID of the last change to the database for > each replication domain. Such changes can either be master events > (ie. local changes made by user or application), > or replicated events originating from another master server. > > and then define the variable essentially as an union of two sets: > > For each replication domain, if the server ID of the corresponding > GTID in @@gtid_binlog_pos is equal to the servers own server_id, and > the sequence number is higher than the corresponding GTID in > @@gtid_slave_pos, then the GTID from @@gtid_binlog_pos will be > used. Otherwise the GTID from @@gtid_slave_pos will be used for that > domain. > > The union operation is on a *subset* of @@gtid_binlog_pos (call it B') > and the whole @@gtid_slave_pos (S): > > C = B' \cup S > > where C @@gtid_current_pos. > The @@gtid_binlog_pos subset > consists of GTID:s originated by @@global.server_id (that is foreign > ones excluded). > > Notice that makes the union non symmetric as binary operator. A foreign > GTID feels perfectly fine in S and gets into the union result. > > The following use case > > --connection master > CREATE TABLE t (a INT); > RESET MASTER; > SELECT @@global.server_id; > # => 1 > SET @@server_id= 3; > INSERT INTO t SET a=3; > select @@global.gtid_binlog_state = '0-3-1'; > # => 1 > > technically it's a 'local changes', but when S (@@global.gtid_slave_pos) is empty > > SELECT @@global_current_pos = ''; > # => 1 > > This result is another way to read MDEV-18404. > > After thinking over the restricted definition of @@gtid_current_pos I could not arrive at > relaxing it to account the whole @@gtid_binlog_set which fixes everything. > I wonder what do think about this case, have I missed any use case that might have forced > to the definition be as it is? > > Below is a straightforward patch that sustains the current mtr rpl > suites, expect rpl_gtid_startpos (that exactly relies at one point on > the empty @@gtid_current_pos due to the union specifics, I am leaving > out changes to the test at this point). > > Cheers, > > Andrei > > > diff --git a/sql/rpl_gtid.cc b/sql/rpl_gtid.cc > index 752b4172b6e..1f9802bd8f8 100644 > --- a/sql/rpl_gtid.cc > +++ b/sql/rpl_gtid.cc > @@ -845,8 +845,7 @@ rpl_slave_state::iterate(int (*cb)(rpl_gtid *, void *), void *data, > my_hash_init(&gtid_hash, &my_charset_bin, 32, offsetof(rpl_gtid, domain_id), > sizeof(uint32), NULL, NULL, HASH_UNIQUE); > for (i= 0; i < num_extra; ++i) > - if (extra_gtids[i].server_id == global_system_variables.server_id && > - my_hash_insert(&gtid_hash, (uchar *)(&extra_gtids[i]))) > + if (my_hash_insert(&gtid_hash, (uchar *)(&extra_gtids[i]))) > goto err; > > mysql_mutex_lock(&LOCK_slave_state);

1 0

Re: [Maria-developers] [Commits] 7cabdc461b2: MDEV-6860 Parallel async replication hangs on a Galera node
by Kristian Nielsen 15 Jul '19

15 Jul '19

sachin.setiya(a)mariadb.com writes: > revision-id: 7cabdc461b24fdebe599799d7964efa4b53815e3 (mariadb-10.1.39-91-g7cabdc461b2) > > MDEV-6860 Parallel async replication hangs on a Galera node > > Wait for previous commit beore preparing next transation for galera > diff --git a/sql/rpl_parallel.cc b/sql/rpl_parallel.cc > index 8fef2d66635..7d38c36b840 100644 > --- a/sql/rpl_parallel.cc > +++ b/sql/rpl_parallel.cc > @@ -1181,7 +1181,7 @@ handle_rpl_parallel_thread(void *arg) > before, then wait now for the prior transaction to complete its > commit. > */ > - if (rgi->speculation == rpl_group_info::SPECULATE_WAIT && > + if ((rgi->speculation == rpl_group_info::SPECULATE_WAIT || WSREP_ON) && > (err= thd->wait_for_prior_commit())) Ouch! That's killing _all_ parallel replication when WSREP_ON :-/ Do you really need to do this? It seems quite a restriction for replicating to Galera if parallel replication is not allowed. (I wonder if this isn't just another symptom of the underlying problem that Galera has never been integrated properly into MariaDB and the group commit algorithm / transaction master?). But if the goal is to disable parallel replication in Galera, then you shouldn't do this, it will just confuse/disappoint users, and it will be slower than just using single-threaded replication. Instead, give an error if parallel replication and Galera is enabled at the same time, so users will know of the restriction. - Kristian.

2 2

[Maria-developers] Weekly Report
by Rucha Deodhar 15 Jul '19

15 Jul '19

Report for week 7: Hello! This week I worked on the initial review for INSERT...RETURNING. I removed some lines from my code, used select_result* instead of using with_returning_list, improved formatting, restored the structure of SQL_I_List by removing *saved_first, moved returning_list to LEX so that current_select and lex->first_select_lex() is not used, fixed the logic of my_ok() and send_eof(), and made some part more readable. Now INSERT...IGNORE is working too. For tests, I merged some test cases, removed the other ones and added some cases for existing tests. I also read about bison and how to use its stack to return items so that it can be used instead of list swapping. As suggested, I am making the list rule return the List<Item> as a return value, then the rule that uses select_item_list uses something like this: lex->returning=$n I have started implementing it but getting errors during build. Initially I was getting error: opt_select_expressions has no declared type when I tried making the entire rule return item_list. To fix that I added opt_select_expressions to %type <item_list> I am still trying to fix other errors. Could you give me feedback whether I am moving along the right path, have I missed something? opt_select_expressions: /*empty*/ {$$=NULL} | RETURNING select_item_list { //Make this entire rule return item_list something like: $$=lex->current_select->item_list } ; Insert parser: insert_field_spec opt_insert_update opt_select_expressions { something like if ($3) Lex->returning_list= $3; Lex->pop_select(); //main select if (Lex->check_main_unit_semantics()) MYSQL_YYABORT; } ; Regards, Rucha Deodhar <https://www.avast.com/en-in/recommend?utm_medium=email&utm_source=link&utm_…> I’m protected online with Avast Free Antivirus. Get it here — it’s free forever. <https://www.avast.com/en-in/recommend?utm_medium=email&utm_source=link&utm_…> <#DAB4FAD8-2DD7-40BB-A1B8-4E2AA1F9FDF2>

1 0

[Maria-developers] MDEV-8306: Some input on the code so far
by Sergey Petrunia 10 Jul '19

10 Jul '19

Hi Varun, Please find below some input on the current patch: > diff -urpN '--exclude=.*' 10.5-mdev8306-orig/sql/sql_class.h 10.5-mdev8306/sql/sql_class.h > --- 10.5-mdev8306-orig/sql/sql_class.h 2019-07-10 11:34:00.256256952 +0300 > +++ 10.5-mdev8306/sql/sql_class.h 2019-07-10 11:34:15.436604510 +0300 > @@ -6037,6 +6037,15 @@ public: > Copy_field *copy_field; /* Needed for SJ_Materialization scan */ > }; > > +class NEST_INFO : public Sql_alloc Needs better naming, there are all kinds of nests. Making the first attempt: SORT_NEST_INFO ? > +{ > +public: > + TMP_TABLE_PARAM tmp_table_param; > + List<Item> nest_table_cols; > + TABLE *table; > + uint n_tables; > +}; > + > > /* Structs used when sorting */ > struct SORT_FIELD_ATTR > diff -urpN '--exclude=.*' 10.5-mdev8306-orig/sql/sql_select.cc 10.5-mdev8306/sql/sql_select.cc > --- 10.5-mdev8306-orig/sql/sql_select.cc 2019-07-10 11:34:00.260257044 +0300 > +++ 10.5-mdev8306/sql/sql_select.cc 2019-07-10 11:34:15.448604784 +0300 > @@ -8135,6 +8143,32 @@ choose_plan(JOIN *join, table_map join_t > use_cond_selectivity)) > DBUG_RETURN(TRUE); > } > + trace_plan.end(); > + > + for (uint tablenr=0;tablenr < join->table_count;tablenr++) > + { > + POSITION *pos= &join->best_positions[tablenr]; > + join->order_nest_tables|= pos->table->table->map; > + if (pos->ordering_achieved) > + { > + join->order_nest= TRUE; > + break; > + } > + } The above looks like query plan refinement action, very close to what is done in get_best_combination. Can this code be moved into that function? > + > + if (join->order_nest && unlikely(thd->trace_started())) > + { > + Json_writer_array trace_order_nest(thd, "order_nest"); > + > + for (uint tablenr=0;tablenr < join->table_count;tablenr++) > + { > + POSITION *pos= &join->best_positions[tablenr]; > + trace_order_nest.add_table_name(pos->table); > + if (pos->ordering_achieved) > + break; > + } > + } > + > > /* > Store the cost of this query into a user variable ... > @@ -14041,6 +14186,132 @@ ORDER *simple_remove_const(ORDER *order, > } > > > +/* > + This function basically tries to propgate all the multiple equalites > + for the Field items, so that one can use them to generate QEP that would > + also take into consideration equality propagation > +*/ > +void propagate_equal_field_for_orderby(JOIN *join, ORDER *first_order) > +{ > + ORDER *order; > + table_map not_const_tables= ~join->const_table_map; > + for (order= first_order; order; order= order->next) > + { > + table_map order_tables=order->item[0]->used_tables(); > + if (!(order_tables & not_const_tables)) > + continue; Is the above necessary? I think we have an optimization which removes constant element from ORDER BY list. (Perhaps, that optimization is invoked after the join optimizer. If that is the case, it should be done before the join optimizer) > + else > + { > + if (optimizer_flag(join->thd, OPTIMIZER_SWITCH_ORDERBY_EQ_PROP) && > + order->item[0]->real_item()->type() == Item::FIELD_ITEM && > + join->cond_equal) > + { > + Item *item= order->item[0]; > + /* > + TODO: equality substitution in the context of ORDER BY is > + sometimes allowed when it is not allowed in the general case. > + We make the below call for its side effect: it will locate the > + multiple equality the item belongs to and set item->item_equal > + accordingly. > + */ > + (void)item->propagate_equal_fields(join->thd, > + Value_source:: > + Context_identity(), > + join->cond_equal); > + } > + } > + } > +} > + > +/* > + This function basicall checks if by considering the current join_tab > + would we be able to achieve the ordering > +*/ > + > +bool check_join_prefix_contains_ordering(JOIN *join, JOIN_TAB *tab, > + table_map previous_tables) > +{ > + ORDER *order; > + table_map not_const_tables= ~join->const_table_map; > + for (order= join->order; order; order= order->next) > + { > + table_map order_tables=order->item[0]->used_tables(); > + if (!(order_tables & not_const_tables)) > + continue; Same question as above - at this stage ORDER BY list should not have const elements. > + else > + { > + Item_equal *item_eq= order->item[0]->get_item_equal(); > + if (((previous_tables | tab->table->map) & order_tables) == order_tables || > + (item_eq && (item_eq->used_tables() & (previous_tables | tab->table->map)))) > + continue; > + else > + return FALSE; > + } What about handling for non-const items that are not members of Item_equal? (e.g. ORDER BY left(table.col, 3)) ?) > + } > + return TRUE; > +} > + > + > +bool setup_order_nest(JOIN *join, JOIN_TAB *tab) (Same question about how to call the nest. I would call it "sort nest") > +{ > + if (!tab->is_order_nest) > + return FALSE; > + THD *thd= join->thd; > + Field_iterator_table field_iterator; > + > + JOIN_TAB *start_tab= join->join_tab+join->const_tables, *j; > + NEST_INFO* order_nest_info= join->order_nest_info; > + > + /* This needs to be added to JOIN structure, looks the best option or we > + can have a seperate struture NEST_INFO to hold it > + */ > + > + for (j= start_tab; j < tab; j++) > + { > + TABLE *table= start_tab->table; > + field_iterator.set_table(table); > + for (; !field_iterator.end_of_fields(); field_iterator.next()) > + { > + Field *field= field_iterator.field(); > + if (!bitmap_is_set(table->read_set, field->field_index)) > + continue; > + Item *item; > + if (!(item= field_iterator.create_item(thd))) > + return TRUE; > + order_nest_info->nest_table_cols.push_back(item, thd->mem_root); > + } > + } > + > + /* > + TODO also add ORDER ITEMS that are expressions, only fields are covered above > + */ > + DBUG_ASSERT(!tab->table); > + > + order_nest_info->tmp_table_param.init(); > + order_nest_info->tmp_table_param.bit_fields_as_long= TRUE; > + order_nest_info->tmp_table_param.field_count= order_nest_info->nest_table_cols.elements; > + order_nest_info->tmp_table_param.force_not_null_cols= FALSE; > + > + const LEX_CSTRING order_nest_name= { STRING_WITH_LEN("order-nest") }; > + if (!(tab->table= create_tmp_table(thd, &order_nest_info->tmp_table_param, > + order_nest_info->nest_table_cols, (ORDER*) 0, > + FALSE /* distinct */, > + 0, /*save_sum_fields*/ > + thd->variables.option_bits | TMP_TABLE_ALL_COLUMNS, > + HA_POS_ERROR /*rows_limit */, > + &order_nest_name))) > + return TRUE; /* purecov: inspected */ > + > + //tab->tmp_table_param= &order_nest_info->tmp_table_param; > + tab->type= JT_ALL; > + > + /* > + add_sort_to_table(); > + */ > + > + return false; > +} > + > static int > return_zero_rows(JOIN *join, select_result *result, List<TABLE_LIST> &tables, > List<Item> &fields, bool send_row, ulonglong select_options, > @@ -28550,6 +28821,26 @@ select_handler *SELECT_LEX::find_select_ > return 0; > } > > +double postjoin_oper_cost(THD *thd, double join_record_count, uint rec_len, uint idx) The name of the function is confusing. I see the function is modeled after spl_postjoin_oper_cost(), but that name is not readable either. Is this the cost of using a sort nest? > +{ > + double cost= 0; > + /* > + For only one table in the order_nest, we don't need a fill the temp table, we can > + just read the data into the filesort buffer and read the sorted data from the buffers. > + */ > + if (idx) Comparing idx with zero seems odd here. If we check whether this is the first table, we should compare with join->const_tables (This applies to spl_postjoin_oper_cost, too. Need to ask Igor about that one). > + cost= get_tmp_table_write_cost(thd, join_record_count,rec_len) * > + join_record_count; // cost to fill tmp table > + > + cost+= get_tmp_table_lookup_cost(thd, join_record_count,rec_len) * > + join_record_count; // cost to perform post join operation used here > + cost+= get_tmp_table_lookup_cost(thd, join_record_count, rec_len) + > + (join_record_count == 0 ? 0 : > + join_record_count * log2 (join_record_count)) * > + SORT_INDEX_CMP_COST; // cost to perform sorting > + return cost; > +} > + > > /** > @} (end of group Query_Optimizer) > diff -urpN '--exclude=.*' 10.5-mdev8306-orig/sql/sql_select.h 10.5-mdev8306/sql/sql_select.h > --- 10.5-mdev8306-orig/sql/sql_select.h 2019-07-10 11:34:00.260257044 +0300 > +++ 10.5-mdev8306/sql/sql_select.h 2019-07-10 11:34:15.448604784 +0300 > @@ -289,13 +289,14 @@ typedef struct st_join_table { > > /* TRUE <=> This join_tab is inside an SJM bush and is the last leaf tab here */ > bool last_leaf_in_bush; > - > + > /* > ptr - this is a bush, and ptr points to description of child join_tab > range > NULL - this join tab has no bush children > */ > JOIN_TAB_RANGE *bush_children; > + JOIN_TAB_RANGE *order_nest_children; This is not yet used anywhere? Is there any reason to have both bush_children and order_nest_children? I would suggest using bush_children pointer for both SJ-Materialization nests and sort nests. Another variable in the JOIN_TAB will be used to specify what kind of nest it is. (One JOIN_TAB cannot be both a SJ-Mat nest and a sort nest). > > /* Special content for EXPLAIN 'Extra' column or NULL if none */ > enum explain_extra_tag info; > @@ -524,6 +525,12 @@ typedef struct st_join_table { > /* Becomes true just after the used range filter has been built / filled */ > bool is_rowid_filter_built; > > + /* > + Set to true if we consider creating a nest for a prefix of the JOIN order > + that satisfies the ordering > + */ > + bool is_order_nest; This is a member of JOIN_TAB. If we get here, we are not considering, we've firmly decided to use a sort nest. I would prefer the name "sort nest" rather than order_nest. > + > void build_range_rowid_filter_if_needed(); > > void cleanup(); > @@ -991,6 +998,9 @@ typedef struct st_position > /* Cost info for the range filter used at this position */ > Range_rowid_filter_cost_info *range_rowid_filter_info; > > + /* Flag to be set to TRUE if the join prefix satisfies the ORDER BY CLAUSE */ > + bool ordering_achieved; As far as I understand, this is set to true if we have decided to add a sort_nest. We can add a sort nest when the join prefix covers the ORDER BY, but we don't have to. So, the name is confusing and should be changed to something like "sort_nest_operation_here" > + > } POSITION; > > typedef Bounds_checked_array<Item_null_result*> Item_null_array; BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://s.petrunia.net/blog

1 0

[Maria-developers] INSERT ... RETURNING Initial code review
by Vicențiu Ciorbaru 10 Jul '19

10 Jul '19

Hi Rucha! Here is the promised review. I have run the main test suite locally and have noticedthat some tests fail if you run with a debug build. The failure is an assertionfailure on the protocol level. This means you forgot to call a my_ok or my_errorat an appropriate time. I suggest you try and find which one of your commitsintroduces this bug. A handy tool is `git bisect`. It's a great time to learnabout it, if you have not used it before. There are quite a few other things to fix, but first, let's tackle theinitial review comments. Now, on to the actual code. Your text editor inserts tabs instead of spaces. Please configure it to onlyinsert tabs and have the "tab" size be equal to 2 spaces. > diff --git a/mysql-test/main/insert_parser.test b/mysql- test/main/insert_parser.test> new file mode 100644> index 00000000000..5c1f9408a96> --- /dev/null> +++ b/mysql- test/main/insert_parser.test> @@ -0,0 +1,64 @@> +#> +# Syntax check for INSERT...RETURNING> +#> +> +--echo # RETURNING * and RETURNING <COLUMN_NAME> should work. Trailing whitespace here, please fix.> + --echo # Other cases with RETURNING should output syntax error.> +> + --disable_warnings> +drop table if exists t1,t2;> + --enable_warningsTest cases in MariaDB are written such that they do not leave behind anytables or other elements from running the test. Thus, such statements arenot required (and actively discouraged) in new test cases. If tables fromprevious tests are present and mysql-test-run (mtr) does not mark the test asfailed then inserting such statements will hide the mysql-test-run bug.Please delete.> +> +CREATE TABLE t1(id1 INT, val1 VARCHAR(1));> +--echo Table t1 created successfully. Fields: id1 INT, val1 VARCHAR(1);This comment is not necessary, please delete. The fact that mtr passed meansthe command was succesful. Also, when adding comments, I recommend you prefixthem with #, like you did for the start of the test.> +> +CREATE TABLE t2(id2 INT, val2 VARCHAR(1));> +--echo Table t2 created successfully. Fields: id2 INT, val2 VARCHAR(1);Same as previous comment, not necessary, please delete.> +> +#> +#Simple insert statement> +#> +--echo #Simple insert statementOnly keep one, I suggest the --echo variant, but put a space after #.I also like to add 2 extra --echo # like so:--echo #--echo # Simple insert statement--echo # This makes the result file easier to read.> +INSERT INTO t1 (id1, val1) VALUES (1, 'a');> +INSERT INTO t1 (id1, val1) VALUES (2, 'b') RETURNING *;> +INSERT INTO t1 (id1, val1) VALUES (3, 'c') RETURNING id1, val1;> +INSERT INTO t1 (id1, val1) VALUES (3, 'c') RETURNING id1 AS id;> +SELECT * FROM t1;> +TRUNCATE TABLE t1;> +SELECT * FROM t1;> +> +#> +#multiple values in one insert statement> +#> +--echo #multiple values in one insert statementSame as above.> +INSERT INTO t1 VALUES (1,'a'),(2,'b');> +INSERT INTO t1 VALUES (3,'c'),(4,'d') RETURNING *;> +INSERT INTO t1 VALUES (5,'e'),(6,'f') RETURNING id1, val1;> +INSERT INTO t1 VALUES (5,'e'),(6,'f') RETURNING id1 AS id;> +> +#> +#INSERT...ON DULPICATE KEY UPDATE> +#> +--echo # INSERT...ON DULPICATE KEY UPDATESame as above. Typo too.> +CREATE TABLE ins_duplicate (id INT PRIMARY KEY, val VARCHAR(1));> +INSERT INTO ins_duplicate VALUES (1,'a');> +INSERT INTO ins_duplicate VALUES (2,'b') ON DUPLICATE KEY UPDATE val='b' RETURNING id,val;> +INSERT INTO ins_duplicate VALUES (3,'b') ON DUPLICATE KEY UPDATE val='c' RETURNING *;> +INSERT INTO ins_duplicate VALUES (4,'b') ON DUPLICATE KEY UPDATE val='d' RETURNING id AS id1;> +#> +# INSERT...SET> +#> +> +--echo # INSERT...SETSame as above.> +INSERT INTO t1 SET id1 = 1, val1 = 'a'; > +INSERT INTO t1 SET id1 = 2, val1 = 'b' RETURNING *;> +INSERT INTO t1 SET id1 = 3, val1 = 'c' RETURNING id1,val1;> +INSERT INTO t1 SET id1 = 3, val1 = 'c' RETURNING id1 AS id;> +> +DROP TABLE t1; > +DROP TABLE t2;> +DROP TABLE ins_duplicate;> +> +#> +--echo #End of test case > +#This is superfluous, please delete everything after the last DROP TABLE.> \ No newline at end of file> diff --git a/mysql- test/main/insert_parser_1.test b/mysql-test/main/insert_parser_1.test> new file mode 100644> index 00000000000..f83c288a800> --- /dev/null> +++ b/mysql-test/main/insert_parser_1.test> @@ -0,0 +1,75 @@Hmm, this file does not have a corresponding .result file. It's almost a copyof the previous one and it has windows line endings. It's the only one thathas those. I suggest you move the statements not present in insert_parser.testthere and delete this file entirely.> +#> +# Syntax check for INSERT...RETURNING> +#> +> +--disable_warnings> +drop table if exists t1,t2;> +--enable_warnings> +> +CREATE TABLE t1(id1 INT, val1 VARCHAR(1));> +--echo Table t1 created successfully. Fields: id1 INT, val1 VARCHAR(1);> +> +CREATE TABLE t2(id2 INT, val2 VARCHAR(1));> + --echo Table t2 created successfully. Fields: id2 INT, val2 VARCHAR(1);> +> +#> +#Simple insert statement> +#> +--echo #Simple insert statement> +INSERT INTO t1 (id1, val1) VALUES (1, 'a');> +INSERT INTO t1 (id1, val1) VALUES (2, 'b') RETURNING *;> +INSERT INTO t1 (id1, val1) VALUES (3, 'c') RETURNING id1, val1;> +INSERT INTO t1 (id1, val1) VALUES (4, 'd') RETURNING id1 as id;> +INSERT INTO t1 (id1, val1) VALUES (5, 'e') RETURNING id1 && id1;> +INSERT INTO t1 (id1, val1) VALUES (6, 'f') RETURNING id1 + id1;> +INSERT INTO t1 (id1, val1) VALUES (7, 'g') RETURNING id1 | id1;> +SELECT * FROM t1;> +TRUNCATE TABLE t1;> +SELECT * FROM t1;> +> +#> +#multiple values in one insert statement> +#> +--echo #multiple values in one insert statement> +INSERT INTO t1 VALUES (1,'a'),(2,'b');> +INSERT INTO t1 VALUES (3,'c'),(4,'d') RETURNING *;> +INSERT INTO t1 VALUES (5,'e'),(6,'f') RETURNING id1, val2;> +INSERT INTO t1 VALUES (7,'g'),(8,'h') RETURNING id1 AS id;> +INSERT INTO t1 VALUES (9,'i'),(10,'j') RETURNING id1 | id1 ;> +INSERT INTO t1 VALUES (11,'k'),(12,'l') RETURNING id1 + id1;> +INSERT INTO t1 VALUES (13,'m'),(14,'n') RETURNING id1 && id1;> +SELECT * FROM t1;> +> +#> +#INSERT...ON DULPICATE KEY UPDATE> +#> +--echo # INSERT...ON DULPICATE KEY UPDATE> +CREATE TABLE ins_duplicate (id INT PRIMARY KEY, val VARCHAR(1));> +INSERT INTO ins_duplicate VALUES (1,'a');> +INSERT INTO ins_duplicate VALUES (2,'b') ON DUPLICATE KEY UPDATE val='b' RETURNING id,val;> +INSERT INTO ins_duplicate VALUES (2,'b') ON DUPLICATE KEY UPDATE val='c' RETURNING *;> +INSERT INTO ins_duplicate VALUES (2,'b') ON DUPLICATE KEY UPDATE val='c' RETURNING id AS id1;> +INSERT INTO ins_duplicate VALUES (2,'b') ON DUPLICATE KEY UPDATE val='c' RETURNING id | id;> +INSERT INTO ins_duplicate VALUES (2,'b') ON DUPLICATE KEY UPDATE val='c' RETURNING id && id;> +INSERT INTO ins_duplicate VALUES (2,'b') ON DUPLICATE KEY UPDATE val='c' RETURNING id +id;> +SELECT * FROM ins_duplicate;> +> +#> +# INSERT...SET> +#> +--echo # INSERT...SET> +INSERT INTO t1 SET id1 = 1, val1 = 'a'; > +INSERT INTO t1 SET id1 = 2, val1 = 'b' RETURNING *;> +INSERT INTO t1 SET id1 = 3, val1 = 'c' RETURNING id1,val1;> +INSERT INTO t1 SET id1 = 3, val1 = 'c' RETURNING id1 as id;> +INSERT INTO t1 SET id1 = 3, val1 = 'c' RETURNING id1 | id1;> +INSERT INTO t1 SET id1 = 3, val1 = 'c' RETURNING id1 && id1;> +INSERT INTO t1 SET id1 = 3, val1 = 'c' RETURNING id1 + id1;> +> +DROP TABLE t1; > +DROP TABLE t2;> +DROP TABLE ins_duplicate;> +> +#> +--echo #End of test case > +#> \ No newline at end of file> diff --git a/mysql- test/main/insert_parser_sel.test b/mysql- test/main/insert_parser_sel.test> new file mode 100644> index 00000000000..f144cfc48ec> --- /dev/null> +++ b/mysql- test/main/insert_parser_sel.test> @@ -0,0 +1,45 @@> +#> +# Syntax check for INSERT...RETURNING> +#> +> +--disable_warnings> +drop table if exists t1,t2;> +--enable_warningsSimilar to insert_parser.test, these lines are not necessary.> +> +CREATE TABLE t1(id1 INT, val1 VARCHAR(1));> +--echo Table t1 created successfully. Fields: id1 INT, val1 VARCHAR(1);See insert_parser.test comments.> +> +CREATE TABLE t2(id2 INT, val2 VARCHAR(1));> +--echo Table t2 created successfully. Fields: id2 INT, val2 VARCHAR(1);See insert_parser.test comments.> +> +#> +#Simple insert statement> +#> +--echo #Simple insert statementSee insert_parser.test comments.> +INSERT INTO t1 (id1, val1) VALUES (1, 'a');> +INSERT INTO t1 (id1, val1) VALUES (2, 'b');> +INSERT INTO t1 (id1, val1) VALUES (3, 'c');> +INSERT INTO t1 (id1, val1) VALUES (4, 'd');> +INSERT INTO t1 (id1, val1) VALUES (5, 'e');> +INSERT INTO t1 (id1, val1) VALUES (6, 'f');> +INSERT INTO t1 (id1, val1) VALUES (7, 'g');> +SELECT * FROM t1;> +TRUNCATE TABLE t1;> +SELECT * FROM t1;> +> +#> +# INSERT...SELECT> +#> +--echo #INSERT...SELECTSee insert_parser.test comments.> +INSERT INTO t2(id2,val2) SELECT * FROM t1;> +INSERT INTO t2(id2,val2) SELECT * FROM t1 WHERE id1=1 RETURNING *;> +INSERT INTO t2(id2,val2) SELECT * FROM t1 WHERE id1=2 RETURNING id2,val2;> +INSERT INTO t2(id2,val2) SELECT * FROM t1 WHERE id1=3 RETURNING id2 AS id;> +INSERT INTO t2(id2,val2) SELECT * FROM t1 WHERE id1 IN (SELECT id1 FROM t1 WHERE id1 IN (3,4,5) );Ok, this last line does not test RETURNING at all, what was the intent?Also, please wrap queries to maximum 80 lines. Find the right way to indentthe SQL query on multiple lines to make it easier to read.> +SELECT * FROM t2;> +DROP TABLE t1; > +DROP TABLE t2;> +> +#> +--echo #End of test case > +#See insert_parser.test comments.> \ No newline at end of file> diff --git a/mysql-test/main/insert_returning_complex.test b/mysql- test/main/insert_returning_complex.test> new file mode 100644> index 00000000000..59ef3342e76> --- /dev/null> +++ b/mysql- test/main/insert_returning_complex.test> @@ -0,0 +1,117 @@> +#> +# Test for INSERT...RETURNING with virual cols,operators and # subqueries> +#> +> +--disable_warnings> +drop table if exists t1,t2,ins_duplicate;> + --enable_warningsSee insert_returning.testI would merge this test into the insert_returning.test file. No need to split them up.> +> +CREATE TABLE t1(id1t1 INT, id2t1 INT, valt1 VARCHAR(1));> +CREATE TABLE t2(id1t2 INT, id2t2 INT, valt2 VARCHAR(1));> +INSERT INTO t2 VALUES (1,2,'a'),(2,3,'b'),(3,4,'c'),(4,5,'d'),(5,6,'e'),(6,7,'f'),(7,8,'g'),( 8,9,'h'),(9,10,'i'),(10,11,'j');Please wrap this line to 80 characters max.> +> +--echo Table t1 created successfully. Fields: id1 INT, id2 INT, val VARCHAR(1);Superfluous.> +> +#> +#Simple insert statement> +#> +--echo #Simple insert statementSee insert_returning.test comments.> +INSERT INTO t1 (id1t1, id2t1, valt1) VALUES (1, 2, 'a');> +INSERT INTO t1 (id1t1, id2t1, valt1) VALUES (2, 3, 'b') RETURNING *;> +INSERT INTO t1 (id1t1, id2t1, valt1) VALUES (3, 4, 'c') RETURNING id1t1, valt1;> +INSERT INTO t1 (id1t1, id2t1, valt1) VALUES (4, 5, 'd') RETURNING id1t1 AS id;> +INSERT INTO t1 (id1t1, id2t1, valt1) VALUES (5, 6, 'e') RETURNING id1t1+id2t1 AS total;> +INSERT INTO t1 (id1t1, id2t1, valt1) VALUES (6, 7, 'f') RETURNING id1t1 & id2t1;> +INSERT INTO t1 (id1t1, id2t1, valt1) VALUES (7, 8, 'g') RETURNING id1t1 || id2t1;> +INSERT INTO t1 (id1t1, id2t1, valt1) VALUES (8,9,'h') RETURNING id1t1,UPPER(valt1);> +INSERT INTO t1(id1t1,id2t1,valt1) VALUES (9,10,'i') RETURNING (SELECT GROUP_CONCAT(valt2) FROM t2 WHERE id1t2=1);> +INSERT INTO t1(id1t1,id2t1,valt1) VALUES(10,11,'j') RETURNING (SELECT GROUP_CONCAT(valt2) FROM t2 GROUP BY id1t2 HAVING id1t2=id1t2+1);Ok, please wrap all such lines to 80 characters to make them easier to parse.Second, can we test with subqueries that return multiple rows, for example:SELECT GROUP_CONCAT(valt2) FROM t2 group by id1t2.We should return an error here, but this should be covered in the test casetoo.> +INSERT INTO t1 (id1t1, id2t1, valt1) VALUES (8, 9, 'h') RETURNING id1t1,(SELECT id2t2 FROM t2 WHERE valt2='b');Can you also test a subquery that uses columns from t1?Example:INSERT INTO t1 (id1t1, id2t1, valt1) VALUES (8, 9, 'h') RETURNING id1t1, (SELECT id2t2 + id1t1 FROM t2 WHERE valt2='b');> > +SELECT * FROM t1;> +TRUNCATE TABLE t1;> +SELECT * FROM t1;> +> +> +#> +#multiple values in one insert statement> +#> +--echo #multiple values in one insert statementSee insert_returning.test comments.> +INSERT INTO t1 VALUES (1, 2, 'a'),(2, 3, 'b');> +INSERT INTO t1 VALUES (3, 4, 'c'),(4, 5, 'd') RETURNING *;> +INSERT INTO t1 VALUES (5, 6, 'e'),(6, 7, 'f') RETURNING id2t1, valt1;> +INSERT INTO t1 VALUES (7, 8, 'g'),(8, 9, 'h') RETURNING id2t1 AS id;> +INSERT INTO t1 VALUES (9, 10, 'i'),(10, 11, 'j') RETURNING id2t1+id1t1 as total;> +INSERT INTO t1 VALUES (11, 12, 'k'),(12, 13, 'l') RETURNING id2t1 & id1t1;> +INSERT INTO t1 VALUES (5, 6, 'e'),(2, 3, 'f') RETURNING id2t1 || id1t1;> +INSERT INTO t1 VALUES (13,14,'m'),(14,15,'n') RETURNING id1t1,UPPER(valt1);> +INSERT INTO t1 VALUES (15,16,'o'),(16,17,'p') RETURNING (SELECT GROUP_CONCAT(valt2) FROM t2 WHERE id1t2=1);> +INSERT INTO t1 VALUES (17,18,'q'),(18,19,'r') RETURNING (SELECT GROUP_CONCAT(valt2) FROM t2 GROUP BY id1t2 HAVING id1t2=id1t2+1)> +INSERT INTO t1 VALUES (19,20,'r'),(20,21,'s') RETURNING id1t1,(SELECT id2t2 FROM t2 WHERE valt2='b');Same comments as for single-value insert.> +SELECT * FROM t1;> +TRUNCATE TABLE t1;> +> +#> +#INSERT...ON DULPICATE KEY UPDATE> +#> +--echo # INSERT...ON DULPICATE KEY UPDATETypo DULPICATE -> DUPLICATE. Also, see comments from insert_returning.test.> +CREATE TABLE ins_duplicate (id1 INT PRIMARY KEY, id2 INT, val VARCHAR(1));> +INSERT INTO ins_duplicate VALUES (1,2,'a');> +INSERT INTO ins_duplicate VALUES (2,3,'b') ON DUPLICATE KEY UPDATE val='b' RETURNING id1,val;> +INSERT INTO ins_duplicate VALUES (2,4,'b') ON DUPLICATE KEY UPDATE val='c' RETURNING *;> +INSERT INTO ins_duplicate VALUES (2,5,'b') ON DUPLICATE KEY UPDATE val='d' RETURNING id1+id2 AS total;> +INSERT INTO ins_duplicate VALUES (2,6,'b') ON DUPLICATE KEY UPDATE val='e' RETURNING id1 || id2;> +INSERT INTO ins_duplicate VALUES (2,7,'b') ON DUPLICATE KEY UPDATE val='f' RETURNING id1 & id2;> +INSERT INTO ins_duplicate VALUES (2,8,'b') ON DUPLICATE KEY UPDATE val='g' RETURNING id1 AS id;> +INSERT INTO ins_duplicate VALUES (2,9,'b') ON DUPLICATE KEY UPDATE val='h' RETURNING id1,UPPER(val);> +INSERT INTO ins_duplicate VALUES (2,10,'b') ON DUPLICATE KEY UPDATE val='i' RETURNING (SELECT GROUP_CONCAT(valt2) FROM t2 WHERE id1t2=1);> +INSERT INTO ins_duplicate VALUES (2,11,'b') ON DUPLICATE KEY UPDATE val='j' RETURNING (SELECT GROUP_CONCAT(valt2) FROM t2 GROUP BY id1t2 HAVING id1t2=id1t2+1);> +INSERT INTO ins_duplicate VALUES (2,12,'b') ON DUPLICATE KEY UPDATE val='k' RETURNING id1,(SELECT id2t2 FROM t2 WHERE valt2='b');> +SELECT * FROM ins_duplicate;> +> +#> +# INSERT...SET> +#> +> +--echo # INSERT...SET> +INSERT INTO t1 SET id1t1=1, id2t1=2, valt1='a'; > +INSERT INTO t1 SET id1t1=2, id2t1=3, valt1='b' RETURNING *;> +INSERT INTO t1 SET id1t1=3, id2t1=4, valt1='c' RETURNING valt1;> +INSERT INTO t1 SET id1t1=4, id2t1=5, valt1='d' RETURNING id1t1+id2t1 AS total;> +INSERT INTO t1 SET id1t1=5, id2t1=6, valt1='e' RETURNING id1t1 & id2t1;> +INSERT INTO t1 SET id1t1=6, id2t1=7, valt1='f' RETURNING id1t1 || id2t1;> +INSERT INTO t1 SET id1t1=7, id2t1=8, valt1='g' RETURNING valt1 AS letter;> +INSERT INTO t1 SET id1t1=8, id2t1=9, valt1='h' RETURNING id1t1,UPPER(valt1);> +INSERT INTO t1 SET id1t1=9, id2t1=10, valt1='i' RETURNING (SELECT GROUP_CONCAT(valt2) FROM t2 WHERE id1t2=1);> +INSERT INTO t1 SET id1t1=10, id2t1=11, valt1='j' RETURNING (SELECT GROUP_CONCAT(valt2) FROM t2 GROUP BY id1t2 HAVING id1t2=id1t2+1);> +INSERT INTO t1 SET id1t1=11, id2t1=12, valt1='k' RETURNING id1t1,(SELECT id2t2 FROM t2 WHERE valt2='b'); > +SELECT * FROM t1;> +TRUNCATE TABLE t1;> +SELECT * FROM t1;> +> +#> +# INSERT...SELECT> +#> +--echo # INSERT...SELECT> +INSERT INTO t1(id1t1, id2t1, valt1) SELECT * FROM t2 WHERE id1t2=1;> +INSERT INTO t1(id1t1, id2t1, valt1) SELECT * FROM t2 WHERE id1t2=2 RETURNING *;> +INSERT INTO t1(id1t1, id2t1, valt1) SELECT * FROM t2 WHERE id1t2=3 RETURNING valt1;> +INSERT INTO t1(id1t1, id2t1, valt1) SELECT * FROM t2 WHERE id1t2=4 RETURNING id1t1+id2t1 AS total;> +INSERT INTO t1(id1t1, id2t1, valt1) SELECT * FROM t2 WHERE id1t2=5 RETURNING id1t1 & id2t1;> +INSERT INTO t1(id1t1, id2t1, valt1) SELECT * FROM t2 WHERE id1t2=6 RETURNING valt1 AS letter;> +INSERT INTO t1(id1t1, id2t1, valt1) SELECT * FROM t2 WHERE id1t2=2 RETURNING id1t1 || id2t1;> +INSERT INTO t1(id1t1, id2t1, valt1) SELECT * FROM t2 WHERE id1t2=7 RETURNING id1t1,UPPER(valt1);> +INSERT INTO t1(id1t1, id2t1, valt1) SELECT * FROM t2 WHERE id1t2=8> +RETURNING (SELECT GROUP_CONCAT(val) FROM ins_duplicate WHERE id1=1);> +INSERT INTO t1(id1t1, id2t1, valt1) SELECT * FROM t2 WHERE id1t2=9> +RETURNING (SELECT GROUP_CONCAT(val) FROM ins_duplicate GROUP BY id1 HAVING id1=id1+1);> +INSERT INTO t1(id1t1, id2t1, valt1) SELECT * FROM t2 WHERE id1t2=10 RETURNING id1t1,(SELECT id2 FROM ins_duplicate WHERE valt2='b');> +SELECT* FROM t1;> +> +--echo Droping t1 and ins_duplicate> +DROP TABLE t1;> +DROP TABLE t2; > +DROP TABLE ins_duplicate;> +> +#> +--echo #End of test case > +# > \ No newline at end of file> Binary files /dev/null and b/mysql- test/main/insert_returning_datatypes.result differOk, why is this a binary file for git? Something went wrong here.> diff --git a/mysql- test/main/insert_returning_datatypes.test b/mysql- test/main/insert_returning_datatypes.test> new file mode 100644> index 00000000000..803c8326b6f> --- /dev/null> +++ b/mysql- test/main/insert_returning_datatypes.test> @@ -0,0 +1,92 @@> +#> +# Tests for DELETE FROM <table> ... RETURNING <expr>,...> +# What is DELETE FROM .. RETURNING doing here? :)> +> +--disable_warnings> +drop table if exists t1,t2;> +--enable_warningsI vote for merging this into the previous tests.> +> +CREATE TABLE t1(num_int1 INT(2) PRIMARY KEY, > + num_bit1 BIT(2),> + num_float1 FLOAT(5,2),> + num_double1 DOUBLE(5,2),> + char_enum1 ENUM('A','B','C','D'),> + char_set1 SET('a','b','c','d','e'),> + str_varchar1 VARCHAR(2),> + str_binary1 BINARY(2),> + d1 DATE,> + dt1 DATETIME,> + ts1 TIMESTAMP,> + y1 YEAR,> + b1 BOOL);> +> +CREATE TABLE t2(num_int2 INT(2) PRIMARY KEY, > + num_bit2 BIT(2),> + num_float2 FLOAT(5,2),> + num_double2 DOUBLE(5,2),> + char_enum2 ENUM('A','B','C','D'),> + char_set2 SET('a','b','c','d','e'),> + str_varchar2 VARCHAR(2),> + str_binary2 BINARY(2),> + d2 DATE,> + dt2 DATETIME,> + ts2 TIMESTAMP,> + y2 YEAR,> + b2 BOOL);> +> +> +#> +# SIMLPE INSERT STATEMENT> +#Typo, but please delete this anayway.> +--echo # SIMPLE INSERT STATEMENTSee other test comments.> +INSERT INTO t1(num_int1,num_bit1,num_float1,num_double1,char_enum1,char_set1,str_va rchar1, str_binary1,d1,dt1,ts1,y1,b1) VALUES(1,0, 123.45, 123.55, 'A','b,e', 'V','10','120314',"2012-04-19 13:08:22", '2001-07-22 12:12:12',2012,0) RETURNING *;> +> +INSERT INTO t1(num_int1,num_bit1,num_float1,num_double1,char_enum1,char_set1,str_va rchar1, str_binary1,d1,dt1,ts1,y1,b1) VALUES(2,0, 123.45, 123.55, 'A','b,e', 'V','10','120314',"2012-04-19 13:08:22", '2001-07-22 12:12:12',2012,0) RETURNING num_int1,num_bit1,d1,dt1;> +> +> +> +#> +# MULTIPLE ROWS IN SINGLE STATEMENT > +#> +--echo #Multiple rows inserted in one statement> +INSERT INTO t1 VALUES(3,0, 123.45, 123.55, 'A','b,e', 'V','10','120314',"2012-04-19 13:08:22", '2001-07-22 12:12:12',2012,0),(4,0, 123.45, 123.55, 'A','b,e', 'V','10','120314',"2012-04-19 13:08:22", '2001-07-22 12:12:12',2012,1),(5,0, 123.45, 123.55, 'A','b,e','V','10','120314',"2012-04-19 13:08:22", '2001-07-22 12:12:12',2012,1) RETURNING *;> +> +INSERT INTO t1 VALUES(6,0, 123.45, 123.55, 'A','b,e', 'V','10','120314',"2012-04-19 13:08:22", '2001-07-22 12:12:12',2012,0),(7,0, 123.45, 123.55, 'A','b,e', 'V','10','120314',"2012-04-19 13:08:22", '2001-07-22 12:12:12',2012,1),(8,0, 123.45, 123.55, 'A','b,e','V','10','120314',"2012-04-19 13:08:22", '2001-07-22 12:12:12',2012,1) RETURNING char_set1,ts1,y1;> +> +> +> +#> +#INSERT...SET...RETURNING> +#> +--echo #INSERT...SET...RETURNING> +INSERT INTO t1 SET num_int1=9,num_bit1=0,num_float1=124.67,num_double1=231.12,char_enum1=' B',char_set1='a,d,e',str_varchar1='AB',str_binary1='01', d1='120314',dt1="2012-04-19 13:08:22",ts1='2001-07-22 12:12:1',y1=2014,b1=1 RETURNING *;> +> +INSERT INTO t1 SET num_int1=10,num_bit1=0,num_float1=124.67,num_double1=231.12,char_enum1= 'B',char_set1='a,d,e',str_varchar1='AB',str_binary1='01',d1='120314',dt 1="2012-04-19 13:08:22",ts1='2001-07-22 12:12:1',y1=2014,b1=1 RETURNING b1,char_enum1;> +> +> +> +#> +# INSERT...ON DUPLICATE KEY UPDATE> +# > +--echo # INSERT...ON DUPLCATE KEY UPDATE...RETURNING> +INSERT INTO t1(num_int1,num_bit1,num_float1,num_double1,char_enum1,char_set1,str_va rchar1, str_binary1,d1,dt1,ts1,y1,b1) VALUES (10,0, 123.45, 123.55, 'C','b,e', 'V','10','120314',"2012-04-19 13:08:22", '2001-07-22 12:12:12',2012,0) ON DUPLICATE KEY UPDATE num_float1=111.111 RETURNING *;> +> +INSERT INTO t1(num_int1,num_bit1,num_float1,num_double1,char_enum1,char_set1,str_va rchar1, str_binary1,d1,dt1,ts1,y1,b1) VALUES (11,0, 123.45, 123.55, 'A','b,e', 'V','10','120314',"2012-04-19 13:08:22", '2001-07-22 12:12:12',2012,0) ON DUPLICATE KEY UPDATE char_set1='a,b' RETURNING char_set1,num_int1;> +> +> +> +#> +# INSERT...SELECT...RETURNING> +#> +--echo # INSERT...SELECT...RETURNING> +INSERT INTO t2 (num_int2,num_bit2,num_float2,num_double2,char_enum2,char_set2,str_varc har2, str_binary2,d2,dt2,ts2,y2,b2) SELECT * FROM t1 RETURNING *;> +TRUNCATE TABLE t2;> +INSERT INTO t2 (num_int2,num_bit2,num_float2,num_double2,char_enum2,char_set2,str_varc har2, str_binary2,d2,dt2,ts2,y2,b2) SELECT * FROM t1 RETURNING num_double2,str_varchar2,str_binary2;> +> +> +DROP TABLE t1;> +DROP TABLE t2;> +> +#> +#END OF TEST CASE> +# > \ No newline at end of file> diff --git a/mysql-test/main/insert_returning_err.test b/mysql- test/main/insert_returning_err.test> new file mode 100644> index 00000000000..4af798e5c68> --- /dev/null> +++ b/mysql- test/main/insert_returning_err.test> @@ -0,0 +1,97 @@> +#> +# Test for checking error message for INSERT...RETURNING> +#> +> +#INSERT INTO <table> ... RETURNING <not existing col>> +#INSERT INTO <table> ... RETURNING <expr with aggr function>> +#INSERT INTO ... RETURNING subquery with more than 1 row> +#INSERT INTO ... RETURNING operand should contain 1 colunm(s)> +#INSERT INTO ... RETURNING operand should contain 1 colunm(s)> +> +--disable_warnings> +drop table if exists t1,t2;> +--enable_warnings> +> +CREATE TABLE t1(id1 INT,val1 VARCHAR(1));> +CREATE TABLE t2(id2 INT,val2 VARCHAR(1));> +CREATE TABLE ins_duplicate (id INT PRIMARY KEY, val VARCHAR(1));> +> +INSERT INTO t1 VALUES(1,'a'),(2,'b'),(3,'c');> +> +#> +# SIMLPE INSERT STATEMENT> +#> +--error ER_BAD_FIELD_ERROR> +INSERT INTO t2(id2,val2) VALUES(1,'a') RETURNING id1;> +--error ER_INVALID_GROUP_FUNC_USE> +INSERT INTO t2(id2,val2) values(2,'b') RETURNING MAX(id2);> +--error ER_SUBQUERY_NO_1_ROW> +INSERT INTO t2(id2,val2) VALUES(3,'c') RETURNING (SELECT id1 FROM t1);Ok, so you do test this, good. What about a subquery using a column from t2?> +--error ER_OPERAND_COLUMNS> +INSERT INTO t2(id2,val2) VALUES(4,'d') RETURNING (SELECT * FROM t1);This query fails specifically because there are multiple columns in the subselect.What happens if there is only one column?> +--error ER_OPERAND_COLUMNS> +INSERT INTO t2(id2,val2) VALUES(4,'d') RETURNING (SELECT * FROM t2);> +> +#> +# Multiple rows in single insert statement> +#> +--error ER_BAD_FIELD_ERROR> +INSERT INTO t2 VALUES(1,'a'),(2,'b') RETURNING id1;> +--error ER_INVALID_GROUP_FUNC_USE> +INSERT INTO t2 VALUES(3,'c'),(4,'d') RETURNING MAX(id2);> +--error ER_SUBQUERY_NO_1_ROW> +INSERT INTO t2 VALUES(5,'c'),(6,'f') RETURNING (SELECT id1 FROM t1);> +--error ER_OPERAND_COLUMNS> +INSERT INTO t2 VALUES(7,'g'),(8,'h') RETURNING (SELECT * FROM t1);> +--error ER_OPERAND_COLUMNS> +INSERT INTO t2 VALUES(7,'g'),(8,'h') RETURNING (SELECT * FROM t2);> +> +#> +# INSERT ... SET> +#> +--error ER_BAD_FIELD_ERROR> +INSERT INTO t2 SET id2=1, val2='a' RETURNING id1;> +--error ER_INVALID_GROUP_FUNC_USE> +INSERT INTO t2 SET id2=2, val2='b' RETURNING MAX(id2);> +--error ER_SUBQUERY_NO_1_ROW> +INSERT INTO t2 SET id2=3, val2='c' RETURNING (SELECT id1 FROM t1);> +--error ER_OPERAND_COLUMNS> +INSERT INTO t2 SET id2=4, val2='d' RETURNING (SELECT * FROM t1);> +--error ER_OPERAND_COLUMNS> +INSERT INTO t2 SET id2=4, val2='d' RETURNING (SELECT * FROM t2);> +> +#> +#INSERT...ON DUPLICATE KEY UPDATE> +#> + --error ER_BAD_FIELD_ERROR> +INSERT INTO ins_duplicate VALUES (2,'b') ON DUPLICATE KEY UPDATE val='b' RETURNING id1;> +--error ER_INVALID_GROUP_FUNC_USE> +INSERT INTO ins_duplicate VALUES (2,'b') ON DUPLICATE KEY UPDATE val='b' RETURNING MAX(id);> +--error ER_SUBQUERY_NO_1_ROW> +INSERT INTO ins_duplicate VALUES (2,'b') ON DUPLICATE KEY UPDATE val='b' RETURNING (SELECT id1 FROM t1);> +--error ER_OPERAND_COLUMNS> +INSERT INTO ins_duplicate VALUES (2,'b') ON DUPLICATE KEY UPDATE val='b' RETURNING (SELECT * FROM t1);> +--error ER_OPERAND_COLUMNS> +INSERT INTO ins_duplicate VALUES (2,'b') ON DUPLICATE KEY UPDATE val='b' RETURNING (SELECT * FROM ins_duplicate);> +> +#> +#INSERT...SELECT...RETURNING> +#Use --echo # for the lines above please.> +--error ER_BAD_FIELD_ERROR> +INSERT INTO t2(id2, val2) SELECT * FROM t1 WHERE id1=1 RETURNING id1;> +--error ER_INVALID_GROUP_FUNC_USE> +INSERT INTO t2(id2, val2) SELECT * FROM t1 WHERE id1=2 RETURNING MAX(id2);> +--error ER_SUBQUERY_NO_1_ROW> +INSERT INTO t2(id2, val2) SELECT * FROM t1 WHERE id1=2 RETURNING (SELECT id1 FROM t1);> +--error ER_OPERAND_COLUMNS> +INSERT INTO t2(id2, val2) SELECT * FROM t1 WHERE id1=2 RETURNING (SELECT * FROM t1);> +--error ER_OPERAND_COLUMNS> +INSERT INTO t2(id2, val2) SELECT * FROM t1 WHERE id1=2 RETURNING (SELECT * FROM t2);> +> +DROP TABLE t1;> +DROP TABLE t2;> +DROP TABLE ins_duplicate;> +> +#> +# End of test case> +# > \ No newline at end of file> diff --git a/mysql- test/main/insert_returning_stmt.result b/mysql- test/main/insert_returning_stmt.result> new file mode 100644> index 00000000000..3b48cb11b28> --- /dev/null> +++ b/mysql- test/main/insert_returning_stmt.resultHow is this test different than previous ones? I am getting a bit confusedby the large number of test files that seem simillar.> \ No newline at end of file> diff --git a/mysql-test/main/insert_sel_returning.test b/mysql- test/main/insert_sel_returning.test> new file mode 100644> index 00000000000..8b4880e2064> --- /dev/null> +++ b/mysql- test/main/insert_sel_returning.test> diff --git a/sql/sql_class.h b/sql/sql_class.h> index 18ccf992d1e..8e67818f891 100644> --- a/sql/sql_class.h> +++ b/sql/sql_class.h> @@ -5475,13 +5475,15 @@ class select_dump :public select_to_file {> > class select_insert :public select_result_interceptor {> public:> + select_result* sel_result;> + bool with_returning_list;> TABLE_LIST *table_list;> TABLE *table;> List<Item> *fields;> ulonglong autoinc_value_of_last_inserted_row; // autogenerated or not> COPY_INFO info;> bool insert_into_view;> - select_insert(THD *thd_arg, TABLE_LIST *table_list_par,> + select_insert(bool with_ret_list, select_result* sel_ret_list, THD *thd_arg, TABLE_LIST *table_list_par,When adding parameters, please add them to the end of the function, not thestart. Do we need both with_returning_list and sel_result pointer? Can wenot use just the sel_result pointer and if it is null, we assume we do nothave a returning list?> TABLE *table_par, List<Item> *fields_par,> List<Item> *update_fields, List<Item> *update_values,> enum_duplicates duplic, bool ignore);> diff --git a/sql/sql_insert.cc b/sql/sql_insert.cc> index f3a548d7265..6eb9883fe87 100644> --- a/sql/sql_insert.cc> +++ b/sql/sql_insert.cc> @@ -1236,26 +1261,38 @@ bool mysql_insert(THD *thd,TABLE_LIST *table_list,> if ((iteration * values_list.elements) == 1 && (!(thd->variables.option_bits & OPTION_WARNINGS) ||> !thd->cuted_fields))> {> - my_ok(thd, info.copied + info.deleted +> - ((thd- >client_capabilities & CLIENT_FOUND_ROWS) ?> - info.touched : info.updated),> - id);> + if (with_returning_list)> + result->send_eof();> + else > + {> + my_ok(thd, info.copied + info.deleted +> + ((thd->client_capabilities & CLIENT_FOUND_ROWS) ?> + info.touched : info.updated),> + id);> + }> }> e lse> {> char buff[160];> ha_rows updated=((thd- >client_capabilities & CLIENT_FOUND_ROWS) ?> info.touched : info.updated);> - if (ignore)> - sprintf(buff, ER_THD(thd, ER_INSERT_INFO), (ulong) info.records,> - (lock_type == TL_WRITE_DELAYED) ? (ulong) 0 :> - (ulong) (info.records - info.copied),> - (long) thd->get_stmt_da()- >current_statement_warn_count());> - else> - sprintf(buff, ER_THD(thd, ER_INSERT_INFO), (ulong) info.records,> - (ulong) (info.deleted + updated),> - (long) thd->get_stmt_da()- >current_statement_warn_count());> - ::my_ok(thd, info.copied + info.deleted + updated, id, buff);> + if (ignore)> + sprintf (buff, ER_THD(thd, ER_INSERT_INFO), (ulong)info.records,> + (lock_type == TL_WRITE_DELAYED) ? (ulong)0 :> + (ulong) (info.records - info.copied),> + (long)thd- >get_stmt_da()->current_statement_warn_count());> + else > + {> + if (with_returning_list)> + result- >send_eof();> + else > + {> + sprintf(buff, ER_THD(thd, ER_INSERT_INFO), (ulong)info.records,> + (ulong)(info.deleted + updated),> + (long)thd->get_stmt_da()- >current_statement_warn_count());> + ::my_ok(thd, info.copied + info.deleted + updated, id, buff);> + }> + }Uhm, careful here, you have changed the logic of when my_ok gets called! Please re-read this bit of code and try to understand where things havechanged. Also, looking at this, can you test INSERT IGNORE ... RETURNING too? I have a feeling it won't work with the current implementation.> }> thd->abort_on_warning= 0;> if (thd->lex- >current_select->first_cond_optimization)> @@ -1557,7 +1595,10 @@ bool mysql_prepare_insert(THD *thd, TABLE_LIST *table_list,> table_list->next_local= 0;> context- >resolve_in_table_list_only(table_list);> > - res= (setup_fields(thd, Ref_ptr_array(),> + res= ((with_returning_list?((sel_lex->with_wild && setup_wild(thd, table_list, sel_lex->returning_list, NULL, sel_lex->with_wild,> + &select_lex->hidden_bit_fields)) ||> + setup_fields(th d, Ref_ptr_array(),> + sel_lex->returning_list, MARK_COLUMNS_READ, 0, NULL, 0)):false)||setup_fields(thd, Ref_ptr_array(),Uhm, please fix the formatting for this code. The logic is also quite convoluted,can you turn this into something actually readable? Also, you are calling the same setup_wild && setup_fields call both withinmysql_prepare_insert, but also within select_result->prepare. Why 2 times?> *values, MARK_COLUMNS_READ, 0, NULL, 0) ||> check_insert_fields(thd, context->table_list, fields, *values,> !insert_into_view, 0, &map));> @@ -3557,12 +3598,13 @@ bool Delayed_insert::handle_inserts(void)> TRUE Error> */> > -bool mysql_insert_select_prepare(THD *thd)> +bool mysql_insert_select_prepare(THD *thd,select_result *sel_res)> {> LEX *lex= thd->lex;> SELECT_LEX *select_lex= lex- >first_select_lex();> DBUG_ENTER("mysql_insert_select_prepare");> -> + bool with_ret = lex->current_select- >returning_list.is_empty()?false:true;> + List<Item>& returning_list = thd->lex->current_select->returning_list;Not really a fan of this method. By doing this, it's going to be impossibleto extend INSERT .. RETURNING to work within subqueries.> > /*> SELECT_LEX do not belong to INSERT statement, so we can't add WHERE> @@ -3572,14 +3614,18 @@ bool mysql_insert_select_prepare(THD *thd)> if (mysql_prepare_insert(thd, lex- >query_tables,> lex->query_tables->table, lex->field_list, 0,> lex->update_list, lex- >value_list, lex->duplicates,> - &select_lex- >where, TRUE))> + &select_lex->where, TRUE,with_ret?select_lex:NULL))I don't like that we end up passing select_lex to mysql_prepare_insert.Can we make the arguments of this function be strictly what is necessary,not the full SELECT_LEX structure?> DBUG_RETURN(TRUE);> > + if (with_ret)> + (void )sel_res->prepare(returning_list, NULL);> +> DBUG_ASSERT(select_lex- >leaf_tables.elements != 0);> List_iterator<TABLE_LIST> ti(select_lex->leaf_tables);> TABLE_LIST *table;> uint insert_tables;> > +> if (select_lex- >first_cond_optimization)> {> /* Back up leaf_tables list. */> @@ -3630,6 +3677,8 @@ select_insert::select_insert(THD *thd_arg, TABLE_LIST *table_list_par,> info.update_values= update_values;> info.view= (table_list_par->view ? table_list_par : 0);> info.table_list= table_list_par;> + sel_result= result;> + with_returning_list= with_ret_list;Can we get rid of with_returning_list and just use sel_result? If it's null,with_returning_list should also be null.> }> > > @@ -3651,7 +3701,28 @@ select_insert::prepare(List<Item> &values, SELECT_LEX_UNIT *u)> */> lex->current_select= lex->first_select_lex();> > - res= (setup_fields(thd, Ref_ptr_array(),> + /*> + We want the returning_list to point to insert table. But the context is masked. > + So we swap it with the context saved during parsing stage. > + */> + if(with_returning_list) > + { swap_context(select_lex- >table_list.saved_first,select_lex->table_list.first);> + swap_context(select_lex->context.saved_table_list,select_lex- >context.table_list);> + swap_context(select_lex- >context.saved_name_resolution_table,select_lex- >context.first_name_resolution_table);> +> + res=((select_lex- >with_wild && setup_wild(thd, table_list, select_lex->returning_list, NULL, select_lex->with_wild,> + &select_lex- >hidden_bit_fields)) ||> + setup_fields(thd, Ref_ptr_array(),> + select_lex->returning_list, MARK_COLUMNS_READ, 0, NULL, 0));> +> + /*Swap it back to retore the previous state for the rest of the function*/> +> + swap_context(select_lex->table_list.saved_first,select_lex- >table_list.first);> + swap_context(select_lex- >context.saved_table_list,select_lex->context.table_list);> + swap_context(select_lex->context.saved_name_resolution_table, select_lex->context.first_name_resolution_table);> + }I am not sure this will work if you have columns with the same names, bothin t1 and t2 (your tests don't cover this).Ex:CREATE table t1 (pk int);CREATE table t2 (pk int);INSERT INTO t2 (pk) values (1), (2); INSERT INTO t1 SELECT * FROM t2 RETURNING pk; Also, what about INSERT INTO t1 SELECT .. FROM t1? Please add a test casefor this and see if the code still continues to work. Also, the saved_first introduction into SQL_I_List is really a hack.One needs to save the correct list of tables separately, properly. Pleasefind a solution without changing SQL_I_List. If this part gets you stuck,we should discuss it, but I want you first to try and find a proper solutionfirst.> +> + res= res || (setup_fields(thd, Ref_ptr_array(),> values, MARK_COLUMNS_READ, 0, NULL, 0) ||> check_insert_fields(thd, table_list, *fields, values,> !insert_into_view, 1, &map));> @@ -4046,7 +4136,10 @@ bool select_insert::send_ok_packet() {> thd- >first_successful_insert_id_in_prev_stmt :> (info.copied ? autoinc_value_of_last_inserted_row : 0));> > - ::my_ok(thd, row_count, id, message);> + if (with_returning_list)> + sel_r esult->send_eof();> + else> + ::my_ok(thd, row_count, id, message);I believe you need to return my_ok regardless.> > DBUG_RETURN(false);> }> diff --git a/sql/sql_parse.cc b/sql/sql_parse.cc> index 01d0ed1c383..6cee252096b 100644> --- a/sql/sql_parse.cc> +++ b/sql/sql_parse.cc> @@ -4604,11 +4664,21 @@ mysql_execute_command(THD *thd)> TODO: fix it by removing the front element (restoring of it should> be done properly as well)> */> + /*> + If items are present in returning_list, then we need those items to point to > + INSERT table during setup_fields() and setup_wild(). But it gets masked before that. > + So we save the values in saved_first, saved_table_list and saved_first_name_resolution_context.> + before they are masked.> + */> + select_lex- >table_list.saved_first=select_lex->table_list.first;Right, this is a hack. Please check my other comment about adding saved_first.Please find a way to get the correct list of tables for name resolutionwithout extending SQL_I_List.> select_lex->table_list.first= second_table;> + select_lex- >context.saved_table_list=select_lex->context.table_list;> + selec t_lex->context.saved_name_resolution_table=> + selec t_lex->context.first_name_resolution_table;> select_lex- >context.table_list= > select_lex- >context.first_name_resolution_table= second_table;> - res= mysql_insert_select_prepare(thd);> - if (!res && (sel_result= new (thd->mem_root) select_insert(thd,> + res= mysql_insert_select_prepare(thd, lex->result ? lex->result : result);> + if (!res && (sel_result= new (thd->mem_root) select_insert(with_returning_list, lex->result ? lex->result : result, thd,> fir st_table,> first_table- >table,> &lex->field_list,> diff --git a/sql/sql_yacc.yy b/sql/sql_yacc.yy> index 183a2504b70..3ef53350101 100644> --- a/sql/sql_yacc.yy> +++ b/sql/sql_yacc.yy> @@ -13333,10 +13338,15 @@ replace:> }> insert_field_spec> {> - Lex->pop_select(); //main select> - if (Lex- >check_main_unit_semantics())> - MYSQL_YYABORT;> + Lex->current_select->returning_list.swap(Lex- >current_select->item_list);> }> + opt_select_ex pressions> + {> + Lex->current_select- >returning_list.swap(Lex->current_select->item_list);> + Lex->pop_select(); //main select> + if (Lex- >check_main_unit_semantics())> + MYSQL_YYABORT;> + }Please fix indentation.> ;> > insert_lock_option:

2 2