Re: [Maria-developers] 250bc3b6d74: Ensure that table is truly dropped when using DROP TABLE
Hi, Michael! On Jun 11, Michael Widenius wrote:
revision-id: 250bc3b6d74 (mariadb-10.5.2-390-g250bc3b6d74) parent(s): ba2c2cfb20e author: Michael Widenius <michael.widenus@gmail.com> committer: Michael Widenius <michael.widenus@gmail.com> timestamp: 2020-06-11 11:33:55 +0300 message:
Ensure that table is truly dropped when using DROP TABLE
MDEV-11412 AliSQL: [Feature] Issue#34 Support force drop table
please, put this line ^^^ first in the comment comment.
The used code is largely based on code from Tencent
The problem is that in some rare cases there may be a conflict between .frm files and the files in the storage engine. In this case the DROP TABLE was not able to properly drop the table.
Some MariaDB/MySQL forks has solved this by adding a FORCE option to
TSQL? Or some other fork?
DROP TABLE. After some discussion among MariaDB developers, we concluded that users expects that DROP TABLE should always work, even if the table would not be consistent. There should not be a need to use a separate keyword to ensure that the table is really deleted.
The used solution is: - If a .frm table doesn't exists, try dropping the table from all storage engines. - If the .frm table exists but the table does not exist in the engine try dropping the table from all storage engines.
I don't see why is that needed, if .frm exists, it tells exactly what storage engine to use, there is no point in trying other engines.
- Update storage engines using many table files (.CVS, MyISAM, Aria) to succeed with the drop even if some of the files are missing. - Add HTON_AUTOMATIC_DELETE_TABLE to handlerton's where delete_table() is not needed and always succeed. This is used by ha_delete_table_force() to know which handlers to ignore when trying to drop a table without a .frm file.
The disadvantage of this solution is that a DROP TABLE on a non existing table will be a bit slower as we have to ask all active storage engines if they know anything about the table.
Other things: - Added a new flag MY_IGNORE_ENOENT to my_delete() to not give an error if the file doesn't exist. This simplifies some of the code. - Don't clear thd->error in ha_delete_table() if there was an active error. This is a bug fix. - handler::delete_table() will not abort if first file doesn't exists. This is bug fix to handle the case when a drop table was aborted in the middle. - Cleaned up mysql_rm_table_no_locks() to ensure that if_exists uses same code path as when it's not used. - Use non_existing_Table_error() to detect if table didn't exists. Old code used different errors tests in different position. - Table_triggers_list::drop_all_triggers() now drops trigger file if it can't be parsed instead of leaving it hanging around (bug fix) - InnoDB doesn't anymore print error about .frm file out of sync with InnoDB directory if .frm file does not exists. This change was required to be able to try to drop an InnoDB file when .frm doesn't exists. - Fixed bug in mi_delete_table() where the .MYD file would not be dropped if the .MYI file didn't exists. - Fixed memory leak in Mroonga when deleting non existing table - Fixed memory leak in Connect when deleting non existing table
Bugs fixed introduced by the original version of this commit: MDEV-22826 Presence of Spider prevents tables from being force-deleted from other engines
I don't think this part of the comment makes any sense, when one looks through git history there is no "original version of this commit". It's like a rolled back transaction, that value did not exist, unless you use READ UNCOMMITTED :)
diff --git a/mysql-test/main/drop_table_force.test b/mysql-test/main/drop_table_force.test new file mode 100644 index 00000000000..8fdd79465b7 --- /dev/null +++ b/mysql-test/main/drop_table_force.test @@ -0,1 +1,197 @@ +--source include/have_log_bin.inc +--source include/have_innodb.inc + +# +# This test is based on the orginal test from Tencent for DROP TABLE ... FORCE +# In MariaDB we did reuse the code but MariaDB does not require the FORCE +# keyword to drop a table even if the .frm file or some engine files are +# missing. +# To make it easy to see the differences between the orginal code and +# the new one, we have left some references to the original test case +# + +CALL mtr.add_suppression("Operating system error number"); +CALL mtr.add_suppression("The error means the system cannot"); +CALL mtr.add_suppression("returned OS error 71");
not sure it's a good idea, error numbers aren't very portable, this will blow up some day like a time bomb
+ +let $DATADIR= `select @@datadir`; + +--echo #Test1: table with missing .ibd can be dropped directly +# drop table without ibd +create table t1(a int)engine=innodb; +--remove_file $DATADIR/test/t1.ibd +drop table t1; +--list_files $DATADIR/test/ + +# Original DROP TABLE .. FORCE required SUPER privilege. MariaDB doesn't +--echo # Test droping table without frm without super privilege + +# create table t1 and rm frm +create table t1(a int) engine=innodb; +--remove_file $DATADIR/test/t1.frm + +# create test user +create user test identified by '123456'; +grant all privileges on test.t1 to 'test'@'%'identified by '123456' with grant option;
why with grant option? better remove it if it's not essential for drop to work. if it is essential, why is it?
+ +# connect as test +connect (con_test, localhost, test,'123456', ); +--connection con_test + +# drop table with user test +drop table t1; +--error ER_BAD_TABLE_ERROR +drop table t1; + +# connect as root +--connection default + +--disconnect con_test +drop user test; + +# check files in datadir about t1 +--list_files $DATADIR/test/ + +--echo #Test4: drop table can drop consistent table as well +create table t1(a int) engine=innodb; +drop table t1;
aha, so it works! :)
+ +# check files in datadir about t1 +--list_files $DATADIR/test/ + +--echo #Test5: drop table with triger, and with missing frm +# create table t1 with triger and rm frm +create table t1(a int)engine=innodb; +create trigger t1_trg before insert on t1 for each row begin end; + +let $DATADIR= `select @@datadir`; +--remove_file $DATADIR/test/t1.frm + +drop table t1; +--error ER_BAD_TABLE_ERROR +drop table t1; + +# check files in datadir about t1 +--list_files $DATADIR/test/ + +--echo #Test6: table with foreign key references can not be dropped +# create table with foreign key reference and rm frm +CREATE TABLE parent (id INT NOT NULL, PRIMARY KEY (id)) ENGINE=INNODB; +CREATE TABLE child (id INT, parent_id INT, INDEX par_ind (parent_id), FOREIGN KEY (parent_id) REFERENCES parent(id) ON DELETE CASCADE) ENGINE=INNODB; +--remove_file $DATADIR/test/parent.frm + +# parent can not be dropped when there are foreign key references +--error ER_ROW_IS_REFERENCED_2 +drop table parent; + +# parent can be dropped when there are no foreign key references +drop table child; +drop table parent; + +# check files in datadir about child and parent +--list_files $DATADIR/test/ + +--echo #Test7: drop table twice +create table t1(a int)engine=innodb; +--remove_file $DATADIR/test/t1.frm + +# first drop table will success +drop table t1; + +# check files in datadir about t1 +--list_files $DATADIR/test/ + +# second drop with if exists will also ok +drop table if exists t1; + +# check files in datadir about t1 +--list_files $DATADIR/test/ + +--echo #Test8: check compatibility with if exists +create table t1(a int)engine=innodb; +--remove_file $DATADIR/test/t1.frm + +# first drop will success +drop table t1; + +# check files in datadir about t1 +--list_files $DATADIR/test/ + +# second drop with if exists will success +drop table if exists t1;
you've just tested it in Test7, haven't you?
+ +--echo #Test9: check compatibility with restrict/cascade +# create table with foreign key reference and rm frm +CREATE TABLE parent (id INT NOT NULL, PRIMARY KEY (id)) ENGINE=INNODB; +CREATE TABLE child (id INT, parent_id INT, INDEX par_ind (parent_id), FOREIGN KEY (parent_id) REFERENCES parent(id) ON DELETE CASCADE) ENGINE=INNODB; + +# parent can not be dropped when there are froeign key references
typo "foreign"
+--error ER_ROW_IS_REFERENCED_2 +drop table parent; +--error ER_ROW_IS_REFERENCED_2 +drop table parent restrict; +--error ER_ROW_IS_REFERENCED_2 +drop table parent cascade; +--error ER_ROW_IS_REFERENCED_2 +drop table parent; +--error ER_ROW_IS_REFERENCED_2 +drop table parent restrict; +--error ER_ROW_IS_REFERENCED_2 +drop table parent cascade; + +# parent can be dropped when there are no foreign key references +drop table child; +drop table parent; + +--echo #Test10: drop non-innodb engine table returns ok +# create myisam table t1 and rm .frm +create table t1(a int) engine=myisam; +--remove_file $DATADIR/test/t1.frm +--replace_result \\ / +drop table t1; + +# create myisam table t1 and rm .MYD +create table t1(a int) engine=myisam; +--remove_file $DATADIR/test/t1.MYD +--replace_result \\ / +drop table t1; + +# create myisam table t1 and rm .MYI +create table t1(a int) engine=myisam; +--remove_file $DATADIR/test/t1.MYI +--replace_result \\ / +drop table t1; +--list_files $DATADIR/test/ + +# create Aria table t1 and rm .frm and .MAD +create table t1(a int) engine=aria; +--remove_file $DATADIR/test/t1.frm +--remove_file $DATADIR/test/t1.MAD +--list_files $DATADIR/test/ +--error ER_BAD_TABLE_ERROR +drop table t1; +--replace_result \\ / +show warnings; +--list_files $DATADIR/test/ + +# create Aria table t2 and rm .frm and .MAI +create table t2(a int) engine=aria; +flush tables; +--remove_file $DATADIR/test/t2.frm +--remove_file $DATADIR/test/t2.MAI +--list_files $DATADIR/test/ +--error ER_BAD_TABLE_ERROR +drop table t2; +--replace_result \\ / +show warnings; +--list_files $DATADIR/test/ + +# create Aria table t2 and rm .MAI and .MAD +create table t2(a int) engine=aria; +flush tables; +--remove_file $DATADIR/test/t2.MAD +--remove_file $DATADIR/test/t2.MAI +--list_files $DATADIR/test/ +--replace_result \\ / +drop table t2;
may be csv and archive tests too?
diff --git a/sql/handler.cc b/sql/handler.cc index 1af0157783e..5dd02d057c7 100644 --- a/sql/handler.cc +++ b/sql/handler.cc @@ -2674,26 +2712,34 @@ const char *get_canonical_filename(handler *file, const char *path, }
-/** delete a table in the engine +/** + Delete a table in the engine
+ @return 0 Table was deleted + @return -1 Table didn't exists, no error given + @return # Error from table handler + @note ENOENT and HA_ERR_NO_SUCH_TABLE are not considered errors. - The .frm file will be deleted only if we return 0. + The .frm file should be deleted by the caller only if we return <= 0. */ + int ha_delete_table(THD *thd, handlerton *table_type, const char *path, - const LEX_CSTRING *db, const LEX_CSTRING *alias, bool generate_warning) + const LEX_CSTRING *db, const LEX_CSTRING *alias, + bool generate_warning) { handler *file; char tmp_path[FN_REFLEN]; int error; TABLE dummy_table; TABLE_SHARE dummy_share; + bool is_error= thd->is_error(); DBUG_ENTER("ha_delete_table");
/* table_type is NULL in ALTER TABLE when renaming only .frm files */ if (table_type == NULL || table_type == view_pseudo_hton || ! (file=get_new_handler((TABLE_SHARE*)0, thd->mem_root, table_type))) - DBUG_RETURN(0); + DBUG_RETURN(-1);
"Table didn't exists, no error given"? if get_new_handler() fails it's OOM.
bzero((char*) &dummy_table, sizeof(dummy_table)); bzero((char*) &dummy_share, sizeof(dummy_share)); @@ -2723,8 +2769,10 @@ int ha_delete_table(THD *thd, handlerton *table_type, const char *path, } if (intercept) { + /* Clear error if we got it in this function */ + if (!is_error) thd->clear_error();
this is generally a wrong approach. One should use push_internal_handler to silence errors, not thd->is_error() or thd->clear_error(). In this particilar case there will be no errors if there was no error before ha_delete_table() - which is correct. But if there was an error before, this will leave spurious and confusing errors in `SHOW WARNINGS` output.
- error= 0; + error= -1; } } delete file; @@ -4365,45 +4413,63 @@ uint handler::get_dup_key(int error)
@note We assume that the handler may return more extensions than - was actually used for the file. + was actually used for the file. We also assume that the first + extension is the most important one. If this exist and we can't delete + that one we will abort the delete.
after "the most important one" you can add "(see the comment near handlerton::tablefile_extensions)"
+ If the first one doesn't exists, we have to try to delete all other
"doesn't exist"
+ extension as there is chance that the server had crashed between + the delete of the first file and the next
@retval 0 If we successfully deleted at least one file from base_ext and didn't get any other errors than ENOENT + @retval !0 Error */ + int handler::delete_table(const char *name) { - int saved_error= 0; - int error= 0; - int enoent_or_zero; + int saved_error= ENOENT; + bool abort_if_first_file_error= 1; + bool some_file_deleted= 0; + DBUG_ENTER("handler::delete_table");
+ // For discovery tables, it's ok if first file doesn't exists
why would that be ok?
if (ht->discover_table) - enoent_or_zero= 0; // the table may not exist in the engine, it's ok - else - enoent_or_zero= ENOENT; // the first file of bas_ext() *must* exist + { + abort_if_first_file_error= 0; + saved_error= 0; + if (!bas_ext()) + { + DBUG_ASSERT(ht->flags & HTON_AUTOMATIC_DELETE_TABLE); + DBUG_RETURN(0); // Drop succeded + } + }
for (const char **ext= bas_ext(); *ext ; ext++) { + int error; - if (mysql_file_delete_with_symlink(key_file_misc, name, *ext, 0)) + if ((error= mysql_file_delete_with_symlink(key_file_misc, name, *ext, + MYF(0))))
why do you need this `error` variable, if you never use it?
{ if (my_errno != ENOENT) { + saved_error= my_errno; /* - If error on the first existing file, return the error. + If error other than file not found on the first existing file, + return the error. Otherwise delete as much as possible. */ - if (enoent_or_zero) - return my_errno; - saved_error= my_errno; + if (abort_if_first_file_error) + DBUG_RETURN(saved_error); } } else - enoent_or_zero= 0; // No error for ENOENT - error= enoent_or_zero; + some_file_deleted= 1; + abort_if_first_file_error= 0; } - return saved_error ? saved_error : error; + DBUG_RETURN(some_file_deleted && saved_error == ENOENT ? 0 : saved_error); }
@@ -4439,5 +4505,20 @@ void handler::drop_table(const char *name)
/** + Return true if the error from drop table means that the + table didn't exists +*/ + +bool non_existing_table_error(int error) +{ + return (error == ENOENT || error == HA_ERR_NO_SUCH_TABLE || + error == HA_ERR_UNSUPPORTED || + error == ER_NO_SUCH_TABLE || + error == ER_NO_SUCH_TABLE_IN_ENGINE || + error == ER_WRONG_OBJECT); +}
this should, I believe, be inline in handler.h
+ + +/** Performs checks upon the table.
@@ -4899,5 +4981,89 @@ handler::ha_drop_table(const char *name)
/** + Structure used during force drop table. +*/ + +struct st_force_drop_table_params +{ + const char *path; + const LEX_CSTRING *db; + const LEX_CSTRING *alias; + bool generate_warning; + int error; +}; + + +/** + Try to delete table from a given plugin + Table types with discovery is ignored as these .frm files would have + been created during discovery and thus doesn't need to be found + for drop table force +*/ + +static my_bool delete_table_force(THD *thd, plugin_ref plugin, void *arg) +{ + handlerton *hton = plugin_hton(plugin); + st_force_drop_table_params *param = (st_force_drop_table_params *)arg; + + /* + We have to ignore HEAP tables as these may not have been created yet + We also remove engines that is using discovery (as these will recrate + any missing .frm if needed) and tables marked with + HTON_AUTOMATIC_DELETE_TABLE as for these we can't check if the table + ever existed. + */ + if (!hton->discover_table && hton->db_type != DB_TYPE_HEAP && + !(hton->flags & HTON_AUTOMATIC_DELETE_TABLE)) + { + int error; + error= ha_delete_table(thd, hton, param->path, param->db, + param->alias, param->generate_warning);
This is doing a a lot of extra work. dummy_table/dummy_share, if()s. Allocating and constructing new handlers. I suggest the following: 1. Introduce new handlerton method hton->drop_table(). By default it'll be hton->get_new_handler() get_canonical_filename() file->ha_delete_table() like now, so all engines will still work. But later we can start moving drop table functionality out of the handler. In this commit only engines like blackhole will have a dummy drop_table() method. 2. Here you can do, basically hton->drop_table(hton, param->path) without checking for discovery, without a new flag HTON_AUTOMATIC_DELETE_TABLE. I'm not sure why you check for DB_TYPE_HEAP, could you please explain that?
+ if (error > 0 && !non_existing_table_error(error)) + param->error= error; + if (error == 0) + { + param->error= 0; + return TRUE; // Table was deleted + } + } + return FALSE; +} + +/** + @brief + Traverse all plugins to delete table when .frm file is missing. + + @return -1 Table was not found in any engine + @return 0 Table was found in some engine and delete succeded + @return # Error from first engine that had a table but didn't succeed to + delete the table + @return HA_ERR_ROW_IS_REFERENCED if foreign key reference is encountered, + +*/ + +int ha_delete_table_force(THD *thd, const char *path, const LEX_CSTRING *db, + const LEX_CSTRING *alias, bool generate_warning) +{ + st_force_drop_table_params param; + Table_exists_error_handler no_such_table_handler; + DBUG_ENTER("ha_delete_table_force"); + + param.path= path; + param.db= db; + param.alias= alias; + param.generate_warning= generate_warning;
this is never used, you always invoke ha_delete_table_force() with generate_warning=0.
+ param.error= -1; // Table not found + + thd->push_internal_handler(&no_such_table_handler); + if (plugin_foreach(thd, delete_table_force, MYSQL_STORAGE_ENGINE_PLUGIN, + ¶m)) + param.error= 0; // Delete succeded + thd->pop_internal_handler(); + DBUG_RETURN(param.error); +} + + +/** Create a table in the engine: public interface.
diff --git a/sql/handler.h b/sql/handler.h index f592c635c5d..f5ccdbe1ee4 100644 --- a/sql/handler.h +++ b/sql/handler.h @@ -1780,6 +1780,13 @@ handlerton *ha_default_tmp_handlerton(THD *thd); */ #define HTON_TRANSACTIONAL_AND_NON_TRANSACTIONAL (1 << 17)
+/* + The engine doesn't keep track of tables, delete_table() is not + needed and delete_table() always returns 0 (table deleted). This flag + mainly used to skip storage engines in case of ha_delete_table_force() +*/ +#define HTON_AUTOMATIC_DELETE_TABLE (1 << 18) + class Ha_trx_info;
struct THD_TRANS diff --git a/sql/sql_table.cc b/sql/sql_table.cc index 61b1113f680..7c79acef3ea 100644 --- a/sql/sql_table.cc +++ b/sql/sql_table.cc @@ -2327,36 +2329,42 @@ int mysql_rm_table_no_locks(THD *thd, TABLE_LIST *tables, bool if_exists, thd->find_temporary_table(table) && table->mdl_request.ticket != NULL));
+ /* First try to delete temporary tables and temporary sequences */ - if (table->open_type == OT_BASE_ONLY || !is_temporary_table(table) || - (drop_sequence && table->table->s->table_type != TABLE_TYPE_SEQUENCE)) - error= 1; - else + if ((table->open_type != OT_BASE_ONLY && is_temporary_table(table)) &&
unnecessary parentheses
+ (!drop_sequence || table->table->s->table_type == TABLE_TYPE_SEQUENCE)) { table_creation_was_logged= table->table->s->table_creation_was_logged; if (thd->drop_temporary_table(table->table, &is_trans, true)) { + /* + This is a very unlikely scenaro as dropping a temporary table + should always work. Would be better if we tried to drop all + temporary tables before giving the error. + */ error= 1; goto err; } - error= 0; table->table= 0; + temporary_table_was_dropped= 1; }
- if ((drop_temporary && if_exists) || !error) + if ((drop_temporary && if_exists) || temporary_table_was_dropped) { /* This handles the case of temporary tables. We have the following cases:
- . "DROP TEMPORARY" was executed and a temporary table was affected - (i.e. drop_temporary && !error) or the if_exists was specified (i.e. - drop_temporary && if_exists). - - . "DROP" was executed but a temporary table was affected (.i.e - !error). + - "DROP TEMPORARY" was executed and table was dropped + temporary_table_was_dropped == 1 + - "DROP TEMPORARY IF EXISTS" was specified but no temporary table + existed + temporary_table_was_dropped == 0 */ if (!dont_log_query && table_creation_was_logged) { /* + DROP TEMPORARY succeded. For the moment when we only come + here on success (error == 0)
DBUG_ASSERT(error == 0) please
+ If there is an error, we don't know the type of the engine at this point. So, we keep it in the trx-cache. */ @@ -2387,7 +2395,8 @@ int mysql_rm_table_no_locks(THD *thd, TABLE_LIST *tables, bool if_exists, is no need to proceed with the code that tries to drop a regular table. */ - if (!error) continue; + if (temporary_table_was_dropped)
can temporary_table_was_dropped be false here?
+ continue; } else if (!drop_temporary) { @@ -2402,48 +2411,33 @@ int mysql_rm_table_no_locks(THD *thd, TABLE_LIST *tables, bool if_exists, path_length= build_table_filename(path, sizeof(path) - 1, db.str, alias.str, reg_ext, 0); } + DEBUG_SYNC(thd, "rm_table_no_locks_before_delete_table"); error= 0; - if (drop_temporary || - (ha_table_exists(thd, &db, &alias, &table_type, &is_sequence) == 0 && + if (drop_temporary) + { + /* "DROP TEMPORARY" but a temporary table was not found */ + error= ENOENT; + } + else if (((frm_exists= ha_table_exists(thd, &db, &alias, &table_type, + &is_sequence)) == 0 &&
this is an overkill. ha_table_exists() will do a full table discovery, you don't need it here, because you'll try to delete anyway. So, basically you only need dd_frm_type() here.
table_type == 0) || (!drop_view && (was_view= (table_type == view_pseudo_hton))) || (drop_sequence && !is_sequence)) { /* One of the following cases happened: - . "DROP TEMPORARY" but a temporary table was not found. . "DROP" but table was not found . "DROP TABLE" statement, but it's a view. . "DROP SEQUENCE", but it's not a sequence */ - was_table= drop_sequence && table_type; - if (if_exists) - { - char buff[FN_REFLEN]; - int err= (drop_sequence ? ER_UNKNOWN_SEQUENCES : - ER_BAD_TABLE_ERROR); - String tbl_name(buff, sizeof(buff), system_charset_info); - tbl_name.length(0); - tbl_name.append(&db); - tbl_name.append('.'); - tbl_name.append(&table->table_name); - push_warning_printf(thd, Sql_condition::WARN_LEVEL_NOTE, - err, ER_THD(thd, err), - tbl_name.c_ptr_safe()); - - /* - Our job is done here. This statement was added to avoid executing - unnecessary code farther below which in some strange corner cases - caused the server to crash (see MDEV-17896). - */ - goto log_query; - } - else - { - non_tmp_error = (drop_temporary ? non_tmp_error : TRUE); - error= 1; - } + wrong_drop_sequence= drop_sequence && table_type; + was_table|= wrong_drop_sequence; + local_non_tmp_error= 1; + error= -1; + if ((!frm_exists && !table_type) || // no .frm + if_exists) + error= ENOENT; } else { @@ -2509,49 +2507,138 @@ int mysql_rm_table_no_locks(THD *thd, TABLE_LIST *tables, bool if_exists, goto err; } } - else + + /* + Delete the .frm file if we managed to delete the table from the + engine or the table didn't exists in the engine + */ + if (likely(!error) || non_existing_table_error(error)) { /* Delete the table definition file */ strmov(end,reg_ext); if (table_type && table_type != view_pseudo_hton && - table_type->discover_table) + (table_type->discover_table || error))
I don't think this new drop table logic needs to have any special handling of discovery. If the table didn't exist in engine - it's the normal case now, discovery or not, it's be handled normally.
{ /* - Table type is using discovery and may not need a .frm file. + Table type is using discovery and may not need a .frm file + or the .frm file existed but no table in engine. Delete it silently if it exists */ - (void) mysql_file_delete(key_file_frm, path, MYF(0)); + if (mysql_file_delete(key_file_frm, path, + MYF(MY_WME | MY_IGNORE_ENOENT))) + error= my_errno; } else if (unlikely(mysql_file_delete(key_file_frm, path, - MYF(MY_WME)))) + !error ? MYF(MY_WME) : + MYF(MY_WME | MY_IGNORE_ENOENT)))) { frm_delete_error= my_errno; DBUG_ASSERT(frm_delete_error); } } + frm_was_deleted= 1; if (thd->replication_flags & OPTION_IF_EXISTS) log_if_exists= 1;
- if (likely(!error)) + if (frm_delete_error) { - int trigger_drop_error= 0; + /* + Remember error if unexpected error from dropping the .frm file + or we got an error from ha_delete_table() + */ + if (frm_delete_error != ENOENT) + error= frm_delete_error; + else if (if_exists && ! error) + thd->clear_error();
no, please, don't do thd->clear_error(). I've seen and fixed so many bugs because of that. The last one - just an hour ago. Use push_internal_handler() instead.
+ } + if (likely(!error) || !frm_delete_error) + non_tmp_table_deleted= TRUE;
- if (likely(!frm_delete_error)) + if (likely(!error) || non_existing_table_error(error)) { + trigger_drop_executed= 1; + + if (Table_triggers_list::drop_all_triggers(thd, &db, + &table->table_name, + MYF(MY_WME | + MY_IGNORE_ENOENT))) + error= error ? error : -1; + } + local_non_tmp_error|= MY_TEST(error); + } + + /* + If there was no .frm file and the table is not temporary, + scan all engines try to drop the table from there. + This is to ensure we don't have any partial table files left. + + We check for trigger_drop_executed to ensure we don't again try + to drop triggers when it failed above (after sucecssfully dropping + the table). + */ + if (non_existing_table_error(error) && !drop_temporary && + table_type != view_pseudo_hton && !trigger_drop_executed && + !wrong_drop_sequence) + { + char *end; + int ferror= 0; + + /* Remove extension for delete */ + *(end = path + path_length - reg_ext_length) = '\0'; + ferror= ha_delete_table_force(thd, path, &db, + &table->table_name, 0); + if (!ferror) + { + /* Table existed and was deleted */ non_tmp_table_deleted= TRUE; - trigger_drop_error= - Table_triggers_list::drop_all_triggers(thd, &db, - &table->table_name); + local_non_tmp_error= 0; + error= 0; } + if (ferror <= 0) + { + ferror= 0; // Ignore table not found
- if (unlikely(trigger_drop_error) || - (frm_delete_error && frm_delete_error != ENOENT)) - error= 1; - else if (frm_delete_error && if_exists) - thd->clear_error(); + /* Delete the table definition file */ + if (!frm_was_deleted) + { + strmov(end, reg_ext); + if (mysql_file_delete(key_file_frm, path, + MYF(MY_WME | MY_IGNORE_ENOENT))) + ferror= my_errno; } - non_tmp_error|= MY_TEST(error); + if (Table_triggers_list::drop_all_triggers(thd, &db, + &table->table_name, + MYF(MY_WME | + MY_IGNORE_ENOENT))) + ferror= -1;
Why do you need two copies of code to drop triggers? Why cannot you do it once, on the first drop_all_triggers() place?
} + if (!error) + error= ferror; + } + + /* + Don't give an error if we are using IF EXISTS for a table that + didn't exists + */ + + if (if_exists && non_existing_table_error(error)) + { + char buff[FN_REFLEN]; + int err= (drop_sequence ? ER_UNKNOWN_SEQUENCES : + ER_BAD_TABLE_ERROR); + String tbl_name(buff, sizeof(buff), system_charset_info); + tbl_name.length(0); + tbl_name.append(&db); + tbl_name.append('.'); + tbl_name.append(&table->table_name); + push_warning_printf(thd, Sql_condition::WARN_LEVEL_NOTE, + err, ER_THD(thd, err), + tbl_name.c_ptr_safe()); + error= 0; + local_non_tmp_error= 0; + drop_table_not_done= 1; + } + non_tmp_error|= local_non_tmp_error;
if (error) { diff --git a/storage/blackhole/ha_blackhole.cc b/storage/blackhole/ha_blackhole.cc index 15548350b20..98589f1d043 100644 --- a/storage/blackhole/ha_blackhole.cc +++ b/storage/blackhole/ha_blackhole.cc @@ -399,7 +399,7 @@ static int blackhole_init(void *p) blackhole_hton= (handlerton *)p; blackhole_hton->db_type= DB_TYPE_BLACKHOLE_DB; blackhole_hton->create= blackhole_create_handler; - blackhole_hton->flags= HTON_CAN_RECREATE; + blackhole_hton->flags= HTON_CAN_RECREATE | HTON_AUTOMATIC_DELETE_TABLE;
mysql_mutex_init(bh_key_mutex_blackhole, &blackhole_mutex, MY_MUTEX_INIT_FAST); diff --git a/storage/blackhole/ha_blackhole.h b/storage/blackhole/ha_blackhole.h index 6ee30877b64..646fba6da9f 100644 --- a/storage/blackhole/ha_blackhole.h +++ b/storage/blackhole/ha_blackhole.h @@ -95,6 +95,10 @@ class ha_blackhole: public handler THR_LOCK_DATA **store_lock(THD *thd, THR_LOCK_DATA **to, enum thr_lock_type lock_type); + int delete_table(const char *name) + { + return 0; + } private: virtual int write_row(const uchar *buf); virtual int update_row(const uchar *old_data, const uchar *new_data); diff --git a/storage/connect/ha_connect.cc b/storage/connect/ha_connect.cc index 0194b08c792..b1b44085e53 100644 --- a/storage/connect/ha_connect.cc +++ b/storage/connect/ha_connect.cc @@ -5192,7 +5192,8 @@ int ha_connect::delete_or_rename_table(const char *name, const char *to) } // endif pos
} // endif open_table_def - + else + rc= ENOENT; free_table_share(share); } else // Temporary file ok= true; diff --git a/storage/connect/mysql-test/connect/r/drop-open-error.result b/storage/connect/mysql-test/connect/r/drop-open-error.result index f0ad8553d8b..34f58a845dc 100644 --- a/storage/connect/mysql-test/connect/r/drop-open-error.result +++ b/storage/connect/mysql-test/connect/r/drop-open-error.result @@ -2,6 +2,8 @@ create table t1 (c varchar(8)); create table tcon engine=connect table_type=mysql CONNECTION='mysql://root@localhost/test/t1' SRCDEF='select c from t1 where c in ("foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar", "foo", "bar", "qux", "foobar")'; ERROR HY000: Too long value for 'SRCDEF' drop table mdev9949; +Warnings: +Warning 1017 Can't find file: './test/mdev9949.dos' (errno: 2 "No such file or directory") drop table t1; select @@secure_file_priv 'must be NULL'; must be NULL diff --git a/storage/federated/ha_federated.cc b/storage/federated/ha_federated.cc index 00168791d35..b62b74d12bb 100644 --- a/storage/federated/ha_federated.cc +++ b/storage/federated/ha_federated.cc @@ -484,7 +484,8 @@ int federated_db_init(void *p) federated_hton->commit= federated_commit; federated_hton->rollback= federated_rollback; federated_hton->create= federated_create_handler; - federated_hton->flags= HTON_ALTER_NOT_SUPPORTED | HTON_NO_PARTITION; + federated_hton->flags= (HTON_ALTER_NOT_SUPPORTED | HTON_NO_PARTITION | + HTON_AUTOMATIC_DELETE_TABLE);
/* Support for transactions disabled until WL#2952 fixes it. diff --git a/storage/federated/ha_federated.h b/storage/federated/ha_federated.h index 080d0ebd5f0..2e785f87858 100644 --- a/storage/federated/ha_federated.h +++ b/storage/federated/ha_federated.h @@ -244,7 +244,10 @@ class ha_federated: public handler void update_auto_increment(void); int repair(THD* thd, HA_CHECK_OPT* check_opt); int optimize(THD* thd, HA_CHECK_OPT* check_opt); - + int delete_table(const char *name) + { + return 0; + } int delete_all_rows(void); int truncate(); int create(const char *name, TABLE *form, diff --git a/storage/federatedx/ha_federatedx.cc b/storage/federatedx/ha_federatedx.cc index 6547964cc11..2370473236e 100644 --- a/storage/federatedx/ha_federatedx.cc +++ b/storage/federatedx/ha_federatedx.cc @@ -438,7 +438,8 @@ int federatedx_db_init(void *p) federatedx_hton->rollback= ha_federatedx::rollback; federatedx_hton->discover_table_structure= ha_federatedx::discover_assisted; federatedx_hton->create= federatedx_create_handler; - federatedx_hton->flags= HTON_ALTER_NOT_SUPPORTED; + federatedx_hton->flags= (HTON_ALTER_NOT_SUPPORTED | + HTON_AUTOMATIC_DELETE_TABLE); federatedx_hton->create_derived= create_federatedx_derived_handler; federatedx_hton->create_select= create_federatedx_select_handler;
diff --git a/storage/federatedx/ha_federatedx.h b/storage/federatedx/ha_federatedx.h index a62456e1c33..67fe5f8cc22 100644 --- a/storage/federatedx/ha_federatedx.h +++ b/storage/federatedx/ha_federatedx.h @@ -431,7 +431,10 @@ class ha_federatedx: public handler void update_auto_increment(void); int repair(THD* thd, HA_CHECK_OPT* check_opt); int optimize(THD* thd, HA_CHECK_OPT* check_opt); - + int delete_table(const char *name) + { + return 0; + } int delete_all_rows(void); int create(const char *name, TABLE *form, HA_CREATE_INFO *create_info); //required diff --git a/storage/innobase/handler/ha_innodb.cc b/storage/innobase/handler/ha_innodb.cc index 6940f8ff3bd..97cfdb01d9f 100644 --- a/storage/innobase/handler/ha_innodb.cc +++ b/storage/innobase/handler/ha_innodb.cc @@ -13347,5 +13347,18 @@ ha_innobase::discard_or_import_tablespace( }
/** + @return 1 if frm file exists + @return 0 if it doesn't exists +*/ + +static bool frm_file_exists(const char *path) +{ + char buff[FN_REFLEN]; + strxnmov(buff, FN_REFLEN, path, reg_ext, NullS); + return !access(buff, F_OK); +} + + +/** Drops a table from an InnoDB database. Before calling this function, MySQL calls innobase_commit to commit the transaction of the current user. @@ -13446,7 +13459,9 @@ inline int ha_innobase::delete_table(const char* name, enum_sql_command sqlcom) } }
- if (err == DB_TABLE_NOT_FOUND) { + if (err == DB_TABLE_NOT_FOUND && + frm_file_exists(name)) + { /* Test to drop all tables which matches db/tablename + '#'. Only partitions can have '#' as non-first character in the table name! diff --git a/storage/mroonga/mrn_table.cpp b/storage/mroonga/mrn_table.cpp index b1b2db6161b..037a6a59487 100644 --- a/storage/mroonga/mrn_table.cpp +++ b/storage/mroonga/mrn_table.cpp @@ -1080,6 +1080,7 @@ TABLE_SHARE *mrn_create_tmp_table_share(TABLE_LIST *table_list, const char *path if (open_table_def(thd, share, GTS_TABLE)) { *error = ER_CANT_OPEN_FILE; + mrn_free_tmp_table_share(share); DBUG_RETURN(NULL); } DBUG_RETURN(share); diff --git a/storage/myisam/mi_delete_table.c b/storage/myisam/mi_delete_table.c index 7990c3e8a80..d318b44720a 100644 --- a/storage/myisam/mi_delete_table.c +++ b/storage/myisam/mi_delete_table.c @@ -28,19 +28,23 @@
int mi_delete_table(const char *name) { + int error= 0; DBUG_ENTER("mi_delete_table");
#ifdef EXTRA_DEBUG check_table_is_closed(name,"delete"); #endif
- if (mysql_file_delete_with_symlink(mi_key_file_kfile, name, MI_NAME_IEXT, MYF(MY_WME)) || - mysql_file_delete_with_symlink(mi_key_file_dfile, name, MI_NAME_DEXT, MYF(MY_WME))) - DBUG_RETURN(my_errno); + if (mysql_file_delete_with_symlink(mi_key_file_kfile, name, MI_NAME_IEXT, + MYF(MY_WME))) + error= my_errno; + if (mysql_file_delete_with_symlink(mi_key_file_dfile, name, MI_NAME_DEXT, + MYF(MY_WME))) + error= my_errno;
// optionally present: mysql_file_delete_with_symlink(mi_key_file_dfile, name, ".OLD", MYF(0)); mysql_file_delete_with_symlink(mi_key_file_dfile, name, ".TMD", MYF(0));
- DBUG_RETURN(0); + DBUG_RETURN(error); } diff --git a/storage/perfschema/ha_perfschema.cc b/storage/perfschema/ha_perfschema.cc index 33d4c854c8b..c4c59d109f5 100644 --- a/storage/perfschema/ha_perfschema.cc +++ b/storage/perfschema/ha_perfschema.cc @@ -95,10 +95,11 @@ static int pfs_init_func(void *p)
pfs_hton->create= pfs_create_handler; pfs_hton->show_status= pfs_show_status; - pfs_hton->flags= HTON_ALTER_NOT_SUPPORTED | + pfs_hton->flags= (HTON_ALTER_NOT_SUPPORTED | HTON_TEMPORARY_NOT_SUPPORTED | HTON_NO_PARTITION | - HTON_NO_BINLOG_ROW_OPT; + HTON_NO_BINLOG_ROW_OPT | + HTON_AUTOMATIC_DELETE_TABLE);
/* As long as the server implementation keeps using legacy_db_type, diff --git a/storage/sequence/sequence.cc b/storage/sequence/sequence.cc index 8684a5c60b9..31522b8f3b7 100644 --- a/storage/sequence/sequence.cc +++ b/storage/sequence/sequence.cc @@ -69,10 +69,15 @@ class ha_seq: public handler
/* open/close/locking */ int create(const char *name, TABLE *table_arg, - HA_CREATE_INFO *create_info) { return HA_ERR_WRONG_COMMAND; } + HA_CREATE_INFO *create_info) + { return HA_ERR_WRONG_COMMAND; }
int open(const char *name, int mode, uint test_if_locked); int close(void); + int delete_table(const char *name) + { + return 0; + } THR_LOCK_DATA **store_lock(THD *, THR_LOCK_DATA **, enum thr_lock_type);
/* table scan */ @@ -503,6 +508,7 @@ static int init(void *p) hton->savepoint_set= hton->savepoint_rollback= hton->savepoint_release= dummy_savepoint; hton->create_group_by= create_group_by_handler; + hton->flags= HTON_AUTOMATIC_DELETE_TABLE; return 0; }
@@ -526,4 +532,3 @@ maria_declare_plugin(sequence) MariaDB_PLUGIN_MATURITY_STABLE } maria_declare_plugin_end; - diff --git a/storage/sphinx/ha_sphinx.cc b/storage/sphinx/ha_sphinx.cc index 8403c767796..d60a4d229e6 100644 --- a/storage/sphinx/ha_sphinx.cc +++ b/storage/sphinx/ha_sphinx.cc @@ -696,7 +696,7 @@ handlerton sphinx_hton = NULL, // create_cursor_read_view NULL, // set_cursor_read_view NULL, // close_cursor_read_view - HTON_CAN_RECREATE + HTON_CAN_RECREATE | HTON_AUTOMATIC_DELETE_TABLE }; #else static handlerton * sphinx_hton_ptr = NULL; @@ -749,7 +749,7 @@ static int sphinx_init_func ( void * p ) hton->close_connection = sphinx_close_connection; hton->show_status = sphinx_show_status; hton->panic = sphinx_panic; - hton->flags = HTON_CAN_RECREATE; + hton->flags = HTON_CAN_RECREATE | HTON_AUTOMATIC_DELETE_TABLE; #endif } SPH_RET(0); diff --git a/storage/spider/spd_table.cc b/storage/spider/spd_table.cc index 1607dd07902..583bfebbed6 100644 --- a/storage/spider/spd_table.cc +++ b/storage/spider/spd_table.cc @@ -7249,7 +7249,7 @@ int spider_db_init( DBUG_ENTER("spider_db_init"); spider_hton_ptr = spider_hton;
- spider_hton->flags = HTON_NO_FLAGS; + spider_hton->flags = HTON_NO_FLAGS | HTON_AUTOMATIC_DELETE_TABLE; #ifdef HTON_CAN_READ_CONNECT_STRING_IN_PARTITION spider_hton->flags |= HTON_CAN_READ_CONNECT_STRING_IN_PARTITION; #endif
Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
Hi, Michael!
Please use Monty. No reason to be formal!
Ensure that table is truly dropped when using DROP TABLE
MDEV-11412 AliSQL: [Feature] Issue#34 Support force drop table
please, put this line ^^^ first in the comment comment.
Already done, as we discussed
The used code is largely based on code from Tencent
The problem is that in some rare cases there may be a conflict between .frm files and the files in the storage engine. In this case the DROP TABLE was not able to properly drop the table.
Some MariaDB/MySQL forks has solved this by adding a FORCE option to
TSQL? Or some other fork?
At least Alibaba and TSQL.
DROP TABLE. After some discussion among MariaDB developers, we concluded that users expects that DROP TABLE should always work, even if the table would not be consistent. There should not be a need to use a separate keyword to ensure that the table is really deleted.
The used solution is: - If a .frm table doesn't exists, try dropping the table from all storage engines. - If the .frm table exists but the table does not exist in the engine try dropping the table from all storage engines.
I don't see why is that needed, if .frm exists, it tells exactly what storage engine to use, there is no point in trying other engines.
I had cases on my machine where I had a frm but tables from another engine. May happen when you do an alter table from one engine to another. If the .frm is inconsistent, there is no harm to try a drop in all engines. After all, this is not something that happens often and when it happens, better to check things around. <cut>
Bugs fixed introduced by the original version of this commit: MDEV-22826 Presence of Spider prevents tables from being force-deleted from other engines
I don't think this part of the comment makes any sense, when one looks through git history there is no "original version of this commit".
I want to record that a MDEV is fixed by this commit. I think that is perfectly reasonable and correct thing to do. So this is more a comment for Jira than anything else.
It's like a rolled back transaction, that value did not exist, unless you use READ UNCOMMITTED :)
Yes, it doesn't exist in the code, but it exists in Jira. I assume you would not like me to delete the Jira entry?
diff --git a/mysql-test/main/drop_table_force.test b/mysql-test/main/drop_table_force.test new file mode 100644 index 00000000000..8fdd79465b7 --- /dev/null +++ b/mysql-test/main/drop_table_force.test @@ -0,1 +1,197 @@ +--source include/have_log_bin.inc +--source include/have_innodb.inc + +# +# This test is based on the orginal test from Tencent for DROP TABLE ... FORCE +# In MariaDB we did reuse the code but MariaDB does not require the FORCE +# keyword to drop a table even if the .frm file or some engine files are +# missing. +# To make it easy to see the differences between the orginal code and +# the new one, we have left some references to the original test case +# + +CALL mtr.add_suppression("Operating system error number"); +CALL mtr.add_suppression("The error means the system cannot"); +CALL mtr.add_suppression("returned OS error 71");
not sure it's a good idea, error numbers aren't very portable, this will blow up some day like a time bomb
This is from the original commit. I don't think they are relevant anymore but no harm in keeping them. Buildbot will show us if things are wrong, so nothing that can really 'blow up'. <cut>
+# create test user +create user test identified by '123456'; +grant all privileges on test.t1 to 'test'@'%'identified by '123456' with grant option;
why with grant option? better remove it if it's not essential for drop to work. if it is essential, why is it?
Read the comment in the beginning of the test. This is to test that one doesn't need a separate privilge to be able to drop an inconsistent table.
+ +# connect as test +connect (con_test, localhost, test,'123456', ); +--connection con_test + +# drop table with user test +drop table t1; +--error ER_BAD_TABLE_ERROR +drop table t1; + +# connect as root +--connection default + +--disconnect con_test +drop user test; + +# check files in datadir about t1 +--list_files $DATADIR/test/ + +--echo #Test4: drop table can drop consistent table as well +create table t1(a int) engine=innodb; +drop table t1;
aha, so it works! :)
Orginal code was drop table followed by drop table force. <cut>
+# second drop with if exists will success +drop table if exists t1;
you've just tested it in Test7, haven't you?
Read again comment at the beginning of the test file. <cut>
+# create Aria table t2 and rm .MAI and .MAD +create table t2(a int) engine=aria; +flush tables; +--remove_file $DATADIR/test/t2.MAD +--remove_file $DATADIR/test/t2.MAI +--list_files $DATADIR/test/ +--replace_result \\ / +drop table t2;
may be csv and archive tests too?
CSV has only one table file, so not that interesting as it's covered by other tests. Archive has discovery and thus will automatically remove the .frm or .ARZ in case of missing files. I have now added bot to make the test case more complete.
diff --git a/sql/handler.cc b/sql/handler.cc index 1af0157783e..5dd02d057c7 100644 --- a/sql/handler.cc +++ b/sql/handler.cc @@ -2674,26 +2712,34 @@ const char *get_canonical_filename(handler *file, const char *path, }
-/** delete a table in the engine +/** + Delete a table in the engine
+ @return 0 Table was deleted + @return -1 Table didn't exists, no error given + @return # Error from table handler + @note ENOENT and HA_ERR_NO_SUCH_TABLE are not considered errors. - The .frm file will be deleted only if we return 0. + The .frm file should be deleted by the caller only if we return <= 0. */ + int ha_delete_table(THD *thd, handlerton *table_type, const char *path, - const LEX_CSTRING *db, const LEX_CSTRING *alias, bool generate_warning) + const LEX_CSTRING *db, const LEX_CSTRING *alias, + bool generate_warning) { handler *file; char tmp_path[FN_REFLEN]; int error; TABLE dummy_table; TABLE_SHARE dummy_share; + bool is_error= thd->is_error(); DBUG_ENTER("ha_delete_table");
/* table_type is NULL in ALTER TABLE when renaming only .frm files */ if (table_type == NULL || table_type == view_pseudo_hton || ! (file=get_new_handler((TABLE_SHARE*)0, thd->mem_root, table_type))) - DBUG_RETURN(0); + DBUG_RETURN(-1);
"Table didn't exists, no error given"?
Yes (for this function)
if get_new_handler() fails it's OOM.
No problem with OOM as it sets thd->error() and the user will get that error message.
bzero((char*) &dummy_table, sizeof(dummy_table)); bzero((char*) &dummy_share, sizeof(dummy_share)); @@ -2723,8 +2769,10 @@ int ha_delete_table(THD *thd, handlerton *table_type, const char *path, } if (intercept) { + /* Clear error if we got it in this function */ + if (!is_error) thd->clear_error();
this is generally a wrong approach. One should use push_internal_handler to silence errors, not thd->is_error() or thd->clear_error().
I know that silensing the error is normally wrong approach. Old code did that without no testing which was definitely wrong and hide some errors. However if we got a table not exists error from the handler and there was no error from before, this is a safe way to do it.
In this particilar case there will be no errors if there was no error before ha_delete_table() - which is correct. But if there was an error before, this will leave spurious and confusing errors in `SHOW WARNINGS` output.
The only thing that will be left in SHOW WARNINGS is a note that a file didn't exist, not a big deal for an edge case. In case of ha_delete_table_force(), which is the one that can generate many warnings from different engines, we have error handler in place.
delete file; @@ -4365,45 +4413,63 @@ uint handler::get_dup_key(int error)
@note We assume that the handler may return more extensions than - was actually used for the file. + was actually used for the file. We also assume that the first + extension is the most important one. If this exist and we can't delete + that one we will abort the delete.
after "the most important one" you can add "(see the comment near handlerton::tablefile_extensions)"
Done.
+ If the first one doesn't exists, we have to try to delete all other
"doesn't exist"
+ extension as there is chance that the server had crashed between + the delete of the first file and the next
@retval 0 If we successfully deleted at least one file from base_ext and didn't get any other errors than ENOENT + @retval !0 Error */ + int handler::delete_table(const char *name) { - int saved_error= 0; - int error= 0; - int enoent_or_zero; + int saved_error= ENOENT; + bool abort_if_first_file_error= 1; + bool some_file_deleted= 0; + DBUG_ENTER("handler::delete_table");
+ // For discovery tables, it's ok if first file doesn't exists
why would that be ok?
For tables with discovery, there may not be any files on disk. They may be generated by the discovery process. That first file in a list is missing doesn't mean that there isn't other files.
for (const char **ext= bas_ext(); *ext ; ext++) { + int error; - if (mysql_file_delete_with_symlink(key_file_misc, name, *ext, 0)) + if ((error= mysql_file_delete_with_symlink(key_file_misc, name, *ext, + MYF(0))))
why do you need this `error` variable, if you never use it?
Mostly for debugging. The compiler will delete it, so no big deal. I originally thought my_handler_delete_with_symlink() should return the errno, but as it can generate several errno that didn't work. <cut>
/** + Return true if the error from drop table means that the + table didn't exists +*/ + +bool non_existing_table_error(int error) +{ + return (error == ENOENT || error == HA_ERR_NO_SUCH_TABLE || + error == HA_ERR_UNSUPPORTED || + error == ER_NO_SUCH_TABLE || + error == ER_NO_SUCH_TABLE_IN_ENGINE || + error == ER_WRONG_OBJECT); +}
this should, I believe, be inline in handler.h
After having to change this multiple times and have to recompile, I added this to the .c file. As this is only used for DDL's (which are slow), it's better to NOT have it in the head file as it's generates less code to have it here. <cut>
+static my_bool delete_table_force(THD *thd, plugin_ref plugin, void *arg) +{ + handlerton *hton = plugin_hton(plugin); + st_force_drop_table_params *param = (st_force_drop_table_params *)arg; + + /* + We have to ignore HEAP tables as these may not have been created yet + We also remove engines that is using discovery (as these will recrate + any missing .frm if needed) and tables marked with + HTON_AUTOMATIC_DELETE_TABLE as for these we can't check if the table + ever existed. + */ + if (!hton->discover_table && hton->db_type != DB_TYPE_HEAP && + !(hton->flags & HTON_AUTOMATIC_DELETE_TABLE)) + { + int error; + error= ha_delete_table(thd, hton, param->path, param->db, + param->alias, param->generate_warning);
This is doing a a lot of extra work. dummy_table/dummy_share, if()s. Allocating and constructing new handlers.
This is for extreme cases when there is no reason to optimize for performance but less code is better.
I suggest the following:
1. Introduce new handlerton method hton->drop_table(). By default it'll be
hton->get_new_handler() get_canonical_filename() file->ha_delete_table()
like now, so all engines will still work. But later we can start moving drop table functionality out of the handler. In this commit only engines like blackhole will have a dummy drop_table() method.
We already have two handler delete_table methods. Don't want to add yet another one. Note that a several handlers needs special handling. (Just check who have HTON_AUTOMATIC_DELETE_TABLE), not just blackhole, According to your suggestion, we would have to create a new drop_table() for all engines and have flags when to generate errors and which errors. And we would have to duplicate some of that code in ha_delete_table(). Definetly much more code than the current one and much more changes in many engines.
2. Here you can do, basically
hton->drop_table(hton, param->path)
without checking for discovery, without a new flag HTON_AUTOMATIC_DELETE_TABLE. I'm not sure why you check for DB_TYPE_HEAP, could you please explain that?
See code comment! HEAP TABLES are created on open, so when we call delete_table_force() the table is not yet created. We can't detect that a not yet created table exists, can we ? <cut>
+int ha_delete_table_force(THD *thd, const char *path, const LEX_CSTRING *db, + const LEX_CSTRING *alias, bool generate_warning) +{ + st_force_drop_table_params param; + Table_exists_error_handler no_such_table_handler; + DBUG_ENTER("ha_delete_table_force"); + + param.path= path; + param.db= db; + param.alias= alias; + param.generate_warning= generate_warning;
this is never used, you always invoke ha_delete_table_force() with generate_warning=0.
Yes, I know. This was in the original code and it was not used there either. However, I can imagine there are scenarios where this would be useful in the future, so I kept it. If you feel strongly about it I can delete the generate_warning options. <cut>
index 61b1113f680..7c79acef3ea 100644 --- a/sql/sql_table.cc +++ b/sql/sql_table.cc @@ -2327,36 +2329,42 @@ int mysql_rm_table_no_locks(THD *thd, TABLE_LIST *tables, bool if_exists, thd->find_temporary_table(table) && table->mdl_request.ticket != NULL));
+ /* First try to delete temporary tables and temporary sequences */ - if (table->open_type == OT_BASE_ONLY || !is_temporary_table(table) || - (drop_sequence && table->table->s->table_type != TABLE_TYPE_SEQUENCE)) - error= 1; - else + if ((table->open_type != OT_BASE_ONLY && is_temporary_table(table)) &&
unnecessary parentheses
Yes, but easier to read with != and other things and especially when it's combined with another expression with != <cut>
if (!dont_log_query && table_creation_was_logged) { /* + DROP TEMPORARY succeded. For the moment when we only come + here on success (error == 0)
DBUG_ASSERT(error == 0) please
Not needed as the code makes it clear if one looks up a couple of lines. The comment is more clear than an assert. <cut>
@@ -2387,7 +2395,8 @@ int mysql_rm_table_no_locks(THD *thd, TABLE_LIST *tables, bool if_exists, is no need to proceed with the code that tries to drop a regular table. */ - if (!error) continue; + if (temporary_table_was_dropped)
can temporary_table_was_dropped be false here?
yes The condition for the branch is: if ((drop_temporary && if_exists) || temporary_table_was_dropped)
+ continue; } else if (!drop_temporary) { @@ -2402,48 +2411,33 @@ int mysql_rm_table_no_locks(THD *thd, TABLE_LIST *tables, bool if_exists, path_length= build_table_filename(path, sizeof(path) - 1, db.str, alias.str, reg_ext, 0); } + DEBUG_SYNC(thd, "rm_table_no_locks_before_delete_table"); error= 0; - if (drop_temporary || - (ha_table_exists(thd, &db, &alias, &table_type, &is_sequence) == 0 && + if (drop_temporary) + { + /* "DROP TEMPORARY" but a temporary table was not found */ + error= ENOENT; + } + else if (((frm_exists= ha_table_exists(thd, &db, &alias, &table_type, + &is_sequence)) == 0 &&
this is an overkill. ha_table_exists() will do a full table discovery, you don't need it here, because you'll try to delete anyway.
This was in old code, nothing changed. I assume that we also need this for discovery as ha_delete_table() needs to know in which engine the table exists. Note that if .frm file exists, ha_table_exists() is going to do just dd_frm_type(). <cut>
+ if (frm_delete_error) { - int trigger_drop_error= 0; + /* + Remember error if unexpected error from dropping the .frm file + or we got an error from ha_delete_table() + */ + if (frm_delete_error != ENOENT) + error= frm_delete_error; + else if (if_exists && ! error) + thd->clear_error();
no, please, don't do thd->clear_error(). I've seen and fixed so many bugs because of that. The last one - just an hour ago. Use push_internal_handler() instead.
This is from the old code, nothing I added (I just moved thing around). I don't have time to fix this now and better for this patch keep things same as before. You can create a new MDEV for fixing this if needed. <cut>
+ if (Table_triggers_list::drop_all_triggers(thd, &db, + &table->table_name, + MYF(MY_WME | + MY_IGNORE_ENOENT))) + ferror= -1;
Why do you need two copies of code to drop triggers? Why cannot you do it once, on the first drop_all_triggers() place?
Because the first drop_all_triggers is only done if the .frm exists and the table was dropped. This is part of the "drop force", but only in the cases where it makes sence to do DROP FORCE: if (non_existing_table_error(error) && !drop_temporary && table_type != view_pseudo_hton && !trigger_drop_executed && !wrong_drop_sequence) { Regards, Monty
Hi, Monty! On Jun 13, Michael Widenius wrote:
Hi, Michael!
Please use Monty. No reason to be formal!
That comes from the sender's name :) I replace it manually, but sometimes I forget to.
+# create test user +create user test identified by '123456'; +grant all privileges on test.t1 to 'test'@'%'identified by '123456' with grant option;
why with grant option? better remove it if it's not essential for drop to work. if it is essential, why is it?
Read the comment in the beginning of the test. This is to test that one doesn't need a separate privilge to be able to drop an inconsistent table.
My question was, why did you need "with grant option" and not just grant all privileges on test.t1 to 'test'@'%'identified by '123456'; I was quite eager to know how comes that "with grant option" is essential to this test.
+--echo #Test4: drop table can drop consistent table as well +create table t1(a int) engine=innodb; +drop table t1;
aha, so it works! :)
Orginal code was drop table followed by drop table force.
Sure, if you want to keep a test to ensure that drop table works, it's up to you, keep it :) If it were my commit, I'd removed it.
+# second drop with if exists will success +drop table if exists t1;
you've just tested it in Test7, haven't you?
Read again comment at the beginning of the test file.
Yes, but you don't need to keep duplicate tests, do you?
+# create Aria table t2 and rm .MAI and .MAD +create table t2(a int) engine=aria; +flush tables; +--remove_file $DATADIR/test/t2.MAD +--remove_file $DATADIR/test/t2.MAI +--list_files $DATADIR/test/ +--replace_result \\ / +drop table t2;
may be csv and archive tests too?
CSV has only one table file, so not that interesting as it's covered by other tests.
/* The file extension */ #define CSV_EXT ".CSV" // The data file #define CSN_EXT ".CSN" // Files used during repair and update #define CSM_EXT ".CSM" // Meta file
Archive has discovery and thus will automatically remove the .frm or .ARZ in case of missing files.
diff --git a/sql/handler.cc b/sql/handler.cc index 1af0157783e..5dd02d057c7 100644 --- a/sql/handler.cc +++ b/sql/handler.cc @@ -2674,26 +2712,34 @@ const char *get_canonical_filename(handler *file, const char *path, }
-/** delete a table in the engine +/** + Delete a table in the engine
+ @return 0 Table was deleted + @return -1 Table didn't exists, no error given + @return # Error from table handler + @note ENOENT and HA_ERR_NO_SUCH_TABLE are not considered errors. - The .frm file will be deleted only if we return 0. + The .frm file should be deleted by the caller only if we return <= 0. */ + int ha_delete_table(THD *thd, handlerton *table_type, const char *path, - const LEX_CSTRING *db, const LEX_CSTRING *alias, bool generate_warning) + const LEX_CSTRING *db, const LEX_CSTRING *alias, + bool generate_warning) { handler *file; char tmp_path[FN_REFLEN]; int error; TABLE dummy_table; TABLE_SHARE dummy_share; + bool is_error= thd->is_error(); DBUG_ENTER("ha_delete_table");
/* table_type is NULL in ALTER TABLE when renaming only .frm files */ if (table_type == NULL || table_type == view_pseudo_hton || ! (file=get_new_handler((TABLE_SHARE*)0, thd->mem_root, table_type))) - DBUG_RETURN(0); + DBUG_RETURN(-1);
"Table didn't exists, no error given"?
Yes (for this function)
if get_new_handler() fails it's OOM.
No problem with OOM as it sets thd->error() and the user will get that error message.
yes, but OOM it's not -1, it's ENOMEM, a hard error.
int handler::delete_table(const char *name) { - int saved_error= 0; - int error= 0; - int enoent_or_zero; + int saved_error= ENOENT; + bool abort_if_first_file_error= 1; + bool some_file_deleted= 0; + DBUG_ENTER("handler::delete_table");
+ // For discovery tables, it's ok if first file doesn't exists
why would that be ok?
For tables with discovery, there may not be any files on disk. They may be generated by the discovery process. That first file in a list is missing doesn't mean that there isn't other files.
Not really. First extension really means "file exists" even with discovery.
for (const char **ext= bas_ext(); *ext ; ext++) { + int error; - if (mysql_file_delete_with_symlink(key_file_misc, name, *ext, 0)) + if ((error= mysql_file_delete_with_symlink(key_file_misc, name, *ext, + MYF(0))))
why do you need this `error` variable, if you never use it?
Mostly for debugging. The compiler will delete it, so no big deal. I originally thought my_handler_delete_with_symlink() should return the errno, but as it can generate several errno that didn't work.
a compiler (depending on the version of flags) will also issue a warning that a variable is set, but not used. May be move it out of if()? int error= mysql_file_delete_with_symlink(...); if (error) that'll be ok for any compiler.
<cut>
/** + Return true if the error from drop table means that the + table didn't exists +*/ + +bool non_existing_table_error(int error) +{ + return (error == ENOENT || error == HA_ERR_NO_SUCH_TABLE || + error == HA_ERR_UNSUPPORTED || + error == ER_NO_SUCH_TABLE || + error == ER_NO_SUCH_TABLE_IN_ENGINE || + error == ER_WRONG_OBJECT); +}
this should, I believe, be inline in handler.h
After having to change this multiple times and have to recompile, I added this to the .c file. As this is only used for DDL's (which are slow), it's better to NOT have it in the head file as it's generates less code to have it here.
:) It doesn't generate less code, it's only used in one other file. After having to to change this multiple times ans being done with it you can still move it into handler.h.
<cut>
+static my_bool delete_table_force(THD *thd, plugin_ref plugin, void *arg) +{ + handlerton *hton = plugin_hton(plugin); + st_force_drop_table_params *param = (st_force_drop_table_params *)arg; + + /* + We have to ignore HEAP tables as these may not have been created yet + We also remove engines that is using discovery (as these will recrate + any missing .frm if needed) and tables marked with + HTON_AUTOMATIC_DELETE_TABLE as for these we can't check if the table + ever existed. + */ + if (!hton->discover_table && hton->db_type != DB_TYPE_HEAP && + !(hton->flags & HTON_AUTOMATIC_DELETE_TABLE)) + { + int error; + error= ha_delete_table(thd, hton, param->path, param->db, + param->alias, param->generate_warning);
This is doing a a lot of extra work. dummy_table/dummy_share, if()s. Allocating and constructing new handlers.
This is for extreme cases when there is no reason to optimize for performance but less code is better.
I suggest the following:
1. Introduce new handlerton method hton->drop_table(). By default it'll be
hton->get_new_handler() get_canonical_filename() file->ha_delete_table()
like now, so all engines will still work. But later we can start moving drop table functionality out of the handler. In this commit only engines like blackhole will have a dummy drop_table() method.
We already have two handler delete_table methods. Don't want to add yet another one.
Note that a several handlers needs special handling. (Just check who have HTON_AUTOMATIC_DELETE_TABLE), not just blackhole,
as a rule, Monty, you can assume that if I've reviewed a patch I've read every single line and checked every single comment. And after that I might ask questions or comment on something.
According to your suggestion, we would have to create a new drop_table() for all engines and have flags when to generate errors and which errors. And we would have to duplicate some of that code in ha_delete_table().
Definetly much more code than the current one and much more changes in many engines.
Not exactly. See my commit c1c2f6bb70f in bb-10.5-serg. It removes more code than it adds. The next commit adds drop_table implementations in some simple cases. But in total they still remove more code then they add. Long term there will be no handler::delete_table() at all, so that code duplication will go away completely.
2. Here you can do, basically
hton->drop_table(hton, param->path)
without checking for discovery, without a new flag HTON_AUTOMATIC_DELETE_TABLE. I'm not sure why you check for DB_TYPE_HEAP, could you please explain that?
See code comment! HEAP TABLES are created on open, so when we call delete_table_force() the table is not yet created. We can't detect that a not yet created table exists, can we ?
How is it different from, say, sequence engine? Sequence tables are also created on open.
+int ha_delete_table_force(THD *thd, const char *path, const LEX_CSTRING *db, + const LEX_CSTRING *alias, bool generate_warning) +{ + st_force_drop_table_params param; + Table_exists_error_handler no_such_table_handler; + DBUG_ENTER("ha_delete_table_force"); + + param.path= path; + param.db= db; + param.alias= alias; + param.generate_warning= generate_warning;
this is never used, you always invoke ha_delete_table_force() with generate_warning=0.
Yes, I know. This was in the original code and it was not used there either. However, I can imagine there are scenarios where this would be useful in the future, so I kept it. If you feel strongly about it I can delete the generate_warning options.
Yes, please, do. We have enough dead code as it is, no need to add more of it.
if (!dont_log_query && table_creation_was_logged) { /* + DROP TEMPORARY succeded. For the moment when we only come + here on success (error == 0)
DBUG_ASSERT(error == 0) please
Not needed as the code makes it clear if one looks up a couple of lines. The comment is more clear than an assert.
No, definitely not. Comments become obsolete and then it's worse than no comment at all. assert is basically a comment that ensures that it's always true. Please, add an assert.
@@ -2402,48 +2411,33 @@ int mysql_rm_table_no_locks(THD *thd, TABLE_LIST *tables, bool if_exists, path_length= build_table_filename(path, sizeof(path) - 1, db.str, alias.str, reg_ext, 0); } + DEBUG_SYNC(thd, "rm_table_no_locks_before_delete_table"); error= 0; - if (drop_temporary || - (ha_table_exists(thd, &db, &alias, &table_type, &is_sequence) == 0 && + if (drop_temporary) + { + /* "DROP TEMPORARY" but a temporary table was not found */ + error= ENOENT; + } + else if (((frm_exists= ha_table_exists(thd, &db, &alias, &table_type, + &is_sequence)) == 0 &&
this is an overkill. ha_table_exists() will do a full table discovery, you don't need it here, because you'll try to delete anyway.
This was in old code, nothing changed. I assume that we also need this for discovery as ha_delete_table() needs to know in which engine the table exists.
I know it's the old code. In the old code it was needed. In your new DROP logic it is not needed, that's why I think it's better not to have it.
+ if (Table_triggers_list::drop_all_triggers(thd, &db, + &table->table_name, + MYF(MY_WME | + MY_IGNORE_ENOENT))) + ferror= -1;
Why do you need two copies of code to drop triggers? Why cannot you do it once, on the first drop_all_triggers() place?
Because the first drop_all_triggers is only done if the .frm exists and the table was dropped. This is part of the "drop force", but only in the cases where it makes sence to do DROP FORCE:
I know. But you can also do the first (and only) drop_all_triggers() if .frm doesn't exist. I cannot imagine when it'd be wrong. Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
participants (2)
-
Michael Widenius
-
Sergei Golubchik