- developers - lists.mariadb.org

[Maria-developers] INSERT ... RETURNING intermediate review
by Vicențiu Ciorbaru 21 Jul '19

21 Jul '19

Hi Rucha! I've spent quite a bit of time looking at the implementation. It is working and passes almost all test cases, although you forgot to also commit the insert_parser updated result file. Good job so far. I want to give you something to work on and think about for a bit before we do another overall overview of the whole project. Hopefully this will help you across all your coding endeavours, not just now. What I want you to learn is that it is important to think about how your code can and will evolve in the future. Just getting things to work is ok up to a certain point, but not implementing things in a decoupled fashion is what we call "hacks". These hacks are to be avoided as much as possible as it leads to much more difficult work down the line. Let's elaborate on the topic. There are a number of problems with our current code (hacks). I am not referring to yours in particular. All these hacks are a bit difficult to overcome in one go. I do not expect you to fix them yourself, not during this GSoC at least, but it is something to consider and if you are willing to work towards fixing some of them, excellent! Historically our codebase evolved from a simple parser to the "monster" that you see now. Sometimes due to time constraints, things were not implemented in the most extendable way, rather they made use of certain particular constructs, only valid in the contexts they were written in. Keyword here is context, as you'll soon see. Some of this contain things you have interacted with so far in your project. Here is an example that you've had to deal with: A SELECT statement has what we call a SELECT LIST, this list is traditionally stored in SELECT_LEX::item_list. This item_list however is overloaded at times. For example DELETE ... RETURNING uses it to store the expressions part of the RETURNING clause. This overloading generally works ok, unless you run into situations like you did with your INSERT ... RETURNING project. INSERT ... RETURNING clauses already made use of the item_list to store something else. Now you were forced to introduce another list in SELECT_LEX, which we called returning_list. Everything ok so far. But then, you ran into another problem, the bison grammar rules do not put Items in the list you want. They always used item_list, so you could not use your returning_list. So what did we do then? Well, we came up with another hack! We suggested this hack to you because it is something that will work quickly and get you unstuck and that is to use the same grammar rules like before, but swap the item_list with the returning list temporarily, such that the grammar rules will put the right items in the right list. This works, but it masks the underlying problem. The problem is that we have a very rigid implementation for select_item_list grammar rule. What we should do is to make select_item_list grammar return a list using the bison stack. That means no relying on SELECT_LEX::item_list to store our result temporarily. You have done steps towards fixing this, which brings us to where your code's current state. The problem with your implementation is you have not lifted the restriction of the grammar rules making use of SELECT_LIST::item_list. Instead, you have masked it by returning the address of that member on the bison stack. The funny part is that this works, but it still a hack. It is still a hack, because these grammar rules have a side effect of modifying this member behind the scenes. A very, very, very (!) good rule is to consider all side effects as bad, especially now that you are starting out. With experience you might get away with them every now-and-then, but for now avoid them like the plague and try to remove them whenever possible. The current code makes use of a function I really don't like. The inline function add_item_list(). It is one of those hacky functions that strictly relies on context. The context is: take the thd->lex->current_select and put the item passed as a parameter in the item_list. The diff I've attached shows an implementation of how to use the bison stack properly. I took inspiration from the "expr" rule. Unfortunately the hack chain goes deeper, but we need to do baby steps and some things are best left outside your GSoC project, or we risk going down a rabbit whole we might not get out of. One other issue I've observed is with the REPLACE code here: insert_field_spec { if ($6) { List<Item> list; list=*($6); Lex->current_select- >item_list.empty(); $6=&list; } } opt_ select_expressions { if ($8) Lex- >returning_list=*($8); if ($6) Lex->current_select- >item_list=*($6); Lex->pop_select(); //main select if (Lex->check_main_unit_semantics()) MYSQL_YYABORT; } This is really problematic and your probably wrote it because of opt_select_expressions putting stuff into item_list. In the insert_field_spec rule, you receive the address of current_select->item_list as parameter $6. You store the value of this list (basically the pointer to the head element) in a temporary variable that is only in scope during the if statement (!). Attempting to use it after the if statement is over (via the $6 parameter) is illegal! It only works because the compiler did not chose to use that area of memory for something else. Please find a way to store things here properly. After this is fixed, we'll spend the next month cleaning everything up, documenting our code, the added test cases etc. We have an overall working solution, but we need to make it as hack-free as possible, so we can later work on supporting INSERT ... RETURNING in subqueries. Let me know if anything in the email is unclear. It took me a while to write it, trying to explain the reasoning, but I may have missed a few things. :) Vicențiu

2 2

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 17 Jul '19

17 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