maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

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@xxxxxxxxxxx