- developers - lists.mariadb.org

Re: [Maria-developers] [Commits] 1e58c579f5b: MDEV-18930: Failed CREATE OR REPLACE TEMPORARY not written into binary log makes data on master and slave diverge
by Sachin Setiya 02 Aug '19

02 Aug '19

Hi Sujatha! On Thu, Aug 1, 2019 at 4:16 PM sujatha <sujatha.sivakumar(a)mariadb.com> wrote: > > revision-id: 1e58c579f5b61644a74045a0470d2c62b343eb75 (mariadb-10.1.41-3-g1e58c579f5b) > parent(s): 0bb8f33b55cc016d0ede86b97990a3ee30dcb069 > author: Sujatha > committer: Sujatha > timestamp: 2019-08-01 16:16:05 +0530 > message: > > MDEV-18930: Failed CREATE OR REPLACE TEMPORARY not written into binary log makes data on master and slave diverge > > Problem: > ======= > Failed CREATE OR REPLACE TEMPORARY TABLE statement which dropped the table but > failed at a later stage of creation of temporary table is not written to > binarylog in row based replication. This causes the slave to diverge. > > Analysis: > ======== > CREATE OR REPLACE statements work as shown below. > > CREATE OR REPLACE TABLE table_name (a int); > is basically the same as: > > DROP TABLE IF EXISTS table_name; > CREATE TABLE table_name (a int); > > Hence every CREATE OR REPLACE TABLE command which dropped the table should be > written to binary log, even when following CREATE TABLE part fails. In order > to achieve this, during the execution of CREATE OR REPLACE command, when a > table is dropped 'thd->log_current_statement' flag is set. When table creation > results in an error within 'mysql_create_table' code, the error handling part > looks for this flag. If it is set the failed CREATE OR REPLACE statement is > written into the binary log inspite of error. This ensure that slave doesn't > diverge from the master. In case of row based replication the error handling > code returns very early, if the table is of type temporary. This is done based > on the assumption that temporary tables are not replicated in row based > replication. > > It fails to handle the cases where a temporary table was created as part of > statement based replication at an earlier stage and the binary log format was > changed to row because of an unsafe statement. In this case when a CREATE OR > REPLACE statement is executed on this temporary table it will dropped but the > query will not be written to binary log. Hence slave diverges. > > Fix: > === > In error handling code check the return status of create table operation. If > it is successful and replication mode is row based and table is of type > temporary then return. Other wise proceed further to the code which checks for > thd->log_current_statement flag and does appropriate logging. > > --- > .../suite/rpl/r/rpl_create_or_replace_fail.result | 18 +++++++ > .../suite/rpl/t/rpl_create_or_replace_fail.test | 56 ++++++++++++++++++++++ > sql/sql_table.cc | 2 +- > 3 files changed, 75 insertions(+), 1 deletion(-) > > diff --git a/mysql-test/suite/rpl/r/rpl_create_or_replace_fail.result b/mysql-test/suite/rpl/r/rpl_create_or_replace_fail.result > new file mode 100644 > index 00000000000..57178f0efbe > --- /dev/null > +++ b/mysql-test/suite/rpl/r/rpl_create_or_replace_fail.result > @@ -0,0 +1,18 @@ > +include/master-slave.inc > +[connection master] > +CREATE TEMPORARY TABLE t1 (a INT NOT NULL); > +LOAD DATA INFILE 'x' INTO TABLE x; > +ERROR 42S02: Table 'test.x' doesn't exist > +CREATE OR REPLACE TEMPORARY TABLE t1 (x INT) PARTITION BY HASH(x); > +ERROR HY000: Cannot create temporary table with partitions > +"************** DROP TEMPORARY TABLE Should be present in Binary log **************" > +include/show_binlog_events.inc > +Log_name Pos Event_type Server_id End_log_pos Info > +master-bin.000001 # Gtid # # GTID #-#-# > +master-bin.000001 # Query # # use `test`; CREATE TEMPORARY TABLE t1 (a INT NOT NULL) > +master-bin.000001 # Gtid # # GTID #-#-# > +master-bin.000001 # Query # # use `test`; CREATE OR REPLACE TEMPORARY TABLE t1 (x INT) PARTITION BY HASH(x) > +CREATE TABLE t1 (b INT); > +INSERT INTO t1 VALUES (NULL); > +DROP TABLE t1; > +include/rpl_end.inc > diff --git a/mysql-test/suite/rpl/t/rpl_create_or_replace_fail.test b/mysql-test/suite/rpl/t/rpl_create_or_replace_fail.test > new file mode 100644 > index 00000000000..e75f34b0b56 > --- /dev/null > +++ b/mysql-test/suite/rpl/t/rpl_create_or_replace_fail.test > @@ -0,0 +1,56 @@ > +# ==== Purpose ==== > +# > +# Test verifies that failed CREATE OR REPLACE TEMPORARY TABLE statement which > +# dropped the table but failed at a later stage of creation of temporary table > +# is written to binarylog in row based replication. > +# > +# ==== Implementation ==== > +# > +# Steps: > +# 0 - Have mixed based replication mode. > +# 1 - Create a temporary table. It will be replicated as mixed replication > +# mode is in use. > +# 2 - Execute an unsafe statement which will switch current statement > +# binlog format to 'ROW'. i.e If binlog_format=MIXED, there are open > +# temporary tables, and an unsafe statement is executed, then subsequent > +# statements are logged in row format. Why not just set binlog format to row ? > +# 3 - Execute a CREATE OR REPLACE TEMPORARY TABLE statement which tries to > +# create partitions on temporary table. Since it is not supported it will > +# fail. > +# 4 - Check the binary log output to ensure that the failed statement is > +# written to the binary log. > +# 5 - Slave should be up and running and in sync with master. > +# > +# ==== References ==== > +# > +# MDEV-18930: Failed CREATE OR REPLACE TEMPORARY not written into binary log > +# makes data on master and slave diverge > +# > + > +--source include/have_partition.inc > +--source include/have_binlog_format_mixed.inc > +--source include/master-slave.inc > + > +CREATE TEMPORARY TABLE t1 (a INT NOT NULL); > + > +# Execute an unsafe statement which switches replication mode internally from > +# "STATEMENT" to "ROW". > +--error ER_NO_SUCH_TABLE > +LOAD DATA INFILE 'x' INTO TABLE x; > + > +--error ER_PARTITION_NO_TEMPORARY > +CREATE OR REPLACE TEMPORARY TABLE t1 (x INT) PARTITION BY HASH(x); > + > +--echo "************** DROP TEMPORARY TABLE Should be present in Binary log **************" > +--source include/show_binlog_events.inc > + > +CREATE TABLE t1 (b INT); > +INSERT INTO t1 VALUES (NULL); > +--sync_slave_with_master > + > +# Cleanup > +--connection master > +DROP TABLE t1; > + > +--source include/rpl_end.inc > + > diff --git a/sql/sql_table.cc b/sql/sql_table.cc > index 656834c7852..fe923d73200 100644 > --- a/sql/sql_table.cc > +++ b/sql/sql_table.cc > @@ -5092,7 +5092,7 @@ bool mysql_create_table(THD *thd, TABLE_LIST *create_table, > > err: > /* In RBR we don't need to log CREATE TEMPORARY TABLE */ > - if (thd->is_current_stmt_binlog_format_row() && create_info->tmp_table()) > + if (!result && thd->is_current_stmt_binlog_format_row() && create_info->tmp_table()) > DBUG_RETURN(result); Can we only binary log only the drop part ? not the create part. > > if (create_info->tmp_table()) > _______________________________________________ > commits mailing list > commits(a)mariadb.org > https://lists.askmonty.org/cgi-bin/mailman/listinfo/commits -- Regards Sachin Setiya Software Engineer at MariaDB

