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