Hi! Review of MDEV-37382 crash on CREATE OR REPLACE GTT under LOCK TABLES. This is part of MDEV-35915 Implement Global temporary tables
Patches reviewed: https://github.com/MariaDB/server/commit/95e16641a56c (first patch) b27b8a518d950e1c7121837b12f8c0aa0aa0db8d > * CREATE ... SELECT t means that local handle of t should be opened; > * LOCK TABLES adds an extra complexity layer: > 1. It re-opens the tables a > few times, not in a single operation (so we couldn't rely on the > order of table_list), Isn't there not only one re-open ? - In case of lock tables the original table is already open. If not, it is opened. - First the new table is created and opened (it is not locked as it's a temporary) - Then we copy data from the old one to the new - Then we close the new. - Then we close the old one, unlock it and delete/rename it - Then rename the new one to the old name - Then we open the old one and lock it in place of the original table. As far as I can see, there is only one unlock and one re-lock. Please clarify. > 2. MDL on the SELECT tables is not necessarily MDL_READ, and so > table lock is. Sorry, I cannot parse the above. Please rephrase. > 3. CREATE OR REPLACE needs to open a global handle to substitute old > table in locked_tables_list With GTT, we should only lock the local handle, not the global one! A LOCK TABLE on a GTT table should not interfere with LOCK TABLE for other threads. The definition is global, the data is local. See also other comments about LOCK TABLE later. > 4. CREATE...SELECT additionally needs to open a local handle to > insert into a correct sink. Not clear what you mean with 'sink'. Please replace with new table, temporary table, original table etc. >From previous commit to solve the same thing: > a) CREATE OR REPLACE requires a careful decision on whether to open a > parent (global) TABLE handle, or a child (local). > It should choose a child when AS SELECT is used, for both source and > sink. > b) It also shouldn't end up in creating a new child, > if the table to replace is opened (in MDL_SHARED_UPGRADEABLE mode). I don't understand the problem Especially "It should choose a child when AS SELECT is used, for both source and sink" is unclear and probably wrong (depending on the meaning of sink). Here is how things should work: create or replace table t1 will always work with the real table, never with a temporary table. create or replace temporary table t1 should work on the local temporary table, never on the global one. create or replace global temporary table should work on the global one. create or replace ... SELECT.. ; Here the tables in SELECT part should follow the normal rules. First use local temporary tables, then global ones. Here is a test how things works now: create or replace table t1 (a int); create temporary table t1 (b int); create or replace table t1 (c int); show create table t1; | t1 | CREATE TEMPORARY TABLE `t1` ( `b` int(11) DEFAULT NULL ) ENGINE=Aria DEFAULT CHARSET=latin1 COLLATE=latin1_swedish_ci PAGE_CHECKSUM=0 | drop table t1; show create table t1; | t1 | CREATE TABLE `t1` ( `c` int(11) DEFAULT NULL ) ENGINE=Aria DEFAULT CHARSET=latin1 COLLATE=latin1_swedish_ci PAGE_CHECKSUM=1 In other words, create part (sink?) will always be the real table. Please also update MDEV-35915 "Implement Global temporary tables" in how LOCK TABLES should work with GTT tables. See comments about this later in this email! Back to newer patch b27b8a518d950e1c7121837b12f8c0aa0aa0db8d > * In CREATE...SELECT, open both global and local handles: > the former is for replacing the old table in locked_tables_list and > the latter is for inserting the data. The above sounds wrong. I would have assumed that we only have the local handle in the locked table list. See later comments > Also fixes MDEV-37395 Please add the title for the MDEV ------ > create or replace table t(x int); > create or replace global temporary table t(t text); > - ERROR HY000: Can't create table `test`.`t` (errno: 0 "Internal error/check > (Not system error)") I am not sure it is correct the create or replace should replace a normal table. I understand the logic (as we internally implement global temporary tables as normal tables), but this is not expected. It would be better if create or replace global temporary table t(t text) would give an error that t exists and is type 'table'. This is what we do with views: create view v1 as select 1; create or replace table v1 as select 1; ERROR 1965 (42S02): 'test.v1' is a view create or replace table t1 (a int); create or replace view t1 as select 1; ERROR 1347 (HY000): 'test.t1' is not of type 'VIEW' Please implement simlar logic when doing create or replace global... I checked this with Serg and he agrees that create or replace global temporary should not drop a normal table. The same as create or replace temporary table will not drop a normal table. > create global temporary table t(x int) on commit preserve rows; Note that oracle uses the syntax: create global temporary table t on commit preserve rows (x int); > create or replace table t(y int); > ERROR HY000: Lock wait timeout exceeded; try restarting transaction With the above suggested change, the above will give a proper error, not a lock timeout which is confusing. Note that 'create global temporary table t on commit preserve rows (x int)' here should replace the global temporary table with the new empty one, even if it has data. > drop table t; Please also add support for DROP GLOBAL TEMPORARY TABLE, which will give an error if the table is not a global temporary table. For legacy reasons DROP global_temporary_table should work. > create or replace global temporary table t (d int); > ERROR HY000: Lock wait timeout exceeded; try restarting transaction Should not give an error (as this is the same connection that has data in the table) ---- sql/sql_base.cc if (best_table->s->table_type == TABLE_TYPE_GLOBAL_TEMPORARY && - !thd->use_real_global_temporary_share()) + !thd->use_real_global_temporary_share(table_list)) Why do we have 'real' in the name here. Better to use just hd->use_global_temporary_share(table_list) as real does not add any usefull information. Please also fix indentation for THD::use_real_global_temporary_share(const TABLE_LIST *table) all || should be at the end of the line and indentation should be done properly. like: bool THD::use_global_temporary_share(const TABLE_LIST *table) const { return (table->open_strategy == TABLE_LIST::OPEN_STUB || table->open_strategy == TABLE_LIST::OPEN_FOR_LOCKED_TABLES_LIST || (sql_command_flags() & (CF_ALTER_TABLE | CF_SCHEMA_CHANGE | CF_STATUS_COMMAND) && lex->sql_command != SQLCOM_CREATE_TABLE) || lex->sql_command == SQLCOM_CREATE_VIEW || lex->sql_command == SQLCOM_TRUNCATE || lex->sql_command == SQLCOM_LOCK_TABLES || stmt_arena->is_stmt_prepare()); } ----- If we store the local table in the locked list, we do not need the following: + dst_table_list->open_strategy= TABLE_LIST::OPEN_FOR_LOCKED_TABLES_LIST; sql_insert.cc: + if (!table_list->mdl_request.ticket) + table_list->mdl_request.ticket= create_info->mdl_ticket; + DBUG_ASSERT(table_list->mdl_request.ticket); + + if (create_info->pos_in_locked_tables && create_info->global_tmp_table()) + { + /* + Also pre-open a parent GTT handle to replace a + locked_tables_list item later in select_create::send_eof() + */ + TABLE_LIST *parent_gtt= thd->alloc<TABLE_LIST>(1); + new(parent_gtt) TABLE_LIST(&table_list->db, &table_list->table_name, + &table_list->alias, table_list->lock_type); + // Will open the parent table + parent_gtt->open_strategy= TABLE_LIST::OPEN_FOR_LOCKED_TABLES_LIST; + parent_gtt->prev_global= &table_list; + // Tables from SELECT are stored in select_create::select_tables + DBUG_ASSERT(table_list->next_global == NULL); + table_list->next_global= parent_gtt; + open_table(thd, parent_gtt, &ot_ctx); + } I think the above is wrong (assuming we should keep the local table in the lock table list. If we do that, then we should not need any special code for handling global temporary tables here. Note that with LOCK TABLES, the table is only release on unlock tables. This means, that the following should work: create global temporary table t1 (a int) on commit delete rows; lock tables t1 write; insert into t1 values (1); select * from t1; The above returns 0 rows while it should return 1 row. This is important as myisam and aria (for not transactional tables) does not support transactions. For these (and in part of other tables), the transaction boundary is lock tables ... unlock tables; In other words, we should define "on commit delete row" to mean: - In not locked mode on commit. In locked mode, on unlock tables. This is critical as otherwise global temporary tables will not work for MySIAM or some Aria tables (as there is no way to do a transaction without lock tables). This means that the rest of the code in sql_insert.cc is not relevant for this review. sql/sql_table.cc +static +my_bool +check_global_temporary_tables_for_create(THD *thd, + const HA_CREATE_INFO *create_info, + const DDL_options_st options, + const Lex_ident_db &db, + const Lex_ident_table &table_name) Please replace options with a pointer. THis is safer as someone may later add new elements into it. I would prefer to have &db and &table_name as pointers, but this is a bigger change because of find_temporary_table :( > TABLE *t= create_info->table; > if ((t && t->s->tmp_table && t->s->global_tmp_table()) || > thd->find_temporary_table(db, table_name, THD::TMP_TABLE_ANY, > Tmp_table_kind::GLOBAL)) > { > my_error(ER_LOCK_WAIT_TIMEOUT, MYF(0)); > return TRUE; > } Please do not use one character identifiers. Use 'table' instead of 't' Please also change this to provide a proper error message. Timeout is not that informative. sql/sql_table.cc You added check_global_temporary_tables_for_create in 3 places. Not a big deal, but I wonder if we could just do it once in mysql_create_table_no_lock() as all functions calls this one. - For mysql_create_table() it should be fine - For mysql_create_like_table it should be fine. - For Sql_cmd_create_table_like::execute() things are a bit more complex. However, Sql_cmd_create_table_like::execute() has a small bug: if (check_global_temporary_tables_for_create() call is done before this code: /* Ensure we don't try to create something from which we select from */ if (create_info.or_replace() && !create_info.tmp_table()) While in mysql_create_like_table() it is done after the above check. Please move check_global_temporary_tables_for_create() in Sql_cmd_create_table_like::execute() down after the test and remove the double newlines +++ b/sql/table.h @@ -2916,7 +2916,8 @@ struct TABLE_LIST /* Associate a table share only if the the table exists. */ OPEN_IF_EXISTS, /* Don't associate a table share. */ - OPEN_STUB + OPEN_STUB, + OPEN_FOR_LOCKED_TABLES_LIST, Add documentation for OPEN_FOR_LOCKED_TABLES_LIST (or remove it as we may not need it) Regards, Monty _______________________________________________ commits mailing list -- [email protected] To unsubscribe send an email to [email protected]
