Hello Kostja!
Here is my comments about your patch:
* Konstantin Osipov <kostja@stripped> [09/11/18 21:54]:
> #At file:///opt/local/work/6.0-46224-1/ based on
> revid:tor.didriksen@stripped
>
> 3717 Konstantin Osipov 2009-11-18
> A prerequisite patch for the fix for Bug#46224
> "HANDLER statements within a transaction might lead to deadlocks".
> Introduce a notion of a sentinel to MDL_context. A sentinel
> is a ticket that separates all tickets in the context into two
> groups: before and after it. Currently we can have (and need) only
> one designated sentinel -- it separates all locks taken by LOCK
> TABLE or HANDLER statement, which must survive COMMIT and ROLLBACK
> and all other locks, which must be released at COMMIT and ROLLBACK.
> The tricky part is maintaining the sentinel up to date when
> someone release its corresponding ticket. This can happen, e.g.
> if someone issues DROP TABLE under LOCK TABLES (generally,
> see all calls to release_all_locks_for_name()).
> MDL_context::release_ticket() is modified to take care of it.
>
> ******
> A fix and a test case for Bug#46224 "HANDLER statements within a
> transaction might lead to deadlocks".
>
> An attempt to mix HANDLER SQL and transactions could lead to a deadlock.
IMO this sounds too broad and can be confusing...
I think you should mention that to get deadlock you had to add
concurrent DDL to the mix.
>
> Incompatible change: entering LOCK TABLES mode automatically
> closes all open HANDLERs in the current connection.
>
> Incompatible change: previously an attempt to wait on a lock
> in a connection that has an open HANDLER statement could wait
> indefinitely/deadlock. After this patch, an error ER_LOCK_DEADLOCK
> is produced.
>
> The idea of the fix is to merge thd->handler_mdl_context
> with the main mdl_context of the connection, used for transactional
> locks. This makes deadlock detection possible, since all waits
> with locks are "visible" and available to analysis in a single
> MDL context of a connection.
>
> Since HANDLER locks and transactional locks have a different life
> cycle -- HANDLERs are explicitly open and closed, and so
> are HANDLER locks, explicitly acquired and released, whereas
> transactional locks "accumulate" till the end of a transaction
> and are released only with COMMIT, ROLLBACK and ROLLBACK TO SAVEPOINT,
> a concept of "sentinel" was introduced to MDL_context.
> All locks, HANDLER and others, reside in the same linked list.
> However, a selected element of the list separates locks with
> different life cycle. HANDLER locks always reside at the
> end of the list, after the sentinel. Transactional locks are
> prepended to the beginning of the list, before the sentinel.
> Thus, ROLLBACK, COMMIT or ROLLBACK TO SAVEPOINT, only
> release those locks that reside before the sentinel. HANDLER locks
> must be released explicitly as part of HANDLER CLOSE statement,
> or an implicit close.
>
> @ mysql-test/include/handler.inc
> Add test coverage for Bug#46224 "HANDLER statements within a
> transaction might lead to deadlocks"
I would also explicitly mentioned that you have added test coverage
for mix of HANDLER, transactions and DDL statements.
...
> @ sql/sql_class.cc
> Remove now unused methods. Move mysql_ha_cleanup() a few
^^^^^^^
Which methods? May be you have meant "unused some_tables_deleted member"?
> lines above in THD::cleanup() to make sure that
> all handlers are closed when it's time to destroy the MDL_context
> of this connection.
> Remove handler_mdl_context and handler_tables.
...
> === modified file 'mysql-test/include/handler.inc'
> --- a/mysql-test/include/handler.inc 2009-10-12 09:08:34 +0000
> +++ b/mysql-test/include/handler.inc 2009-11-18 18:47:43 +0000
> @@ -561,14 +561,22 @@ let $wait_condition=
> --source include/wait_condition.inc
> connection default;
> --echo connection: default
> +--echo # RENAME placed two pending locks.
> +--echo # An attempt to open t2 will fail due to one of them.
> +--echo # At the same time, open_tables has mysql_ha_flush which will
> +--echo # close the open HANDLER for t1. Thus RENAME will be able to
> +--echo # go through.
> +--error 0, ER_LOCK_DEADLOCK
> handler t2 open;
I think it is better to rephrase the above comment to make more clear
why this statement sometimes fails and sometimes succeeds.
Something like:
--echo # RENAME placed two pending locks. When HANDLER t2 OPEN does
--echo # open_tables() it calls mysql_ha_flush(), which will close the
--echo # open HANDLER for t1. This will unblock RENAME TABLE. If this
--echo # statement manages to complete before open_tables() will try to
--echo # acquire metadata lock on t1, open_tables() and the whole
--echo # HANDLER t2 OPEN will succeed. Otherwise open_tables() will
--echo # notice pending or active exclusive metadata lock on t2 and
--echo # the whole HANDLER t2 OPEN will fail with ER_LOCK_DEADLOCK
--echo # error.
IMO will do the trick.
...
> @@ -748,3 +756,408 @@ USE information_schema;
> --error ER_WRONG_USAGE
> HANDLER COLUMNS OPEN;
> USE test;
> +
> +--echo #
> +--echo # Add test coverage for HANDLER and LOCK TABLES, HANDLER and DDL.
> +--echo #
How about adding something like:
--disable_warnings
drop table if exists t1, t2, t3;
--enable_warnings
To make this part of test more independent from other parts of test.
> +create table t1 (a int, key a (a));
> +insert into t1 (a) values (1), (2), (3), (4), (5);
> +create table t2 (a int, key a (a)) select * from t1;
> +create temporary table t3 (a int, key a (a)) select * from t2;
> +handler t1 open;
> +handler t2 open;
> +--echo #
> +--echo # LOCK TABLES implicitly closes all handlers.
> +--echo #
> +lock table t3 read;
> +--echo #
> +--echo # No HANDLER sql is available under lock tables anyway.
> +--echo #
> +--error ER_LOCK_OR_ACTIVE_TRANSACTION
> +handler t1 read next;
> +--error ER_LOCK_OR_ACTIVE_TRANSACTION
> +handler t2 close;
> +--error ER_LOCK_OR_ACTIVE_TRANSACTION
> +handler t3 open;
> +--echo # After UNLOCK TABLES no handlers are around, they were
> +--echo # implicitly closed.
> +unlock tables;
> +drop temporary table t3;
> +--error ER_UNKNOWN_TABLE
> +handler t1 read next;
> +--error ER_UNKNOWN_TABLE
> +handler t2 close;
May be it makes sense to test all three kinds of HANDLER operations
for both temporary and non-temporary tables?
...
> +--echo #
> +--echo # Explore the effect of HANDLER locks on concurrent DDL
> +--echo #
> +handler t1 open;
> +connect(con1, localhost, root,,);
> +connect(con2, localhost, root,,);
> +connect(con3, localhost, root,,);
> +--echo # --> Connection con1;
> +connection con1;
> +--send drop table t1
> +--echo # We can't use connection 'default' as wait_condition will
> +--echo # autoclose handlers.
> +--echo # --> Connection con2
> +connection con2;
> +let $wait_condition=select count(*)=1 from information_schema.processlist where
> state='Waiting for table' and info='drop table t1';
> +--source include/wait_condition.inc
> +--echo # --> Connection default
> +connection default;
To increase .result file readability I, personally, also add --echo
statements for --send, --reap and --source include/wait_condition.inc
commands. E.g.:
--echo # Sending:
--send insert into t1 values ...
--echo # Reaping INSERT INTO t1 VALUES ...
--reap
--echo # Waiting for INSERT INTO t1 VALUES ... to be blocked.
let $wait_condition=select count(*)=1 from information_schema.processlist where
state='Waiting for table' and info like 'insert into t1 values%';
--source include/wait_condition.inc
Do you think it makes sense to do the same thing in this test?
...
> +--echo #
> +--echo # Demonstrate that HANDLER locks and transaction locks
> +--echo # reside in the same context, and we don't back-off
> +--echo # when have transaction or handler locks.
> +--echo #
> +create table t1 (a int, key a (a));
> +insert into t1 (a) values (1), (2), (3), (4), (5);
> +create table t2 (a int, key a (a));
> +insert into t2 (a) values (1), (2), (3), (4), (5);
> +begin;
> +select * from t1;
> +--echo # --> connection con1
> +connection con1;
> +lock table t2 read;
> +--echo # --> connection con2
> +connection con2;
> +--send drop table t2
> +--echo # --> connection con1
> +connection con1;
> +let $wait_condition=select count(*)=1 from information_schema.processlist where
> state='Waiting for table' and info='drop table t2';
> +--source include/wait_condition.inc
> +--echo # --> connection default
> +connection default;
> +--error ER_LOCK_DEADLOCK
> +handler t2 open;
> +--error ER_LOCK_DEADLOCK
> +select * from t2;
> +handler t1 open;
> +commit;
> +--error ER_LOCK_DEADLOCK
> +handler t2 open;
> +handler t1 close;
> +--echo # --> connection con1
> +connection con1;
> +unlock tables;
> +--echo # --> connection con2
> +connection con2;
> +--reap
> +--echo # --> connection default
> +connection default;
> +handler t1 open;
> +handler t1 read a prev;
> +handler t1 close;
May be it makes sense to add test for case when ER_LOCK_DEADLOCK is
emitted because we have open HANDLER and current statement encounters
conflicting metadata lock even though we are not inside transaction?
In other words something like:
handler t1 open;
--echo # --> connection con1
connection con1;
lock tables t2 read;
--echo # --> connection con2
connection con2;
--send drop table t2;
--echo # --> connection default
connection default;
--error ER_LOCK_DEADLOCK
select * from t2;
...
> +--echo #
> +--echo # ROLLBACK TO SAVEPOINT releases transactional locks,
> +--echo # but has no effect on open HANDLERs
> +--echo #
See comment about extending test coverage for HANDLERs and SAVEPOINTs
below.
...
> +--echo #
> +--echo # Bug #46224 HANDLER statements within a transaction might
> +--echo # lead to deadlocks
> +--echo #
> +create table t1 (a int, key a(a));
> +
> +
Please consider removing extra empty line to increase test file
readability.
> +--echo # --> Connection default
> +connection default;
> +begin;
> +select * from t1;
> +handler t1 open;
> +
> +--echo # --> Connection con1
> +connection con1;
> +lock tables t1 write;
> +
> +--echo # --> Connection default
> +connection default;
> +--send handler t1 read a next
> +
> +--echo # --> Connection con1
> +connection con1;
> +let $wait_condition=
> + select count(*) = 1 from information_schema.processlist
> + where state = "Table lock" and info = "handler t1 read a next";
> +--source include/wait_condition.inc
> +--send drop table t1
> +
> +--echo # --> Connection con2
> +connection con2;
> +let $wait_condition=
> + select count(*) = 1 from information_schema.processlist
> + where state = "Waiting for table" and info = "drop table t1";
> +--source include/wait_condition.inc
> +
> +--echo # --> Connection con1
^^^^
I guess the above line should look like:
--echo # --> Connection default
...
> === modified file 'mysql-test/r/create-big.result'
> --- a/mysql-test/r/create-big.result 2007-05-23 11:26:16 +0000
> +++ b/mysql-test/r/create-big.result 2009-11-18 18:47:43 +0000
> @@ -175,10 +175,10 @@ t2 CREATE TABLE `t2` (
> `i` int(11) DEFAULT NULL
> ) ENGINE=MyISAM DEFAULT CHARSET=latin1
> drop table t2;
> -show binlog events in 'master-bin.000001' from 106;
> +show binlog events from <binlog_start>;
> Log_name Pos Event_type Server_id End_log_pos Info
> -master-bin.000001 # Query 1 # use `test`; insert into t1 values (1)
> master-bin.000001 # Query 1 # use `test`; create table t2 like t1
> +master-bin.000001 # Query 1 # use `test`; insert into t1 values (1)
> master-bin.000001 # Query 1 # use `test`; drop table t1
> master-bin.000001 # Query 1 # use `test`; drop table t2
> create table t1 (i int);
Hmm... The above change in binary log order has highlighted the fact
that part of create-big.test which covers CREATE TABLE LIKE is outdated.
It relies on debug sleeps in server which are long gone from code
implementing this statement.
Since this problem is not related to your patch in any way I think it is
better to revert all your changes to create-big.test/result file and
report a bug about create-big.test failures (IMO such bug report should
mention that problem does not boil down to change of offset for
"show binlog events" statement).
...
> === modified file 'sql/mdl.cc'
> --- a/sql/mdl.cc 2009-10-25 13:41:27 +0000
> +++ b/sql/mdl.cc 2009-11-18 18:47:43 +0000
...
> @@ -709,6 +651,8 @@ MDL_context::try_acquire_shared_lock(MDL
> MDL_lock *lock;
> MDL_key *key= &mdl_request->key;
> MDL_ticket *ticket;
> + bool is_lt_or_ha;
> + bool has_ticket;
>
> DBUG_ASSERT(mdl_request->is_shared() && mdl_request->ticket ==
> NULL);
>
> @@ -724,10 +668,13 @@ MDL_context::try_acquire_shared_lock(MDL
> }
>
> /*
> - Check whether the context already holds a shared lock on the object,
> - and if so, grant the request.
> + Check whether the context already holds a shared lock on the
> + object, and if so, grant the request.
> + If the existing ticket is a HANDLER or LOCK TABLES ticket,
> + or if we're acquiring a HANDLER ticket, we will create a
> + separate ticket for it, see below.
> */
> - if ((ticket= find_ticket(mdl_request)))
> + if ((ticket= find_ticket(mdl_request, &is_lt_or_ha)) && !is_lt_or_ha)
> {
> DBUG_ASSERT(ticket->m_state == MDL_ACQUIRED);
> /* Only shared locks can be recursive. */
> @@ -735,6 +682,7 @@ MDL_context::try_acquire_shared_lock(MDL
> mdl_request->ticket= ticket;
> return FALSE;
> }
> + has_ticket= test(ticket);
>
> pthread_mutex_lock(&LOCK_mdl);
>
> if (!global_lock.is_lock_type_compatible(mdl_request->type, FALSE))
> {
> pthread_mutex_unlock(&LOCK_mdl);
> return FALSE;
> }
I wonder if it makes sense to add assert which will make clear
that for cases when 'has_ticket' is TRUE (i.e. we have HANDLER
ticket) there can't be any conflicts with global lock.
Something like:
DBUG_ASSERT(!has_ticket || mdl_request->type == MDL_SHARED);
> @@ -764,7 +712,12 @@ MDL_context::try_acquire_shared_lock(MDL
> }
> }
>
> - if (lock->can_grant_lock(this, mdl_request->type, FALSE))
> + /*
> + If we already have such ticket, just
> + create another MDL_ticket object and grant it.
> + Otherwise check if we can grant the lock.
> + */
> + if (has_ticket || lock->can_grant_lock(this, mdl_request->type, FALSE))
> {
> mdl_request->ticket= ticket;
> lock->granted.push_front(ticket);
I think it is worth mentioning in the above comment that existing ticket
corresponds to table open through HANDLER statement. This won't take much
space but will allow to understand without reading other comments why do
we have to create new MDL_ticket object instead of reusing existing one.
Actually, you can simplify code in this method and solve both this and the
previous issue by simply employing MDL_context::clone_ticket().
...
> @@ -1041,6 +1044,23 @@ MDL_ticket::upgrade_shared_lock_to_exclu
> if (m_lock->can_grant_lock(m_ctx, MDL_EXCLUSIVE, TRUE))
> break;
>
> + /*
> + If m_ctx->lt_or_ha_sentinel(), and this sentinel is for HANDLER,
> + we can deadlock. However, HANDLER is not allowed under
> + LOCK TABLES, and the only case of lock upgrade *not* under
> + LOCK TABLES, is ALTER TABLE. Which leaves us with the following
> + scenario for deadlock only:
> +
> + connection 1 connection 2
> + handler t1 open; handler t2 open;
> + alter table t2 ... alter table t1 ...
> +
> + But even that scenario is quite remote, since ALTER
> + will perform mysql_ha_flush() in the beginning, and thus
> + if there is a pending lock upgrade, close the HANDLER.
> + Since the possibility of ALTER interleaving in the above
> + manner is quite remove, we do nothing here to address it.
> + */
Another example of statement which does lock upgrade when not under
LOCK TABLES is CREATE/DROP TRIGGER statement. I think it makes sense
to adjust above comment accordingly. :)
> @@ -1280,6 +1300,9 @@ void MDL_context::release_ticket(MDL_tic
>
> safe_mutex_assert_owner(&LOCK_mdl);
>
> + if (ticket == m_lt_or_ha_sentinel)
> + m_lt_or_ha_sentinel= Ticket_list::Iterator(m_tickets, ticket).next();
> +
> m_tickets.remove(ticket);
>
> switch (ticket->m_type)
...
> @@ -1317,14 +1340,21 @@ void MDL_context::release_ticket(MDL_tic
>
>
> /**
> - Release all locks associated with the context.
> + Release all locks associated with the context. If the sentinel
> + is not NULL, do not release locks before and including the
> + sentinel.
>
> - This function is used to back off in case of a lock conflict.
> - It is also used to release shared locks in the end of an SQL
> - statement.
> + This function is used to:
> + - back off in case of a lock conflict.
> + - release all locks in the end of a transaction
> + - rollback to a savepoint
> +
> + The sentinel semantics is used to support LOCK TABLES
> + mode, and HANDLER statements: locks taken by these statement
> + survive COMMIT, ROLLBACK, ROLLBACK TO SAVEPOINT.
> */
>
> -void MDL_context::release_all_locks()
> +void MDL_context::release_all_locks_after(MDL_ticket *sentinel)
> {
I'm constantly getting confused when trying to interpret terms
"before/after" when you apply them to "sentinel". For some reason
for me they are more associated with ticket's position in
"MDL_context::m_tickets" list rather than with time when ticket
was acquired.
May be we can solve this problem by emphasizing that "sentinel" is
just special kind of savepoint or/and using less ambiguous (at least
for me) terms such as "older than/newer than"?
...
> +
> +
> +void MDL_context::commit_transaction()
> +{
> + DBUG_ENTER("MDL_context::commit_or_rollback_transaction");
> + release_all_locks_after(m_lt_or_ha_sentinel);
> + DBUG_VOID_RETURN;
> +}
Please add Doxygen comment describing this method.
Also IMO we should find a better name for this method which will
describe what this method does rather when it is called...
How about something like "release_trx_locks()"?
And don't forget to adjust DBUG_ENTER macro to the new name
like it has happened last time when you have renamed it :)
> +
> +
> +/**
> + Should be used only when we know for sure
> + we're not under LOCK TABLES.
> +*/
> +
> +void MDL_context::release_all_locks()
> +{
> + DBUG_ENTER("MDL_context::release_all_locks");
> + DBUG_ASSERT(m_lt_or_ha_sentinel == NULL);
> +
> + release_all_locks_after(m_lt_or_ha_sentinel);
> + m_lt_or_ha_sentinel= NULL;
> +
> + DBUG_VOID_RETURN;
> +}
> +
This method also can't be used if if there are open HANDLER tables.
I think this fact at least worth mentioning in comment.
Actually, taking this into account I wonder if it makes sense to get
rid of this method completely and use commit_transaction()/
release_trx_locks() everywhere instead (AFAICS this is quite doable).
Please consider doing this.
If you decide to leave it, please fix its incorrect usage in
unlock_table_names() - since you allow calling lock_table_names()
with open HANDLER tables and fail only if there is a lock conflict,
calling unlock_table_names() in such conditions should be also
possible.
> +
> +/**
> + Does this savepoint has this lock?
> +
> + @retval TRUE if the ticket belongs to a savepoint taken.
> + @retval FALSE The ticket is either LT/HA ticket,
> + or is taken after the savepoint.
> +*/
> +
> +bool MDL_context::has_lock(MDL_ticket *mdl_savepoint,
> + MDL_ticket *mdl_ticket)
> +{
> + MDL_ticket *ticket;
> + MDL_context::Ticket_iterator it(m_tickets);
> +
> + while ((ticket= it++) && ticket != m_lt_or_ha_sentinel)
> {
> - /* Stop when lock was acquired before this savepoint. */
> + /* First met the savepoint. Then ticket must be sometimes after it. */
> if (ticket == mdl_savepoint)
> - break;
> - release_lock(ticket);
> + return TRUE;
> + /* First met the ticket. */
> + if (ticket == mdl_ticket)
> + return FALSE;
> }
> -
> - DBUG_VOID_RETURN;
> + return FALSE;
> }
AFAIU this method can also return TRUE when ticket is a HANDLER ticket.
E.g.:
HANDLER t1 OPEN;
BEGIN;
SELECT * FROM t2;
SAVEPOINT s1;
--> At this point MDL_context::has_lock(s1, t1) will return TRUE.
IMO saying in such situation that "t1" belongs to "s1" is at least
ambiguous. I think it is better to say that for LT/HA tickets this
result of this method is undefined.
> +/**
> + Rearrange the ticket to reside in the part of the list that's
> + beyong m_lt_or_ha_sentinel. This effectively changes the ticket
^^^^^
beyond :)
> + life cycle, from automatic to manual: i.e. the ticket is no
> + longer released by MDL_context::commit_transaction() or
> + MDL_context::rollback_to_savepoint(), it must be released manually.
> +*/
> +
> +void MDL_context::move_lt_or_ha_ticket(MDL_ticket *mdl_ticket)
> +{
> + m_tickets.remove(mdl_ticket);
> + if (m_lt_or_ha_sentinel == NULL)
> + {
> + m_lt_or_ha_sentinel= mdl_ticket;
> + /* sic: linear from the number of transactional tickets acquired so-far! */
> + m_tickets.push_back(mdl_ticket);
> + }
> + else
> + m_tickets.insert_after(m_lt_or_ha_sentinel, mdl_ticket);
> +}
May be it is better to call this method "move_ticket_beyond_lt_or_ha_sentinel"
or "move_beyond_lt_ot_ha_sentinel"?
>
> === modified file 'sql/mdl.h'
> --- a/sql/mdl.h 2009-10-12 09:08:34 +0000
> +++ b/sql/mdl.h 2009-11-18 18:47:43 +0000
...
> @@ -357,19 +356,43 @@ public:
>
> inline MDL_ticket *mdl_savepoint()
> {
> - return m_tickets.head();
> + return m_tickets.front();
> + }
Hmm... AFAICS with introduction of m_lt_or_ha_sentinel mdl_savepoint() and
rollback_to_savepoint() methods won't work correctly for savepoints taken
right at the start of transaction.
For example, consider the following scenario:
HANDLER t1 OPEN;
BEGIN
SAVEPOINT sv1; <-- Stores pointer to HANDLER's MDL_ticket.
HANDLER t1 CLOSE;
SELECT ... FROM t3 ...; <-- for some reason gets MDL_ticket which
has the same address as MDL_ticket for
HANDLER which just have been closed.
ROLLBACK TO sv1; <-- Won't release metadata lock on t3,
which is wrong.
Moreover:
BEGIN
SAVEPOINT sv1; <-- Stores NULL as savepoint.
...
HANDLER t1 OPEN;
...
ROLLBACK TO sv1; <-- Rollback will release ALL tickets
including ticket used by open HANDLER!!!
IMO this problem can be addressed by adjusting mdl_savepoint() method to
return NULL if m_tickets.front() == m_lt_or_ha_sentinel and adjusting
rollback_to_savepoint() to not release tickets which come after sentinel
in m_tickets list.
Also test coverage for such cases should be added.
> +
> + void set_lt_or_ha_sentinel()
> + {
> + DBUG_ASSERT(m_lt_or_ha_sentinel == NULL);
> + m_lt_or_ha_sentinel= mdl_savepoint();
> + }
> + MDL_ticket *lt_or_ha_sentinel() const { return m_lt_or_ha_sentinel; }
> +
> + void clear_lt_or_ha_sentinel()
> + {
> + m_lt_or_ha_sentinel= NULL;
> }
> + void move_lt_or_ha_ticket(MDL_ticket *mdl_ticket);
> +
>
> + void commit_transaction();
> + void release_all_locks();
> void rollback_to_savepoint(MDL_ticket *mdl_savepoint);
>
> inline THD *get_thd() const { return m_thd; }
> private:
> Ticket_list m_tickets;
> bool m_has_global_shared_lock;
> + /**
> + When entering LOCK TABLES mode, remember the last taken
> + metadata lock. COMMIT/ROLLBACK must preserve these metadata
> + locks.
> + */
> + MDL_ticket *m_lt_or_ha_sentinel;
I think it is better to update this comment to mention that we also
use sentinel for HANDLER locks. Moreover I would have added reference
to very nice explanation in "MDL_context::move_lt_or_ha_ticket"
description.
...
> === modified file 'sql/sql_base.cc'
> --- a/sql/sql_base.cc 2009-11-09 10:27:46 +0000
> +++ b/sql/sql_base.cc 2009-11-18 18:47:43 +0000
...
> @@ -1503,16 +1502,21 @@ void close_thread_tables(THD *thd)
> if (thd->open_tables)
> close_open_tables(thd);
>
> - /*
> - Defer the release of metadata locks until the current transaction
> - is either committed or rolled back. This prevents other statements
> - from modifying the table for the entire duration of this transaction.
> - This provides commitment ordering for guaranteeing serializability
> - across multiple transactions.
> - */
> - if (!thd->in_multi_stmt_transaction() ||
> - (thd->state_flags & Open_tables_state::BACKUPS_AVAIL))
> + if (thd->state_flags & Open_tables_state::BACKUPS_AVAIL)
> + {
> thd->mdl_context.release_all_locks();
> + }
OK. Since mdl_context is backed up and reset with the rest of
Open_tables_state calling release_all_locks() here should be fine.
> + else if (! thd->in_multi_stmt_transaction())
> + {
> + /*
> + Defer the release of metadata locks until the current transaction
> + is either committed or rolled back. This prevents other statements
> + from modifying the table for the entire duration of this transaction.
> + This provides commitment ordering for guaranteeing serializability
> + across multiple transactions.
> + */
> + thd->mdl_context.commit_transaction();
> + }
>
> DBUG_VOID_RETURN;
> }
...
> @@ -3643,7 +3648,8 @@ end_with_lock_open:
>
> Open_table_context::Open_table_context(THD *thd)
> :m_action(OT_NO_ACTION),
> - m_can_deadlock(thd->in_multi_stmt_transaction() &&
> + m_can_deadlock((thd->in_multi_stmt_transaction() ||
> + thd->mdl_context.lt_or_ha_sentinel())&&
> thd->mdl_context.has_locks())
> {}
So now we can also encounter ER_LOCK_DEADLOCK error even outside of
transaction, e.g. if there are open HANDLERs and statement which is
currently being executed encounters some conflicting metadata lock
(or table being flushed).
Since upcoming patch with trivial deadlock detector for MDL subsystem
should significantly decrease number of unwarranted ER_LOCK_DEADLOCK
errors I think it should be OK.
...
> @@ -4142,7 +4148,7 @@ bool open_tables(THD *thd, TABLE_LIST **
> even if they don't create problems for current thread (i.e. to avoid
> having DDL blocked by HANDLERs opened for long time).
> */
> - if (thd->handler_tables)
> + if (thd->handler_tables_hash.records)
> mysql_ha_flush(thd);
>
> /*
Since, unlike "handler_tables", "THD::handler_tables_hash" is not
part Open_tables_state it is not backed up and reset during
THD::reset_n_backup_open_tables_state().
So when we will try to open tables using open_system_tables_for_read()
we might discover that there are some open HANDLER tables which need to
be flushed. Attempt to release metadata locks for such tables might
lead to corruption of MDL_context to which they belong since it won't
be an active MDL_context at this point.
I think this problem can be solved by not calling mysql_ha_flush()
if there is a backed up context i.e. if
(thd->state_flags & Open_tables_state::BACKUPS_AVAIL) is TRUE.
I wonder if we should do the same thing in other places where
we can call mysql_ha_flush() in similar conditions.
...
> @@ -4650,11 +4656,10 @@ retry:
> {
> /*
> Even though we have failed to open table we still need to
> - call release_all_locks() to release metadata locks which
> + call release_all_locks_after() to release metadata locks which
> might have been acquired successfully.
> */
Should not the above comment still mention "release_all_locks()?
> - if (! thd->locked_tables_mode)
> - thd->mdl_context.release_all_locks();
> + thd->mdl_context.release_all_locks();
> table_list->mdl_request.ticket= 0;
> if (ot_ctx.recover_from_failed_open_table_attempt(thd, table_list))
> break;
...
> @@ -8235,6 +8239,7 @@ bool mysql_notify_thread_having_shared_l
> if (!thd_table->needs_reopen())
> signalled|= mysql_lock_abort_for_thread(thd, thd_table);
> }
> + broadcast_refresh();
> pthread_mutex_unlock(&LOCK_open);
> return signalled;
> }
>
May be it makes sense to add comment explaining why this broadcast
is necessary? Something like:
"Wake up thread if it is waiting in tdc_wait_for_old_versions().
This can happen if this thread has opened table on which we try
to acquire an exclusive metadata lock through HANDLER statement."
...
> === modified file 'sql/sql_class.h'
> --- a/sql/sql_class.h 2009-11-10 07:47:59 +0000
> +++ b/sql/sql_class.h 2009-11-18 18:47:43 +0000
...
> @@ -964,7 +959,6 @@ public:
> lower level routines, which would otherwise miss that lock.
> */
> MYSQL_LOCK *extra_lock;
> -
> /*
> Enum enum_locked_tables_mode and locked_tables_mode member are
> used to indicate whether the so-called "locked tables mode" is on,
AFAIU this is some kind of left-over from change that you have
decided to revert. IMO it is better to remove it.
...
> === modified file 'sql/sql_handler.cc'
> --- a/sql/sql_handler.cc 2009-11-02 15:16:58 +0000
> +++ b/sql/sql_handler.cc 2009-11-18 18:47:43 +0000
...
Since you have removed Open_tables_state::handler_tables member
could you please update all comments in this file that mention it?
For example, big comment at the beginning of this file.
> @@ -195,7 +182,7 @@ bool mysql_ha_open(THD *thd, TABLE_LIST
> uint dblen, namelen, aliaslen, counter;
> bool error;
> TABLE *backup_open_tables;
> - MDL_context backup_mdl_context;
> + MDL_ticket *mdl_savepoint;
> DBUG_ENTER("mysql_ha_open");
> DBUG_PRINT("enter",("'%s'.'%s' as '%s' reopen: %d",
> tables->db, tables->table_name, tables->alias,
> @@ -265,6 +252,8 @@ bool mysql_ha_open(THD *thd, TABLE_LIST
> memcpy(hash_tables->table_name, tables->table_name, namelen);
> memcpy(hash_tables->alias, tables->alias, aliaslen);
> hash_tables->mdl_request.init(MDL_key::TABLE, db, name, MDL_SHARED);
> + /* for now HANDLER can be used only for real TABLES */
> + hash_tables->required_type= FRMTYPE_TABLE;
>
> /* add to hash */
> if (my_hash_insert(&thd->handler_tables_hash, (uchar*) hash_tables))
> @@ -292,7 +281,7 @@ bool mysql_ha_open(THD *thd, TABLE_LIST
> */
> backup_open_tables= thd->open_tables;
> thd->open_tables= NULL;
> - thd->mdl_context.backup_and_reset(&backup_mdl_context);
> + mdl_savepoint= thd->mdl_context.mdl_savepoint();
>
> /*
> open_tables() will set 'hash_tables->table' if successful.
> @@ -300,53 +289,44 @@ bool mysql_ha_open(THD *thd, TABLE_LIST
> */
> DBUG_ASSERT(! hash_tables->table);
>
> - /* for now HANDLER can be used only for real TABLES */
> - hash_tables->required_type= FRMTYPE_TABLE;
> /*
> We use open_tables() here, rather than, say,
> open_ltable() or open_table() because we would like to be able
> to open a temporary table.
> */
> error= open_tables(thd, &hash_tables, &counter, 0);
> - if (thd->open_tables)
> +
> + if (! error &&
> + hash_tables->table &&
> + ! (hash_tables->table->file->ha_table_flags() &
> HA_CAN_SQL_HANDLER))
> {
Hmm...
In what case can we have "error" == FALSE and "hash_tables->table" == NULL?
Or is "hash_tables->table" part of condition there just for safety?
> - if (thd->open_tables->next)
> - {
> - /*
> - We opened something that is more than a single table.
> - This happens with MERGE engine. Don't try to link
> - this mess into thd->handler_tables list, close it
> - and report an error. We must do it right away
> - because mysql_ha_close_table(), called down the road,
> - can close a single table only.
> - */
> - close_thread_tables(thd);
> - thd->mdl_context.release_all_locks();
> - my_error(ER_ILLEGAL_HA, MYF(0), hash_tables->alias);
> - error= TRUE;
> - }
> - else
> - {
> - /* Merge the opened table into handler_tables list. */
> - thd->open_tables->next= thd->handler_tables;
> - thd->handler_tables= thd->open_tables;
> - }
> + my_error(ER_ILLEGAL_HA, MYF(0), tables->alias);
> + error= TRUE;
> + }
> + if (!error &&
> + hash_tables->mdl_request.ticket &&
> + thd->mdl_context.has_lock(mdl_savepoint,
> + hash_tables->mdl_request.ticket))
> + {
> + /* The ticket returned is within a savepoint. Make a copy. */
> + error= thd->mdl_context.clone_ticket(&hash_tables->mdl_request);
> + hash_tables->table->mdl_ticket= hash_tables->mdl_request.ticket;
> }
> - thd->handler_mdl_context.merge(&thd->mdl_context);
> -
> - /* Restore the state. */
> - thd->open_tables= backup_open_tables;
> - thd->mdl_context.restore_from_backup(&backup_mdl_context);
> -
> if (error)
> - goto err;
> -
> - /* There can be only one table in '*tables'. */
> - if (! (hash_tables->table->file->ha_table_flags() &
> HA_CAN_SQL_HANDLER))
> {
> - my_error(ER_ILLEGAL_HA, MYF(0), tables->alias);
> - goto err;
> + close_thread_tables(thd);
> + thd->open_tables= backup_open_tables;
> + thd->mdl_context.rollback_to_savepoint(mdl_savepoint);
> + if (!reopen)
> + my_hash_delete(&thd->handler_tables_hash, (uchar*) hash_tables);
> + else
> + hash_tables->table= NULL;
> + DBUG_PRINT("exit",("ERROR"));
> + DBUG_RETURN(TRUE);
> }
> + thd->open_tables= backup_open_tables;
> + if (hash_tables->mdl_request.ticket)
> + thd->mdl_context.move_lt_or_ha_ticket(hash_tables->mdl_request.ticket);
>
May be it makes sense to add DBUG_ASSERT that will emphasize that we still
don't support HANDLER statement for tables which to be open require several
TABLE instances, so one won't be able simply enable HA_CAN_SQL_HANDLER flag
for MERGE engine? Something like:
DBUG_ASSERT(hash_tables->table->next == NULL);
> /*
> If it's a temp table, don't reset table->query_id as the table is
> @@ -358,14 +338,6 @@ bool mysql_ha_open(THD *thd, TABLE_LIST
> my_ok(thd);
> DBUG_PRINT("exit",("OK"));
> DBUG_RETURN(FALSE);
> -
> -err:
> - if (hash_tables->table)
> - mysql_ha_close_table(thd, hash_tables);
> - if (!reopen)
> - my_hash_delete(&thd->handler_tables_hash, (uchar*) hash_tables);
> - DBUG_PRINT("exit",("ERROR"));
> - DBUG_RETURN(TRUE);
> }
>
>
> @@ -497,46 +469,25 @@ retry:
> hash_tables->db, hash_tables->table_name,
> hash_tables->alias, table));
> }
> -
> -#if MYSQL_VERSION_ID < 40100
> - if (*tables->db && strcmp(table->table_cache_key, tables->db))
> - {
> - DBUG_PRINT("info",("wrong db"));
> - table= NULL;
> - }
> -#endif
> }
> else
> table= NULL;
>
> if (!table)
> {
> -#if MYSQL_VERSION_ID < 40100
> - char buff[MAX_DBKEY_LENGTH];
> - if (*tables->db)
> - strxnmov(buff, sizeof(buff)-1, tables->db, ".", tables->table_name,
> - NullS);
> - else
> - strncpy(buff, tables->alias, sizeof(buff));
> - my_error(ER_UNKNOWN_TABLE, MYF(0), buff, "HANDLER");
> -#else
> my_error(ER_UNKNOWN_TABLE, MYF(0), tables->alias, "HANDLER");
> -#endif
> goto err0;
> }
> - tables->table=table;
>
> /* save open_tables state */
> backup_open_tables= thd->open_tables;
> /*
> mysql_lock_tables() needs thd->open_tables to be set correctly to
> - be able to handle aborts properly. When the abort happens, it's
> - safe to not protect thd->handler_tables because it won't close any
> - tables.
> + be able to handle aborts properly.
> */
> - thd->open_tables= thd->handler_tables;
> + thd->open_tables= NULL;
AFAIU in order for mysql_notify_thread_having_shared_lock() to work
properly THD::open_tables should point ot hash_tables->table during
this call to mysql_lock_tables().
>
> - lock= mysql_lock_tables(thd, &tables->table, 1, 0, &need_reopen);
> + lock= mysql_lock_tables(thd, &hash_tables->table, 1, 0, &need_reopen);
>
> /* restore previous context */
> thd->open_tables= backup_open_tables;
...
> === modified file 'sql/sql_parse.cc'
> --- a/sql/sql_parse.cc 2009-11-12 12:22:31 +0000
> +++ b/sql/sql_parse.cc 2009-11-18 18:47:43 +0000
> @@ -3639,7 +3636,13 @@ end_with_restore_list:
> if (trans_commit_implicit(thd))
> goto error;
> /* release transactional metadata locks. */
> - thd->mdl_context.release_all_locks();
> + /*
> + As of 5.6, entering LOCK TABLES mode
> + implicitly closes all open HANDLERs. This is
> + to avoid deadlocks.
> + */
> + mysql_ha_cleanup(thd);
> + thd->mdl_context.commit_transaction();
> if (lex->protect_against_global_read_lock &&
> wait_if_global_read_lock(thd, 0, 1))
> goto error;
Could you please provide an example of such a deadlock?
I was not able to come up with one...
IMO it is better to state another reason for closing all HANDLERs
here - the fact that both LOCK TABLES mode and HANDLERs use sentinel
mechanism in MDL subsystem and thus could not be used at the same
time. I would have also mentioned that all HANDLER operations are
prohibited under LOCK TABLES anyway...
...
> === modified file 'sql/sql_plist.h'
> --- a/sql/sql_plist.h 2009-03-04 13:31:31 +0000
> +++ b/sql/sql_plist.h 2009-11-18 18:47:43 +0000
> @@ -71,6 +71,36 @@ public:
> first= a;
> *B::prev_ptr(a)= &first;
> }
> + inline void push_back(T *a)
> + {
> + insert_after(back(), a);
> + }
> + inline T *back()
> + {
> + T *t= front();
> + if (t)
> + {
> + while (*B::next_ptr(t))
> + t= *B::next_ptr(t);
> + }
> + return t;
> + }
> + inline void insert_after(T *pos, T *a)
> + {
> + if (pos == NULL)
> + push_front(a);
> + else
> + {
> + *B::next_ptr(a)= *B::next_ptr(pos);
> + *B::prev_ptr(a)= B::next_ptr(pos);
> + *B::next_ptr(pos)= a;
> + if (*B::next_ptr(a))
> + {
> + T *old_next= *B::next_ptr(a);
> + *B::prev_ptr(old_next)= B::next_ptr(a);
> + }
> + }
> + }
Please consider introducing a descendant of I_P_List class which
will support fast push_back() operation by having a member that
will contain B::next_ptr() of the last element in the list and
using this class for MDL_context::m_tickets list.
> inline void remove(T *a)
> {
> T *next= *B::next_ptr(a);
> @@ -78,8 +108,8 @@ public:
> *B::prev_ptr(next)= *B::prev_ptr(a);
> **B::prev_ptr(a)= next;
> }
> - inline T* head() { return first; }
> - inline const T *head() const { return first; }
> + inline T* front() { return first; }
> + inline const T *front() const { return first; }
Hmm... I guess the long term plan is to change List class to follow
STL naming as well.
BTW should not we simply have:
inline T* front() const;
method instead of two these methods?
> void swap(I_P_List<T,B> &rhs)
> {
> swap_variables(T *, first, rhs.first);
> @@ -106,6 +136,7 @@ class I_P_List_iterator
> T *current;
> public:
> I_P_List_iterator(I_P_List<T, B> &a) : list(&a), current(a.first)
> {}
> + I_P_List_iterator(I_P_List<T, B> &a, T* current_arg) : list(&a),
> current(current_arg) {}
> inline void init(I_P_List<T, B> &a)
> {
> list= &a;
> @@ -118,6 +149,10 @@ public:
> current= *B::next_ptr(current);
> return result;
> }
> + inline T* next()
> + {
> + return *B::next_ptr(current);
> + }
I think having both I_P_List_iterator::next() and postfix ++ operator can
be a bit confusing. May be it is better to implement prefix version of ++
instead or may be we can come up with some better name for this method?
> inline void rewind()
> {
> current= list->first;
>
> === modified file 'sql/sql_prepare.cc'
> --- a/sql/sql_prepare.cc 2009-11-09 10:27:46 +0000
> +++ b/sql/sql_prepare.cc 2009-11-18 18:47:43 +0000
> @@ -3194,8 +3194,7 @@ bool Prepared_statement::prepare(const c
> Marker used to release metadata locks acquired while the prepared
> statement is being checked.
> */
> - if (thd->in_multi_stmt_transaction())
> - mdl_savepoint= thd->mdl_context.mdl_savepoint();
> + mdl_savepoint= thd->mdl_context.mdl_savepoint();
>
> /*
> The only case where we should have items in the thd->free_list is
> @@ -3221,13 +3220,11 @@ bool Prepared_statement::prepare(const c
> lex_end(lex);
> cleanup_stmt();
> /*
> - If not inside a multi-statement transaction, the metadata locks have
> - already been released and the rollback_to_savepoint is a nop.
> - Otherwise, release acquired locks -- a NULL mdl_savepoint means that
> - all locks are going to be released or that the transaction didn't
> - own any locks.
> + If not inside a multi-statement transaction, the metadata
> + locks have already been released and our savepoint pointer
> + is dead beef.
> */
> - if (!thd->locked_tables_mode)
> + if (thd->in_multi_stmt_transaction())
> thd->mdl_context.rollback_to_savepoint(mdl_savepoint);
> thd->restore_backup_statement(this, &stmt_backup);
> thd->stmt_arena= old_stmt_arena;
>
I find the above comment a bit ambiguous. IMO it is better to say something
like:
"If not inside a multi-statement transaction, the metadata
locks have already been released and our savepoint points
to ticket which already has been released."
...
Otherwise, I am OK with your patch and think that it can be pushed after
addressing/discussing the above issues.
--
Dmitry Lenev, Software Developer
MySQL AB, www.mysql.com
Are you MySQL certified? http://www.mysql.com/certification
