* Dmitry Lenev <dlenev@stripped> [09/11/25 19:01]:
> > 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.
Done.
> > @ 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.
Done.
> > @ 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"?
Done, comment updated.
> > +--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:
<cut>
Done.
> > +--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
Done.
> > +--echo #
> > +--echo # LOCK TABLES implicitly closes all handlers.
> > +--echo #
<cut>
> > +lock table t3 read;
> > +--echo #
> > +--echo # No HANDLER sql is available under lock tables anyway.
> > +--echo #
<cut>
> May be it makes sense to test all three kinds of HANDLER operations
> for both temporary and non-temporary tables?
Done.
> > +--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 ...
Done.
> > +--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;
Done.
> > +--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.
OK.
> > +--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.
Done.
>
> > +--echo # --> Connection con1
> ^^^^
>
> I guess the above line should look like:
>
> --echo # --> Connection default
Done.
> > === 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` (
<cut>
> 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).
I updated an existing bug, #37248 "Some 'Big' tests failing on
6.0".
> > @@ -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);
Instead of adding an assert, I eliminated "has_ticket"
case from this execution branch completely, as you suggest
in the following comment. Indeed, the new code is more clear.
> > @@ -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().
Done.
>
> > + /*
> > + 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. :)
Done. I rephrased the comment to not talk about ALTER TABLE
exclusively.
> > @@ -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"?
I renamed the method to release_locks_stored_before(), to
underline that it works on locks in list order, not temporal order.
> > +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 :)
Done.
> > +/**
> > + 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.
OK, I removed release_all_locks(), and instead added asserts
that in some places we can't have lt_or_ha_sentinel.
I also covered unlock_table_names() + HANDLER with a test.
> > +/**
> > + 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.
I fixed the code to follow the comment.
> > +/**
> > + 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 :)
Fixed.
> > + 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"?
Renamed.
> > === 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.
Done.
>
> > +
> > + 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.
Done.
> OK. Since mdl_context is backed up and reset with the rest of
> Open_tables_state calling release_all_locks() here should be fine.
<cut>
> > 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.
<cut>
> > @@ -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.
Done. Since we call mysql_ha_flush() from mdl, I decided to
add the check right inside mysql_ha_flush(), to keep mdl
independent of sql_class.h.
>
> ...
>
> > @@ -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()?
The comment was updated.
>
> > @@ -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."
Done.
> > === 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.
>
> ...
Done. Spurious change removed.
>
> > === 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.
Done.
>
> > @@ -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?
Done. I removed the check. Yes, it was just for safety, but I don't
do it consistently anyway.
>
> > - 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);
Done.
<cut>
> > @@ -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().
Done.
<cut>
> > === 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...
Done, the comment is updated.
> > === 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.
I think we should only do it when it becomes a performance issue
- and it will be the case only in the scenario when a HANDLER
table is opened inside a transaction that involves hundreds of
tables.
> > 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?
I'm following the STL convention, in which const method returns a
const pointer, a non-const method returns a non-const pointer.
> > 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?
Done, I changed the code to use a prefix version of operator ++.
> > 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
<cut>
> > @@ -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."
Done.
Thank you very much for your review. An updated patch will be
posted shortly.
