Re: [Maria-developers] c75cbc1: MDEV-717 LP:1003679 - Wrong binlog order on concurrent DROP schema and
Hi, Alexey! convention below: when I start a comment with "btw" it means that you don't need to change anything, it's just a thought I had when looking at that line in the patch. On May 03, Alexey Botchkov wrote:
revision-id: c75cbc19806223be8e750c27ba0bd4b17ef61f54 (mariadb-10.1.13-24-gc75cbc1) parent(s): 94cd0f6c9b3b04db67501ef29d470f32527ceda2 committer: Alexey Botchkov timestamp: 2016-05-03 13:20:12 +0400 message:
MDEV-717 LP:1003679 - Wrong binlog order on concurrent DROP schema and CREATE function.
Test case added. That required setting some DEBUG_SYNC points.
diff --git a/mysql-test/suite/binlog/t/binlog_mdev717.test b/mysql-test/suite/binlog/t/binlog_mdev717.test new file mode 100644 index 0000000..5e8cce6 --- /dev/null +++ b/mysql-test/suite/binlog/t/binlog_mdev717.test @@ -0,0 +1,49 @@ +# MDEV-717 LP:1003679 - Wrong binlog order on concurrent DROP schema and CREATE function. + +--source include/have_log_bin.inc +RESET MASTER; + +disable_warnings; +DROP DATABASE IF EXISTS mysqltest; +enable_warnings;
btw, generally this isn't necessary anymore, after-test checks verify that every test cleans up after itself.
+connect(con1,localhost,root,,);
btw, this works also without extra commas: connect(con1,localhost,root);
+connection default; + +CREATE DATABASE mysqltest; +SET DEBUG_SYNC= "wait_drop_db_name_locked SIGNAL locked WAIT_FOR release"; +--send DROP DATABASE mysqltest; +connection con1; +SET DEBUG_SYNC= "now WAIT_FOR locked";
Hm... If the default connection signals "locked" before you start waiting for it now - will your test still work? You can test it by adding a sleep right after "connection con1". (don't commit the sleep, of course, it's just to way to verify whether the test case is valid) Alternatively, in many places tests wait for a connection to reach the certain debug_sync point with, like: let $wait_condition= select count(*) = 1 from information_schema.processlist where state like "debug sync point: wait_drop_db_name_locked%"; source include/wait_condition.inc;
+SET DEBUG_SYNC= "sp_create_routine_started SIGNAL release"; +--error 0,ER_BAD_DB_ERROR
why? Do you mean that even with your sync points the specific execution order if not guaranteed?
+CREATE FUNCTION mysqltest.f1() RETURNS INT RETURN 1; +connection default; +--reap + +CREATE DATABASE mysqltest; +SET DEBUG_SYNC= "wait_drop_db_name_locked SIGNAL locked1 WAIT_FOR release1"; +--send DROP DATABASE mysqltest; +connection con1; +SET DEBUG_SYNC= "now WAIT_FOR locked1"; +SET DEBUG_SYNC= "create_event_started SIGNAL release1"; +--error 0,ER_BAD_DB_ERROR +CREATE EVENT mysqltest.e1 ON SCHEDULE EVERY 15 MINUTE DO BEGIN END; +connection default; +--reap + +CREATE DATABASE mysqltest; +CREATE EVENT mysqltest.e1 ON SCHEDULE EVERY 15 MINUTE DO BEGIN END; +SET DEBUG_SYNC= "wait_drop_db_name_locked SIGNAL locked1 WAIT_FOR release1"; +--send DROP DATABASE mysqltest; +connection con1; +SET DEBUG_SYNC= "now WAIT_FOR locked1"; +SET DEBUG_SYNC= "update_event_started SIGNAL release1"; +--error 0,ER_BAD_DB_ERROR +ALTER EVENT mysqltest.e1 ON SCHEDULE EVERY 20 MINUTE DO BEGIN END; +connection default; +--reap + +SET DEBUG_SYNC= "RESET"; +--source include/show_binlog_events.inc + diff --git a/sql/events.cc b/sql/events.cc index 77dbb8b..3c50d6e 100644 diff --git a/sql/sp.cc b/sql/sp.cc index a518b52..c2e3fd2 100644 --- a/sql/sp.cc +++ b/sql/sp.cc @@ -1043,6 +1043,8 @@ sp_create_routine(THD *thd, stored_procedure_type type, sp_head *sp) DBUG_ASSERT(type == TYPE_ENUM_PROCEDURE || type == TYPE_ENUM_FUNCTION);
+ DEBUG_SYNC(thd, "sp_create_routine_started"); +
could you use "before_wait_locked_pname" sync point instead? It's at the beginning of lock_object_name().
/* Grab an exclusive MDL lock. */ if (lock_object_name(thd, mdl_type, sp->m_db.str, sp->m_name.str)) { diff --git a/sql/sql_db.cc b/sql/sql_db.cc index 2ba67cb..fdb49f2 100644 --- a/sql/sql_db.cc +++ b/sql/sql_db.cc @@ -837,6 +837,8 @@ mysql_rm_db_internal(THD *thd,char *db, bool if_exists, bool silent) if (lock_schema_name(thd, dbnorm)) DBUG_RETURN(true);
+ DEBUG_SYNC(thd, "wait_drop_db_name_locked"); +
Could you use "after_wait_locked_schema_name" sync point instead? It's the one at the end of lock_schema_name().
length= build_table_filename(path, sizeof(path) - 1, db, "", "", 0); strmov(path+length, MY_DB_OPT_FILE); // Append db option file name del_dbopt(path); // Remove dboption hash entry
Regards, Sergei Chief Architect MariaDB and security@mariadb.org
Hm... If the default connection signals "locked" before you start waiting for it now - will your test still work?
Well the DEBUG_SYNC manual seems to be explicit about it: " A signal remains in effect until it is overwritten. If conn1 signals 'opened' before conn2 reaches 'now', conn2 will still find the 'opened' signal. It does not wait in this case. "
+SET DEBUG_SYNC= "sp_create_routine_started SIGNAL release"; +--error 0,ER_BAD_DB_ERROR
why? Do you mean that even with your sync points the specific execution order if not guaranteed?
Not sure i understand your question properly. But i mean, getting the BAD_DB_ERROR is the execution result i desire here. Best regards. HF 03.05.2016 21:38, Sergei Golubchik wrote:
Hi, Alexey!
convention below: when I start a comment with "btw" it means that you don't need to change anything, it's just a thought I had when looking at that line in the patch.
On May 03, Alexey Botchkov wrote:
revision-id: c75cbc19806223be8e750c27ba0bd4b17ef61f54 (mariadb-10.1.13-24-gc75cbc1) parent(s): 94cd0f6c9b3b04db67501ef29d470f32527ceda2 committer: Alexey Botchkov timestamp: 2016-05-03 13:20:12 +0400 message:
MDEV-717 LP:1003679 - Wrong binlog order on concurrent DROP schema and CREATE function.
Test case added. That required setting some DEBUG_SYNC points.
diff --git a/mysql-test/suite/binlog/t/binlog_mdev717.test b/mysql-test/suite/binlog/t/binlog_mdev717.test new file mode 100644 index 0000000..5e8cce6 --- /dev/null +++ b/mysql-test/suite/binlog/t/binlog_mdev717.test @@ -0,0 +1,49 @@ +# MDEV-717 LP:1003679 - Wrong binlog order on concurrent DROP schema and CREATE function. + +--source include/have_log_bin.inc +RESET MASTER; + +disable_warnings; +DROP DATABASE IF EXISTS mysqltest; +enable_warnings; btw, generally this isn't necessary anymore, after-test checks verify that every test cleans up after itself.
+connect(con1,localhost,root,,); btw, this works also without extra commas:
connect(con1,localhost,root);
+connection default; + +CREATE DATABASE mysqltest; +SET DEBUG_SYNC= "wait_drop_db_name_locked SIGNAL locked WAIT_FOR release"; +--send DROP DATABASE mysqltest; +connection con1; +SET DEBUG_SYNC= "now WAIT_FOR locked"; Hm... If the default connection signals "locked" before you start waiting for it now - will your test still work? You can test it by adding a sleep right after "connection con1". (don't commit the sleep, of course, it's just to way to verify whether the test case is valid)
Alternatively, in many places tests wait for a connection to reach the certain debug_sync point with, like:
let $wait_condition= select count(*) = 1 from information_schema.processlist where state like "debug sync point: wait_drop_db_name_locked%"; source include/wait_condition.inc;
+SET DEBUG_SYNC= "sp_create_routine_started SIGNAL release"; +--error 0,ER_BAD_DB_ERROR why? Do you mean that even with your sync points the specific execution order if not guaranteed?
+CREATE FUNCTION mysqltest.f1() RETURNS INT RETURN 1; +connection default; +--reap + +CREATE DATABASE mysqltest; +SET DEBUG_SYNC= "wait_drop_db_name_locked SIGNAL locked1 WAIT_FOR release1"; +--send DROP DATABASE mysqltest; +connection con1; +SET DEBUG_SYNC= "now WAIT_FOR locked1"; +SET DEBUG_SYNC= "create_event_started SIGNAL release1"; +--error 0,ER_BAD_DB_ERROR +CREATE EVENT mysqltest.e1 ON SCHEDULE EVERY 15 MINUTE DO BEGIN END; +connection default; +--reap + +CREATE DATABASE mysqltest; +CREATE EVENT mysqltest.e1 ON SCHEDULE EVERY 15 MINUTE DO BEGIN END; +SET DEBUG_SYNC= "wait_drop_db_name_locked SIGNAL locked1 WAIT_FOR release1"; +--send DROP DATABASE mysqltest; +connection con1; +SET DEBUG_SYNC= "now WAIT_FOR locked1"; +SET DEBUG_SYNC= "update_event_started SIGNAL release1"; +--error 0,ER_BAD_DB_ERROR +ALTER EVENT mysqltest.e1 ON SCHEDULE EVERY 20 MINUTE DO BEGIN END; +connection default; +--reap + +SET DEBUG_SYNC= "RESET"; +--source include/show_binlog_events.inc + diff --git a/sql/events.cc b/sql/events.cc index 77dbb8b..3c50d6e 100644 diff --git a/sql/sp.cc b/sql/sp.cc index a518b52..c2e3fd2 100644 --- a/sql/sp.cc +++ b/sql/sp.cc @@ -1043,6 +1043,8 @@ sp_create_routine(THD *thd, stored_procedure_type type, sp_head *sp) DBUG_ASSERT(type == TYPE_ENUM_PROCEDURE || type == TYPE_ENUM_FUNCTION);
+ DEBUG_SYNC(thd, "sp_create_routine_started"); + could you use "before_wait_locked_pname" sync point instead? It's at the beginning of lock_object_name().
/* Grab an exclusive MDL lock. */ if (lock_object_name(thd, mdl_type, sp->m_db.str, sp->m_name.str)) { diff --git a/sql/sql_db.cc b/sql/sql_db.cc index 2ba67cb..fdb49f2 100644 --- a/sql/sql_db.cc +++ b/sql/sql_db.cc @@ -837,6 +837,8 @@ mysql_rm_db_internal(THD *thd,char *db, bool if_exists, bool silent) if (lock_schema_name(thd, dbnorm)) DBUG_RETURN(true);
+ DEBUG_SYNC(thd, "wait_drop_db_name_locked"); +
Could you use "after_wait_locked_schema_name" sync point instead? It's the one at the end of lock_schema_name().
length= build_table_filename(path, sizeof(path) - 1, db, "", "", 0); strmov(path+length, MY_DB_OPT_FILE); // Append db option file name del_dbopt(path); // Remove dboption hash entry
Regards, Sergei Chief Architect MariaDB and security@mariadb.org
_______________________________________________ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
Hi, Alexey! On May 04, Alexey Botchkov wrote:
Hm... If the default connection signals "locked" before you start waiting for it now - will your test still work?
Well the DEBUG_SYNC manual seems to be explicit about it: " A signal remains in effect until it is overwritten. If conn1 signals 'opened' before conn2 reaches 'now', conn2 will still find the 'opened' signal. It does not wait in this case. "
Great. Then replication tests overcomplicate it, as usual.
+SET DEBUG_SYNC= "sp_create_routine_started SIGNAL release"; +--error 0,ER_BAD_DB_ERROR
why? Do you mean that even with your sync points the specific execution order if not guaranteed?
Not sure i understand your question properly. But i mean, getting the BAD_DB_ERROR is the execution result i desire here.
Your test here expects "either ER_BAD_DB_ERROR or a success". I asked - does that mean that even with your debug_sync points the result is still non-deterministic? Regards, Sergei Chief Architect MariaDB and security@mariadb.org
Your test here expects "either ER_BAD_DB_ERROR or a success". I asked - does that mean that even with your debug_sync points the result is still non-deterministic?
Ah, got it. The test is deterministic, so we should only wait for the BAD_DB_ERROR. Will remove the '0'. Best regards. HF.
participants (2)
-
Alexey Botchkov -
Sergei Golubchik