diff --git a/mysql-test/r/innodb_mysql.result b/mysql-test/r/innodb_mysql.result index f32878309b8..6fb98f509ef 100644 --- a/mysql-test/r/innodb_mysql.result +++ b/mysql-test/r/innodb_mysql.result @@ -739,4 +739,32 @@ 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 diff --git a/mysql-test/t/innodb_mysql.test b/mysql-test/t/innodb_mysql.test index 0d43d13ec3a..60d46863bfd 100644 --- a/mysql-test/t/innodb_mysql.test +++ b/mysql-test/t/innodb_mysql.test @@ -754,4 +754,38 @@ 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/sql/handler.h b/sql/handler.h index a3767573178..cd9f9a91008 100644 --- a/sql/handler.h +++ b/sql/handler.h @@ -508,6 +508,29 @@ class handler :public Sql_alloc */ virtual int rnd_init(bool scan) =0; virtual int rnd_end() { return 0; } + /** + 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, int lock_type) { return 0; } public: const handlerton *ht; /* storage engine of this handler */ @@ -548,6 +571,7 @@ public: uint raid_type,raid_chunks; FT_INFO *ft_handler; enum {NONE=0, INDEX, RND} inited; + bool locked; bool auto_increment_column_changed; bool implicit_emptied; /* Can be !=0 only if HEAP */ const COND *pushed_cond; @@ -560,10 +584,11 @@ public: create_time(0), check_time(0), update_time(0), key_used_on_scan(MAX_KEY), active_index(MAX_KEY), ref_length(sizeof(my_off_t)), block_size(0), - raid_type(0), ft_handler(0), inited(NONE), implicit_emptied(0), + raid_type(0), ft_handler(0), inited(NONE), + locked(FALSE), implicit_emptied(0), pushed_cond(NULL) {} - virtual ~handler(void) { /* TODO: DBUG_ASSERT(inited == NONE); */ } + virtual ~handler(void) { DBUG_ASSERT(locked == FALSE); /* TODO: DBUG_ASSERT(inited == NONE); */ } virtual handler *clone(MEM_ROOT *mem_root); int ha_open(const char *name, int mode, int test_if_locked); void adjust_next_insert_id_after_explicit_value(ulonglong nr); @@ -597,6 +622,13 @@ public: virtual const char *index_type(uint key_number) { DBUG_ASSERT(0); return "";} + int ha_external_lock(THD *thd, int lock_type) + { + int rc; + DBUG_ENTER("ha_external_lock"); + locked= lock_type != F_UNLCK; + DBUG_RETURN(external_lock(thd, lock_type)); + } int ha_index_init(uint idx) { DBUG_ENTER("ha_index_init"); @@ -689,7 +721,6 @@ public: virtual int extra_opt(enum ha_extra_function operation, ulong cache_size) { return extra(operation); } virtual int reset() { return extra(HA_EXTRA_RESET); } - virtual int external_lock(THD *thd, int lock_type) { return 0; } virtual void unlock_row() {} virtual int start_stmt(THD *thd, thr_lock_type lock_type) {return 0;} /* @@ -837,6 +868,9 @@ public: /* lock_count() can be more than one if the table is a MERGE */ virtual uint lock_count(void) const { return 1; } + /** + Is not invoked for non-transactional temporary tables. + */ virtual THR_LOCK_DATA **store_lock(THD *thd, THR_LOCK_DATA **to, enum thr_lock_type lock_type)=0; diff --git a/sql/lock.cc b/sql/lock.cc index 93358e56701..f730ac56d35 100644 --- a/sql/lock.cc +++ b/sql/lock.cc @@ -151,7 +151,8 @@ MYSQL_LOCK *mysql_lock_tables(THD *thd, TABLE **tables, uint count, } 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)) { /* Clear the lock type of all lock data to avoid reusage. */ reset_lock_data(sql_lock); @@ -246,12 +247,12 @@ static int lock_external(THD *thd, TABLE **tables, uint count) (*tables)->reginfo.lock_type <= TL_READ_NO_INSERT)) lock_type=F_RDLCK; - if ((error=(*tables)->file->external_lock(thd,lock_type))) + if ((error= (*tables)->file->ha_external_lock(thd,lock_type))) { print_lock_error(error, (*tables)->file->table_type()); for (; i-- ; tables--) { - (*tables)->file->external_lock(thd, F_UNLCK); + (*tables)->file->ha_external_lock(thd, F_UNLCK); (*tables)->current_lock=F_UNLCK; } DBUG_RETURN(error); @@ -353,10 +354,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; @@ -370,6 +389,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; @@ -623,7 +646,7 @@ static int unlock_external(THD *thd, TABLE **table,uint count) if ((*table)->current_lock != F_UNLCK) { (*table)->current_lock = F_UNLCK; - if ((error=(*table)->file->external_lock(thd, F_UNLCK))) + if ((error= (*table)->file->ha_external_lock(thd, F_UNLCK))) { error_code=error; print_lock_error(error_code, (*table)->file->table_type()); diff --git a/sql/mysql_priv.h b/sql/mysql_priv.h index d14aab57489..ca6aa8ecab0 100644 --- a/sql/mysql_priv.h +++ b/sql/mysql_priv.h @@ -1452,7 +1452,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 mysql_lock_abort_for_thread(THD *thd, TABLE *table); MYSQL_LOCK *mysql_lock_merge(MYSQL_LOCK *a,MYSQL_LOCK *b); diff --git a/sql/opt_range.cc b/sql/opt_range.cc index 247f0eada49..d978c8882ac 100644 --- a/sql/opt_range.cc +++ b/sql/opt_range.cc @@ -972,7 +972,7 @@ QUICK_RANGE_SELECT::~QUICK_RANGE_SELECT() DBUG_PRINT("info", ("Freeing separate handler 0x%lx (free: %d)", (long) file, free_file)); file->reset(); - file->external_lock(current_thd, F_UNLCK); + file->ha_external_lock(current_thd, F_UNLCK); file->close(); } } @@ -1142,7 +1142,7 @@ int QUICK_RANGE_SELECT::init_ror_merged_scan(bool reuse_handler) /* Caller will free the memory */ goto failure; /* purecov: inspected */ } - if (file->external_lock(thd, F_RDLCK)) + if (file->ha_external_lock(thd, F_RDLCK)) goto failure; if (!head->no_keyread) { @@ -1152,7 +1152,7 @@ int QUICK_RANGE_SELECT::init_ror_merged_scan(bool reuse_handler) if (file->extra(HA_EXTRA_RETRIEVE_PRIMARY_KEY) || init() || reset()) { - file->external_lock(thd, F_UNLCK); + file->ha_external_lock(thd, F_UNLCK); file->close(); goto failure; } diff --git a/sql/sql_base.cc b/sql/sql_base.cc index 3860dfa1ded..61847a6b168 100644 --- a/sql/sql_base.cc +++ b/sql/sql_base.cc @@ -1037,6 +1037,31 @@ TABLE **find_temporary_table(THD *thd, const char *db, const char *table_name) return 0; // Not a temporary table } + +/** + 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, const char *db, const char *table_name) { TABLE *table,**prev; @@ -1045,6 +1070,11 @@ bool close_temporary_table(THD *thd, const char *db, const char *table_name) return 1; table= *prev; *prev= table->next; + /* + 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); close_temporary(table, 1); if (thd->slave_thread) --slave_open_temp_tables; @@ -1120,7 +1150,7 @@ TABLE *unlink_open_table(THD *thd, TABLE *list, TABLE *find) !memcmp(list->s->table_cache_key, key, key_length)) { if (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,(byte*) list)); // Close table } else @@ -1151,6 +1181,8 @@ TABLE *unlink_open_table(THD *thd, TABLE *list, TABLE *find) 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, @@ -2099,7 +2131,7 @@ bool close_data_tables(THD *thd,const char *db, const char *table_name) 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, TRUE); table->file->close(); table->db_stat=0; } @@ -2239,7 +2271,7 @@ void close_old_data_files(THD *thd, TABLE *table, bool morph_locks, instances of this table. */ mysql_lock_abort(thd, table); - 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. @@ -2343,7 +2375,7 @@ bool drop_locked_tables(THD *thd,const char *db, const char *table_name) 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, TRUE); VOID(hash_delete(&open_cache,(byte*) table)); found=1; } diff --git a/sql/sql_class.h b/sql/sql_class.h index 058f130d4e7..6ffbd9b4ac7 100644 --- a/sql/sql_class.h +++ b/sql/sql_class.h @@ -995,13 +995,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 diff --git a/sql/sql_table.cc b/sql/sql_table.cc index e02595836ca..f11082a712c 100644 --- a/sql/sql_table.cc +++ b/sql/sql_table.cc @@ -3791,10 +3791,10 @@ view_err: { VOID(pthread_mutex_lock(&LOCK_open)); wait_while_table_is_used(thd, table, HA_EXTRA_FORCE_REOPEN); - table->file->external_lock(thd, F_WRLCK); + table->file->ha_external_lock(thd, F_WRLCK); alter_table_manage_keys(table, table->file->indexes_are_disabled(), alter_info->keys_onoff); - table->file->external_lock(thd, F_UNLCK); + table->file->ha_external_lock(thd, F_UNLCK); VOID(pthread_mutex_unlock(&LOCK_open)); error= ha_commit_stmt(thd); if (ha_commit(thd)) @@ -3813,7 +3813,7 @@ view_err: The following function call will free the new_table pointer, in close_temporary_table(), so we can safely directly jump to err */ - close_temporary_table(thd,new_db,tmp_name); + close_temporary_table(thd, new_db, tmp_name); goto err; } /* Close lock if this is a transactional table */ @@ -4086,7 +4086,7 @@ copy_data_between_tables(TABLE *from,TABLE *to, if (!(copy= new Copy_field[to->s->fields])) DBUG_RETURN(-1); /* purecov: inspected */ - if (to->file->external_lock(thd, F_WRLCK)) + if (to->file->ha_external_lock(thd, F_WRLCK)) DBUG_RETURN(-1); /* We need external lock before we can disable/enable keys */ @@ -4238,7 +4238,7 @@ copy_data_between_tables(TABLE *from,TABLE *to, free_io_cache(from); *copied= found_count; *deleted=delete_count; - if (to->file->external_lock(thd,F_UNLCK)) + if (to->file->ha_external_lock(thd,F_UNLCK)) error=1; DBUG_RETURN(error > 0 ? -1 : 0); }