1 0

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

29 Jul '19

Report for week 9: Hello! This week I worked on documentation, added some comments to the code and removed some lines of code from parser as it was still making use of SELECT_LEX::item_list after opt_select_expressions. I was referring DELETE...RETURNING test case and noticed case for using stored functions was missing. I added that to INSERT...RETURNING and REPLACE...RETURNING. Also checked if the implementation supports other functions like string functions, date-time functions, control flow functions, numeric function and secondary functions(Since the aggregate functions cannot be used, I wanted to check if these functions can be used) I have made related changes in the documentation and my github repo is up to date with latest changes. Regards, Rucha Deodhar

1 0

[Maria-developers] Windows line endings in MariaDB sources again (
by Otto Kekäläinen 23 Jul '19

23 Jul '19

Hello! While trying to release 10.0.26 for Debian and Ubuntu I noticed git chokes again on Windows line endings. As our .gitattributes states that by default git should clean up the line endings, why do we keep getting new Windows line endings in new code? mariadb-10.0$ cat .gitattributes # Set the default behavior, in case people don't have core.autocrlf set. * text=auto ... storage/connect/mysql-test/connect/std_data/*.txt -text mariadb-10.0.26$ find . | xargs file | grep CRLF ./storage/connect/mysql-test/connect/std_data/boyswin.txt: ASCII text, with CRLF line terminators ./storage/connect/mysql-test/connect/std_data/expenses.txt: ASCII text, with CRLF line terminators ./storage/connect/mysql-test/connect/std_data/emp.txt: ASCII text, with CRLF line terminators ./mysql-test/std_data/loaddata7.dat: ASCII text, with CRLF line terminators ./mysql-test/r/loadxml.result: ASCII text, with CRLF, LF line terminators ./mysql-test/r/perror-win.result: ASCII text, with CRLF, LF line terminators ./mysql-test/r/mysql_binary_mode.result: ASCII English text, with CRLF, LF line terminators ./mysql-test/r/func_regexp_pcre.result: UTF-8 Unicode C++ program text, with CRLF, CR, LF line terminators ./pcre/testdata/grepoutputN: ASCII text, with CRLF, CR, LF line terminators ./pcre/testdata/greppatN4: ASCII text, with CRLF line terminators This leads for me to: dpkg-source: info: building mariadb-10.0 using existing ./mariadb-10.0_10.0.26.orig.tar.gz dpkg-source: info: local changes detected, the modified files are: mariadb-10.0/storage/connect/mysql-test/connect/std_data/boyswin.txt mariadb-10.0/storage/connect/mysql-test/connect/std_data/emp.txt mariadb-10.0/storage/connect/mysql-test/connect/std_data/expenses.txt dpkg-source: error: aborting due to unexpected upstream changes, see /tmp/mariadb-10.0_10.0.26-1.diff.Yzwwhw Which means I have to do extra work with importing the upstream sources.. - Otto

2 4

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

22 Jul '19

Report for week 8: Hello! This week I worked on the parser. I removed the hack "swapping lists" we had used earlier. Now the grammar uses values from the bison stack. However, I was making the list rule return the SELECT_LEX :: item_list. This was fixed after the intermediate review. I changed the grammar for INSERT...RETURNING and REPLACE...RETURNING, added test and result files for REPLACE...RETURNING and modified the comments of one test file. I also worked on the intermediate review. Now the parser code doesn't rely on SELECT_LEX :: item_list. In REPLACE...RETURNING a list was declared inside the scope of "if" and using it outside the scope was illegal. So I moved it to the scope outside of if and made the grammar return: &list if select_lex::item_list is not empty else NULL. My github repo is up to date with latest changes. Regards, Rucha Deodhar

1 0

[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 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