Re: [Maria-developers] [Commits] 5f02c66: MDEV-11227 - mysqlimport -l doesn't issue UNLOCK TABLES
Hi, Sergey! On Dec 13, Sergey Vojtovich wrote:
revision-id: 5f02c661f3d30baf08071acad29cef40fad0739c (mariadb-10.2.2-225-g5f02c66) parent(s): 65b4d7457e40ee25302d2df28b0d200ff59d9e6d committer: Sergey Vojtovich timestamp: 2016-12-13 16:17:49 +0400 message:
MDEV-11227 - mysqlimport -l doesn't issue UNLOCK TABLES
Implementation of MDEV-7660 introduced unwanted incompatible change: modifications under LOCK TABLES with autocommit enabled are rolled back on disconnect. Previously everything was committed, because LOCK TABLES didn't adjust autocommit setting.
Thanks, very helpful comment (that I didn't quote in my reply for brevity)! Few questions below:
diff --git a/mysql-test/t/delayed.test b/mysql-test/t/delayed.test index dea16c8..30fb694 100644 --- a/mysql-test/t/delayed.test +++ b/mysql-test/t/delayed.test @@ -319,7 +319,7 @@ drop table if exists t1; --enable_warnings create table t1 (a int, b int); insert into t1 values (1,1); -lock table t1 read; +lock table t1 read local;
why?
connect (update,localhost,root,,); connection update; --send insert delayed into t1 values (2,2); diff --git a/sql/mdl.h b/sql/mdl.h index 7d659af..d77c07c 100644 --- a/sql/mdl.h +++ b/sql/mdl.h @@ -196,6 +196,12 @@ enum enum_mdl_type { */ MDL_SHARED_UPGRADABLE, /* + A shared metadata lock for cases when we need to read data from table + and block all concurrent modifications to it (for both data and metadata). + Used by LOCK TABLES READ statement. + */ + MDL_SHARED_READ_ONLY,
Hmm, stronger than MDL_SHARED_WRITE? All read locks were before write locks until now. Why is that changed?
+ /* An upgradable shared metadata lock which blocks all attempts to update table data, allowing reads. A connection holding this kind of lock can read table metadata and read diff --git a/sql/sql_base.cc b/sql/sql_base.cc index 1ca7e9c..d3f4517 100644 --- a/sql/sql_base.cc +++ b/sql/sql_base.cc @@ -1496,6 +1496,7 @@ bool open_table(THD *thd, TABLE_LIST *table_list, Open_table_context *ot_ctx) to general/slow log would be disabled in read only transactions. */ if (table_list->mdl_request.type >= MDL_SHARED_WRITE && + table_list->mdl_request.type != MDL_SHARED_READ_ONLY &&
if you're going to keep read and write locks mixed, then I'd suggest to move this condition into a small inline helper, like table_list->mdl_request.is_write_lock(). it's used few times already in this very file and there will be only one place to change if we'll need to add, say, MDL_SHARED_WRITE_LOW_PRIO
thd->tx_read_only && !(flags & (MYSQL_LOCK_LOG_TABLE | MYSQL_OPEN_HAS_MDL_LOCK))) { diff --git a/sql/sql_parse.cc b/sql/sql_parse.cc index 6dc9192..0d740bb 100644 --- a/sql/sql_parse.cc +++ b/sql/sql_parse.cc @@ -2728,27 +2728,76 @@ bool sp_process_definer(THD *thd) static bool lock_tables_open_and_lock_tables(THD *thd, TABLE_LIST *tables) { Lock_tables_prelocking_strategy lock_tables_prelocking_strategy; + MDL_deadlock_and_lock_abort_error_handler deadlock_handler; + MDL_savepoint mdl_savepoint= thd->mdl_context.mdl_savepoint(); uint counter; TABLE_LIST *table;
thd->in_lock_tables= 1;
+retry: + if (open_tables(thd, &tables, &counter, 0, &lock_tables_prelocking_strategy)) goto err;
- /* - We allow to change temporary tables even if they were locked for read - by LOCK TABLES. To avoid a discrepancy between lock acquired at LOCK - TABLES time and by the statement which is later executed under LOCK TABLES - we ensure that for temporary tables we always request a write lock (such - discrepancy can cause problems for the storage engine). - We don't set TABLE_LIST::lock_type in this case as this might result in - extra warnings from THD::decide_logging_format() even though binary logging - is totally irrelevant for LOCK TABLES. - */ for (table= tables; table; table= table->next_global) - if (!table->placeholder() && table->table->s->tmp_table) - table->table->reginfo.lock_type= TL_WRITE; + { + if (!table->placeholder()) + { + if (table->table->s->tmp_table) + { + /* + We allow to change temporary tables even if they were locked for read + by LOCK TABLES. To avoid a discrepancy between lock acquired at LOCK + TABLES time and by the statement which is later executed under LOCK + TABLES we ensure that for temporary tables we always request a write + lock (such discrepancy can cause problems for the storage engine). + We don't set TABLE_LIST::lock_type in this case as this might result + in extra warnings from THD::decide_logging_format() even though + binary logging is totally irrelevant for LOCK TABLES. + */ + table->table->reginfo.lock_type= TL_WRITE; + } + else if (table->mdl_request.type == MDL_SHARED_READ && + ! table->prelocking_placeholder && + table->table->file->lock_count() == 0) + { + /* + In case when LOCK TABLE ... READ LOCAL was issued for table with + storage engine which doesn't support READ LOCAL option and doesn't + use THR_LOCK locks we need to upgrade weak SR metadata lock acquired + in open_tables() to stronger SRO metadata lock. + This is not needed for tables used through stored routines or + triggers as we always acquire SRO (or even stronger SNRW) metadata + lock for them. + */ + deadlock_handler.init(); + thd->push_internal_handler(&deadlock_handler); + + bool result= thd->mdl_context.upgrade_shared_lock( + table->table->mdl_ticket, + MDL_SHARED_READ_ONLY, + thd->variables.lock_wait_timeout); + + thd->pop_internal_handler(); + + if (deadlock_handler.need_reopen()) + { + /* + Deadlock occurred during upgrade of metadata lock. + Let us restart acquring and opening tables for LOCK TABLES.
will you use MDL_SHARED_READ_ONLY right away when retrying? or it'll do the upgrade thing again?
+ */ + close_tables_for_reopen(thd, &tables, mdl_savepoint); + if (thd->open_temporary_tables(tables)) + goto err; + goto retry; + } + + if (result) + goto err; + } + } + }
if (lock_tables(thd, tables, counter, 0) || thd->locked_tables_list.init_locked_tables(thd))
Regards, Sergei Chief Architect MariaDB and security@mariadb.org
Hi Sergei, Thanks for your feedback. Answers inline. On Thu, Dec 15, 2016 at 02:23:03PM +0100, Sergei Golubchik wrote:
Hi, Sergey!
On Dec 13, Sergey Vojtovich wrote:
revision-id: 5f02c661f3d30baf08071acad29cef40fad0739c (mariadb-10.2.2-225-g5f02c66) parent(s): 65b4d7457e40ee25302d2df28b0d200ff59d9e6d committer: Sergey Vojtovich timestamp: 2016-12-13 16:17:49 +0400 message:
MDEV-11227 - mysqlimport -l doesn't issue UNLOCK TABLES
Implementation of MDEV-7660 introduced unwanted incompatible change: modifications under LOCK TABLES with autocommit enabled are rolled back on disconnect. Previously everything was committed, because LOCK TABLES didn't adjust autocommit setting.
Thanks, very helpful comment (that I didn't quote in my reply for brevity)!
Few questions below:
diff --git a/mysql-test/t/delayed.test b/mysql-test/t/delayed.test index dea16c8..30fb694 100644 --- a/mysql-test/t/delayed.test +++ b/mysql-test/t/delayed.test @@ -319,7 +319,7 @@ drop table if exists t1; --enable_warnings create table t1 (a int, b int); insert into t1 values (1,1); -lock table t1 read; +lock table t1 read local;
why? This is remnant of intermediate patch, when MDL_SHARED_READ_ONLY was acquired for MyISAM too. It'd block subsequent "insert delayed" much earlier, making this test useless.
I reverted this change. Thanks for spotting!
connect (update,localhost,root,,); connection update; --send insert delayed into t1 values (2,2); diff --git a/sql/mdl.h b/sql/mdl.h index 7d659af..d77c07c 100644 --- a/sql/mdl.h +++ b/sql/mdl.h @@ -196,6 +196,12 @@ enum enum_mdl_type { */ MDL_SHARED_UPGRADABLE, /* + A shared metadata lock for cases when we need to read data from table + and block all concurrent modifications to it (for both data and metadata). + Used by LOCK TABLES READ statement. + */ + MDL_SHARED_READ_ONLY,
Hmm, stronger than MDL_SHARED_WRITE? All read locks were before write locks until now. Why is that changed?
I don't think your statement is completely valid: there's MDL_SHARED_NO_WRITE (which is data read lock) after MDL_SHARED_READ_ONLY. Also I don't completely understand how to evalutate strength of MDL_SHARED_* locks by their position. They all do different things and have rather complex compatibility and weight matrices. This position was chosen by MySQL. I can only guess that they want to keep locks up to SW to be used by DML and locks starting with SU used by DDL or LOCK TABLES.
+ /* An upgradable shared metadata lock which blocks all attempts to update table data, allowing reads. A connection holding this kind of lock can read table metadata and read diff --git a/sql/sql_base.cc b/sql/sql_base.cc index 1ca7e9c..d3f4517 100644 --- a/sql/sql_base.cc +++ b/sql/sql_base.cc @@ -1496,6 +1496,7 @@ bool open_table(THD *thd, TABLE_LIST *table_list, Open_table_context *ot_ctx) to general/slow log would be disabled in read only transactions. */ if (table_list->mdl_request.type >= MDL_SHARED_WRITE && + table_list->mdl_request.type != MDL_SHARED_READ_ONLY &&
if you're going to keep read and write locks mixed, then I'd suggest to move this condition into a small inline helper, like table_list->mdl_request.is_write_lock(). it's used few times already in this very file and there will be only one place to change if we'll need to add, say, MDL_SHARED_WRITE_LOW_PRIO
I can merge this from MySQL, it is called is_write_lock_request() there. In fact MySQL didn't have to change this condition for MDL_SHARED_WRITE_LOW_PRIO.
thd->tx_read_only && !(flags & (MYSQL_LOCK_LOG_TABLE | MYSQL_OPEN_HAS_MDL_LOCK))) { diff --git a/sql/sql_parse.cc b/sql/sql_parse.cc index 6dc9192..0d740bb 100644 --- a/sql/sql_parse.cc +++ b/sql/sql_parse.cc @@ -2728,27 +2728,76 @@ bool sp_process_definer(THD *thd) static bool lock_tables_open_and_lock_tables(THD *thd, TABLE_LIST *tables) { Lock_tables_prelocking_strategy lock_tables_prelocking_strategy; + MDL_deadlock_and_lock_abort_error_handler deadlock_handler; + MDL_savepoint mdl_savepoint= thd->mdl_context.mdl_savepoint(); uint counter; TABLE_LIST *table;
thd->in_lock_tables= 1;
+retry: + if (open_tables(thd, &tables, &counter, 0, &lock_tables_prelocking_strategy)) goto err;
- /* - We allow to change temporary tables even if they were locked for read - by LOCK TABLES. To avoid a discrepancy between lock acquired at LOCK - TABLES time and by the statement which is later executed under LOCK TABLES - we ensure that for temporary tables we always request a write lock (such - discrepancy can cause problems for the storage engine). - We don't set TABLE_LIST::lock_type in this case as this might result in - extra warnings from THD::decide_logging_format() even though binary logging - is totally irrelevant for LOCK TABLES. - */ for (table= tables; table; table= table->next_global) - if (!table->placeholder() && table->table->s->tmp_table) - table->table->reginfo.lock_type= TL_WRITE; + { + if (!table->placeholder()) + { + if (table->table->s->tmp_table) + { + /* + We allow to change temporary tables even if they were locked for read + by LOCK TABLES. To avoid a discrepancy between lock acquired at LOCK + TABLES time and by the statement which is later executed under LOCK + TABLES we ensure that for temporary tables we always request a write + lock (such discrepancy can cause problems for the storage engine). + We don't set TABLE_LIST::lock_type in this case as this might result + in extra warnings from THD::decide_logging_format() even though + binary logging is totally irrelevant for LOCK TABLES. + */ + table->table->reginfo.lock_type= TL_WRITE; + } + else if (table->mdl_request.type == MDL_SHARED_READ && + ! table->prelocking_placeholder && + table->table->file->lock_count() == 0) + { + /* + In case when LOCK TABLE ... READ LOCAL was issued for table with + storage engine which doesn't support READ LOCAL option and doesn't + use THR_LOCK locks we need to upgrade weak SR metadata lock acquired + in open_tables() to stronger SRO metadata lock. + This is not needed for tables used through stored routines or + triggers as we always acquire SRO (or even stronger SNRW) metadata + lock for them. + */ + deadlock_handler.init(); + thd->push_internal_handler(&deadlock_handler); + + bool result= thd->mdl_context.upgrade_shared_lock( + table->table->mdl_ticket, + MDL_SHARED_READ_ONLY, + thd->variables.lock_wait_timeout); + + thd->pop_internal_handler(); + + if (deadlock_handler.need_reopen()) + { + /* + Deadlock occurred during upgrade of metadata lock. + Let us restart acquring and opening tables for LOCK TABLES.
will you use MDL_SHARED_READ_ONLY right away when retrying? or it'll do the upgrade thing again?
Upgrade thing again, which is farily stupid. That's how it was in original code and I didn't change it in a hurry. Thanks, Sergey
Hi, Sergey! On Dec 15, Sergey Vojtovich wrote:
+ /* An upgradable shared metadata lock which blocks all attempts to update table data, allowing reads. A connection holding this kind of lock can read table metadata and read diff --git a/sql/sql_base.cc b/sql/sql_base.cc index 1ca7e9c..d3f4517 100644 --- a/sql/sql_base.cc +++ b/sql/sql_base.cc @@ -1496,6 +1496,7 @@ bool open_table(THD *thd, TABLE_LIST *table_list, Open_table_context *ot_ctx) to general/slow log would be disabled in read only transactions. */ if (table_list->mdl_request.type >= MDL_SHARED_WRITE && + table_list->mdl_request.type != MDL_SHARED_READ_ONLY &&
if you're going to keep read and write locks mixed, then I'd suggest to move this condition into a small inline helper, like table_list->mdl_request.is_write_lock(). it's used few times already in this very file and there will be only one place to change if we'll need to add, say, MDL_SHARED_WRITE_LOW_PRIO I can merge this from MySQL, it is called is_write_lock_request() there. In fact MySQL didn't have to change this condition for MDL_SHARED_WRITE_LOW_PRIO.
I think it'd be good. There were three or four places in sql_base.cc where it should replace the >= check.
diff --git a/sql/sql_parse.cc b/sql/sql_parse.cc index 6dc9192..0d740bb 100644 --- a/sql/sql_parse.cc +++ b/sql/sql_parse.cc ... + thd->pop_internal_handler(); + + if (deadlock_handler.need_reopen()) + { + /* + Deadlock occurred during upgrade of metadata lock. + Let us restart acquring and opening tables for LOCK TABLES.
will you use MDL_SHARED_READ_ONLY right away when retrying? or it'll do the upgrade thing again? Upgrade thing again, which is farily stupid. That's how it was in original code and I didn't change it in a hurry.
Can you do it? It's probably just updating the lock in the corresponding table_list entry. Still, better do it as a separate change, after you push this MDEV-11227 commit, so that 10.2.3 wouldn't have to wait for it. Regards, Sergei Chief Architect MariaDB and security@mariadb.org
Hi Sergei, On Thu, Dec 15, 2016 at 04:38:04PM +0100, Sergei Golubchik wrote:
Hi, Sergey!
On Dec 15, Sergey Vojtovich wrote:
+ /* An upgradable shared metadata lock which blocks all attempts to update table data, allowing reads. A connection holding this kind of lock can read table metadata and read diff --git a/sql/sql_base.cc b/sql/sql_base.cc index 1ca7e9c..d3f4517 100644 --- a/sql/sql_base.cc +++ b/sql/sql_base.cc @@ -1496,6 +1496,7 @@ bool open_table(THD *thd, TABLE_LIST *table_list, Open_table_context *ot_ctx) to general/slow log would be disabled in read only transactions. */ if (table_list->mdl_request.type >= MDL_SHARED_WRITE && + table_list->mdl_request.type != MDL_SHARED_READ_ONLY &&
if you're going to keep read and write locks mixed, then I'd suggest to move this condition into a small inline helper, like table_list->mdl_request.is_write_lock(). it's used few times already in this very file and there will be only one place to change if we'll need to add, say, MDL_SHARED_WRITE_LOW_PRIO I can merge this from MySQL, it is called is_write_lock_request() there. In fact MySQL didn't have to change this condition for MDL_SHARED_WRITE_LOW_PRIO.
I think it'd be good. There were three or four places in sql_base.cc where it should replace the >= check. Ok.
diff --git a/sql/sql_parse.cc b/sql/sql_parse.cc index 6dc9192..0d740bb 100644 --- a/sql/sql_parse.cc +++ b/sql/sql_parse.cc ... + thd->pop_internal_handler(); + + if (deadlock_handler.need_reopen()) + { + /* + Deadlock occurred during upgrade of metadata lock. + Let us restart acquring and opening tables for LOCK TABLES.
will you use MDL_SHARED_READ_ONLY right away when retrying? or it'll do the upgrade thing again? Upgrade thing again, which is farily stupid. That's how it was in original code and I didn't change it in a hurry.
Can you do it? It's probably just updating the lock in the corresponding table_list entry.
Still, better do it as a separate change, after you push this MDEV-11227 commit, so that 10.2.3 wouldn't have to wait for it.
Ok. Should I consider this patch is approved? It is possible to push it now, but taking into account complexity and bb state: should we postpone it until bb is greener? Regards, Sergey
participants (2)
-
Sergei Golubchik -
Sergey Vojtovich