diff --git a/mysql-test/include/mix1.inc b/mysql-test/include/mix1.inc index e863e59fad6..d05b1a114a9 100644 --- a/mysql-test/include/mix1.inc +++ b/mysql-test/include/mix1.inc @@ -814,6 +814,39 @@ create table t1 (a int) engine=innodb; alter table t1 alter a set default 1; drop table t1; +--echo +--echo Bug#24918 drop table and lock / inconsistent between +--echo perm and temp tables +--echo +--echo Check transactional tables under LOCK TABLES +--echo +--disable_warnings +drop table if exists t24918, t24918_tmp, t24918_trans, t24918_trans_tmp, +t24918_access; +--enable_warnings +create table t24918_access (id int); +create table t24918 (id int) engine=myisam; +create temporary table t24918_tmp (id int) engine=myisam; +create table t24918_trans (id int) engine=innodb; +create temporary table t24918_trans_tmp (id int) engine=innodb; + +lock table t24918 write, t24918_tmp write, t24918_trans write, t24918_trans_tmp write; +drop table t24918; +--error ER_TABLE_NOT_LOCKED +select * from t24918_access; +drop table t24918_trans; +--error ER_TABLE_NOT_LOCKED +select * from t24918_access; +drop table t24918_trans_tmp; +--error ER_TABLE_NOT_LOCKED +select * from t24918_access; +drop table t24918_tmp; +--error ER_TABLE_NOT_LOCKED +select * from t24918_access; +unlock tables; + +drop table t24918_access; + --echo End of 5.0 tests diff --git a/mysql-test/r/innodb_mysql.result b/mysql-test/r/innodb_mysql.result index 3dc134352db..13457cc4c09 100644 --- a/mysql-test/r/innodb_mysql.result +++ b/mysql-test/r/innodb_mysql.result @@ -814,6 +814,34 @@ drop table if exists t1; create table t1 (a int) engine=innodb; alter table t1 alter a set default 1; drop table t1; + +Bug#24918 drop table and lock / inconsistent between +perm and temp tables + +Check transactional tables under LOCK TABLES + +drop table if exists t24918, t24918_tmp, t24918_trans, t24918_trans_tmp, +t24918_access; +create table t24918_access (id int); +create table t24918 (id int) engine=myisam; +create temporary table t24918_tmp (id int) engine=myisam; +create table t24918_trans (id int) engine=innodb; +create temporary table t24918_trans_tmp (id int) engine=innodb; +lock table t24918 write, t24918_tmp write, t24918_trans write, t24918_trans_tmp write; +drop table t24918; +select * from t24918_access; +ERROR HY000: Table 't24918_access' was not locked with LOCK TABLES +drop table t24918_trans; +select * from t24918_access; +ERROR HY000: Table 't24918_access' was not locked with LOCK TABLES +drop table t24918_trans_tmp; +select * from t24918_access; +ERROR HY000: Table 't24918_access' was not locked with LOCK TABLES +drop table t24918_tmp; +select * from t24918_access; +ERROR HY000: Table 't24918_access' was not locked with LOCK TABLES +unlock tables; +drop table t24918_access; End of 5.0 tests CREATE TABLE `t2` ( `k` int(11) NOT NULL auto_increment, diff --git a/sql/handler.h b/sql/handler.h index 1992af5b1ad..7cf7bfe043d 100644 --- a/sql/handler.h +++ b/sql/handler.h @@ -992,6 +992,7 @@ public: uint ref_length; FT_INFO *ft_handler; enum {NONE=0, INDEX, RND} inited; + bool locked; bool implicit_emptied; /* Can be !=0 only if HEAP */ const COND *pushed_cond; /* @@ -1022,11 +1023,13 @@ public: estimation_rows_to_insert(0), ht(ht_arg), ref(0), key_used_on_scan(MAX_KEY), active_index(MAX_KEY), ref_length(sizeof(my_off_t)), - ft_handler(0), inited(NONE), implicit_emptied(0), + ft_handler(0), inited(NONE), + locked(FALSE), implicit_emptied(0), pushed_cond(0), next_insert_id(0), insert_id_for_cur_row(0) {} virtual ~handler(void) { + DBUG_ASSERT(locked == FALSE); /* TODO: DBUG_ASSERT(inited == NONE); */ } virtual handler *clone(MEM_ROOT *mem_root); @@ -1591,8 +1594,10 @@ public: /* lock_count() can be more than one if the table is a MERGE */ virtual uint lock_count(void) const { return 1; } - /* - NOTE that one can NOT rely on table->in_use in store_lock(). It may + /** + Is not invoked for non-transactional temporary tables. + + @note that one can NOT rely on table->in_use in store_lock(). It may refer to a different thread if called from mysql_lock_abort_for_thread(). */ virtual THR_LOCK_DATA **store_lock(THD *thd, @@ -1723,6 +1728,29 @@ private: overridden by the storage engine class. To call these methods, use the corresponding 'ha_*' method above. */ + + /** + Is not invoked for non-transactional temporary tables. + + Tells the storage engine that we intend to read or write data + from the table. This call is prefixed with a call to handler::store_lock() + and is invoked only for those handler instances that stored the lock. + + Calls to rnd_init/index_init are prefixed with this call. When table + IO is complete, we call external_lock(F_UNLCK). + A storage engine writer should expect that each call to + ::external_lock(F_[RD|WR]LOCK is followed by a call to + ::external_lock(F_UNLCK). If it is not, it is a bug in MySQL. + + The name and signature originate from the first implementation + in MyISAM, which would call fcntl to set/clear an advisory + lock on the data file in this method. + + @param lock_type F_RDLCK, F_WRLCK, F_UNLCK + + @return non-0 in case of failure, 0 in case of success. + When lock_type is F_UNLCK, the return value is ignored. + */ virtual int external_lock(THD *thd __attribute__((unused)), int lock_type __attribute__((unused))) { diff --git a/sql/lock.cc b/sql/lock.cc index acb34c0d66a..c401cbda05c 100644 --- a/sql/lock.cc +++ b/sql/lock.cc @@ -244,7 +244,8 @@ MYSQL_LOCK *mysql_lock_tables(THD *thd, TABLE **tables, uint count, thd->proc_info="System lock"; DBUG_PRINT("info", ("thd->proc_info %s", thd->proc_info)); - if (lock_external(thd, tables, count)) + if (sql_lock->table_count && lock_external(thd, sql_lock->table, + sql_lock->table_count)) { /* Clear the lock type of all lock data to avoid reusage. */ reset_lock_data(sql_lock); @@ -340,6 +341,7 @@ static int lock_external(THD *thd, TABLE **tables, uint count) ((*tables)->reginfo.lock_type >= TL_READ && (*tables)->reginfo.lock_type <= TL_READ_NO_INSERT)) lock_type=F_RDLCK; + if ((error=(*tables)->file->ha_external_lock(thd,lock_type))) { print_lock_error(error, (*tables)->file->table_type()); @@ -447,10 +449,28 @@ void mysql_unlock_read_tables(THD *thd, MYSQL_LOCK *sql_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. -void mysql_lock_remove(THD *thd, MYSQL_LOCK *locked,TABLE *table) + @note This function has a legacy side effect: the table is + unlocked even if it is not found in the locked list. + 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, + 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; @@ -464,6 +484,10 @@ void mysql_lock_remove(THD *thd, MYSQL_LOCK *locked,TABLE *table) DBUG_ASSERT(table->lock_position == i); + /* Unlock if not yet unlocked */ + if (always_unlock == FALSE) + mysql_unlock_some_tables(thd, &table, /* table count */ 1); + /* Decrement table_count in advance, making below expressions easier */ old_tables= --locked->table_count; diff --git a/sql/mysql_priv.h b/sql/mysql_priv.h index 1b36d97ad3f..2fe70980ea8 100644 --- a/sql/mysql_priv.h +++ b/sql/mysql_priv.h @@ -1907,7 +1907,8 @@ MYSQL_LOCK *mysql_lock_tables(THD *thd, TABLE **table, uint count, void mysql_unlock_tables(THD *thd, MYSQL_LOCK *sql_lock); void mysql_unlock_read_tables(THD *thd, MYSQL_LOCK *sql_lock); void mysql_unlock_some_tables(THD *thd, TABLE **table,uint count); -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); void mysql_lock_abort(THD *thd, TABLE *table, bool upgrade_lock); void mysql_lock_downgrade_write(THD *thd, TABLE *table, thr_lock_type new_lock_type); diff --git a/sql/sql_base.cc b/sql/sql_base.cc index b471ff8a108..f85f3a2884a 100644 --- a/sql/sql_base.cc +++ b/sql/sql_base.cc @@ -1677,17 +1677,42 @@ TABLE *find_temporary_table(THD *thd, TABLE_LIST *table_list) } -/* - Close temporary table and unlink from thd->temporary tables +/** + Drop a temporary table. + + Try to locate the table in the list of thd->temporary_tables. + If the table is found: + - if the table is in thd->locked_tables, unlock it and + remove it from the list of locked tables. Currently only transactional + temporary tables are present in the locked_tables list. + - Close the temporary table, remove its .FRM + - remove the table from the list of temporary tables + + This function is used to drop user temporary tables, as well as + internal tables created in CREATE TEMPORARY TABLE ... SELECT + or ALTER TABLE. Even though part of the work done by this function + is redundant when the table is internal, as long as we + link both internal and user temporary tables into the same + thd->temporary_tables list, it's impossible to tell here whether + we're dealing with an internal or a user temporary table. + + @retval TRUE the table was not found in the list of temporary tables + of this thread + @retval FALSE the table was found and dropped successfully. */ -bool close_temporary_table(THD *thd, TABLE_LIST *table_list) +bool close_temporary_table(THD *thd, const char *db, const char *table_name) { TABLE *table; if (!(table= find_temporary_table(thd, table_list))) return 1; close_temporary_table(thd, table, 1, 1); + /* + If LOCK TABLES list is not empty and contains this table, + unlock the table and remove the table from this list. + */ + mysql_lock_remove(thd, thd->locked_tables, table, FALSE); return 0; } @@ -1833,7 +1858,7 @@ void unlink_open_table(THD *thd, TABLE *find, bool unlock) !memcmp(list->s->table_cache_key.str, key, key_length)) { if (unlock && thd->locked_tables) - mysql_lock_remove(thd, thd->locked_tables,list); + mysql_lock_remove(thd, thd->locked_tables, list, TRUE); VOID(hash_delete(&open_cache,(uchar*) list)); // Close table } else @@ -1859,8 +1884,13 @@ void unlink_open_table(THD *thd, TABLE *find, bool unlock) @note This routine assumes that table to be closed is open only by calling thread so we needn't wait until other threads - will close the table. It also assumes that table to be - dropped is already unlocked. + will close the table. Also unless called under implicit or + explicit LOCK TABLES mode it assumes that table to be + dropped is already unlocked. In the former case it will + also remove lock on the table. But one should not rely on + this behaviour as it may change in future. + Currently, however, this function is never called for a + table that was locked with LOCK TABLES. */ void drop_open_table(THD *thd, TABLE *table, const char *db_name, @@ -2837,7 +2867,7 @@ void close_data_files_and_morph_locks(THD *thd, const char *db, !strcmp(table->s->db.str, db)) { if (thd->locked_tables) - mysql_lock_remove(thd, thd->locked_tables, table); + mysql_lock_remove(thd, thd->locked_tables, table, TRUE); table->open_placeholder= 1; close_handle_and_leave_table_as_lock(table); } @@ -2975,7 +3005,7 @@ void close_old_data_files(THD *thd, TABLE *table, bool morph_locks, instances of this table. */ mysql_lock_abort(thd, table, TRUE); - mysql_lock_remove(thd, thd->locked_tables, table); + mysql_lock_remove(thd, thd->locked_tables, table, TRUE); /* We want to protect the table from concurrent DDL operations (like RENAME TABLE) until we will re-open and re-lock it. @@ -3120,7 +3150,7 @@ TABLE *drop_locked_tables(THD *thd,const char *db, const char *table_name) if (!strcmp(table->s->table_name.str, table_name) && !strcmp(table->s->db.str, db)) { - mysql_lock_remove(thd, thd->locked_tables,table); + mysql_lock_remove(thd, thd->locked_tables, table, TRUE); if (!found) { found= table; diff --git a/sql/sql_class.h b/sql/sql_class.h index d8005c40522..ff4f3022c2b 100644 --- a/sql/sql_class.h +++ b/sql/sql_class.h @@ -765,13 +765,25 @@ enum prelocked_mode_type {NON_PRELOCKED= 0, PRELOCKED= 1, 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 + /** + List of regular tables in use by this thread. Contains temporary and + base tables that were opened with @see open_tables(). */ - TABLE *open_tables, *temporary_tables, *handler_tables, *derived_tables; + TABLE *open_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; + /** + List of tables that were opened with HANDLER OPEN and are + still in use by this thread. + */ + TABLE *handler_tables; + TABLE *derived_tables; /* During a MySQL session, one can lock tables in two modes: automatic or manual. In automatic mode all necessary tables are locked just before