Hi!
On Jul 24, konstantin@stripped wrote:
> ChangeSet@stripped, 2007-07-24 22:52:18+04:00, kostja@bodhi.(none) +9 -0
> A fix and a test case for Bug#24918 drop table and lock / inconsistent
> between perm and temp tables
A couple of questions and style comments below.
otherwise looks ok.
> This patch introduces no changes in behavior -- the discrepancy in
> behavior will be fixed when we start calling
> ::store_lock()/::external_lock() for all tables, regardless whether
> they are transactional or not, temporary or not.
So, what will happen with this bug when you push ? cannot be closed,
right ? you didn't fix the sympthoms in the original bugreport.
> diff -Nrup a/sql/lock.cc b/sql/lock.cc
> --- a/sql/lock.cc 2007-06-01 12:54:30 +04:00
> +++ b/sql/lock.cc 2007-07-24 22:52:15 +04:00
> @@ -151,7 +151,8 @@ MYSQL_LOCK *mysql_lock_tables(THD *thd,
> }
>
> thd->proc_info="System lock";
> - if (lock_external(thd, tables, count))
> + if (sql_lock->table_count && lock_external(thd, sql_lock->table,
> + sql_lock->table_count))
Why ? Am I correct that this is just an optimization, not critical for
correctness of the code ?
> {
> /* Clear the lock type of all lock data to avoid reusage. */
> reset_lock_data(sql_lock);
> @@ -353,10 +354,28 @@ void mysql_lock_remove(THD *thd, MYSQL_LOCK
> }
>
>
> +/**
> + Try to find the table in the list of locked tables.
> + In case of success, unlock the table and remove it from this list.
> +
> + @note This function has a legacy side effect: the table is
> + unlocked even if it is not found in the locked list.
Could that happen ?
> + It's not clear if this side effect is intentional or still
> + desirable. It might lead to unmatched calls to
> + unlock_external(). Moreover, a discrepancy can be left
> + unnoticed by the storage engine, because in
> + unlock_external() we call handler::external_lock(F_UNLCK) only
> + if table->current_lock is not F_UNLCK.
> +
> + @param always_unlock specify explicitly if the legacy side
> + effect is desired.
> +*/
> +
> -void mysql_lock_remove(THD *thd, MYSQL_LOCK *locked,TABLE *table)
> +void mysql_lock_remove(THD *thd, MYSQL_LOCK *locked,TABLE *table,
> + bool always_unlock)
> {
> - mysql_unlock_some_tables(thd, &table,1);
> + if (always_unlock == TRUE)
> + mysql_unlock_some_tables(thd, &table, /* table count */ 1);
> if (locked)
> {
> reg1 uint i;
> diff -Nrup a/sql/sql_base.cc b/sql/sql_base.cc
> --- a/sql/sql_base.cc 2007-07-20 19:46:11 +04:00
> +++ b/sql/sql_base.cc 2007-07-24 22:52:16 +04:00
> @@ -2343,7 +2387,10 @@ bool drop_locked_tables(THD *thd,const c
> if (!strcmp(table->s->table_name, table_name) &&
> !strcmp(table->s->db, db))
> {
> - mysql_lock_remove(thd, thd->locked_tables,table);
> + mysql_lock_remove(thd, thd->locked_tables, table,
> + /* Unlock the table even if it is not
> + found in the list of locked tables. */
> + TRUE);
If you absolutely must have this last argument documented, instead of
adding a comment to the every call, give it a better name, for example:
#define UNLOCK_EVEN_IF_NOT_IN_THE_LOCKED_LIST TRUE
> VOID(hash_delete(&open_cache,(byte*) table));
> found=1;
> }
> diff -Nrup a/sql/sql_class.h b/sql/sql_class.h
> --- a/sql/sql_class.h 2007-07-21 17:52:13 +04:00
> +++ b/sql/sql_class.h 2007-07-24 22:52:16 +04:00
> @@ -997,11 +997,18 @@ class Open_tables_state
> public:
> /*
> open_tables - list of regular tables in use by this thread
> - temporary_tables - list of temp tables in use by this thread
> handler_tables - list of tables that were opened with HANDLER OPEN
> and are still in use by this thread
> */
> - TABLE *open_tables, *temporary_tables, *handler_tables, *derived_tables;
> + TABLE *open_tables, *handler_tables, *derived_tables;
> + /**
> + List of temporary tables used by this thread. Contains user-level
> + temporary tables, created with CREATE TEMPORARY TABLE, and
> + internal temporary tables, created, e.g., to resolve a SELECT,
> + or for an intermediate table used in ALTER.
> + XXX Why are internal temporary tables added to this list?
> + */
> + TABLE *temporary_tables;
Generally it's best to avoid moving members in the class when adding
comments. Otherwise constructors that initialize them will start to
trigger compiler warnings.
> /*
> During a MySQL session, one can lock tables in two modes: automatic
Regards / Mit vielen Grüssen,
Sergei
--
__ ___ ___ ____ __
/ |/ /_ __/ __/ __ \/ / Sergei Golubchik <serg@stripped>
/ /|_/ / // /\ \/ /_/ / /__ Principal Software Developer
/_/ /_/\_, /___/\___\_\___/ MySQL GmbH, Radlkoferstr. 2, D-81373 München
<___/ Geschäftsführer: Kaj Arnö - HRB
München 162140
