Hello, Monty! On Wed, Dec 3, 2025 at 1:38 PM Michael Widenius <[email protected]> wrote:
> 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. > I will leave this question for later, will go on with the rest, as I can answer them now. > > 2. MDL on the SELECT tables is not necessarily MDL_READ, and so > > table lock is. > > Sorry, I cannot parse the above. Please rephrase. > Table lock is TL_, for example TL_READ. The above means MDL on the SELECT tables is not necessarily MDL_READ, as mush as table lock is not necessarily TL_READ (f.ex. LOCK TABLES t WRITE). I'll clarify. > 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. > LOCK TABLES for GTT was implemented earlier. It's really hard to make it a no-op, and besides, is not too useful. It's useful as it's made now though. One example is backup vs schema migration for more than one table. Migration thread: LOCK TABLES t1 WRITE, t2 WRITE; ALTER TABLE t1 add t text; ALTER TABLE t2 add t text; UNLOCK TABLES; Backup thread: LOCK TABLES t1 READ, t2 READ; // mariabackup does it .... do a backup. For GTT, only DDL will be backed up. UNLOCK TABLES; Besides, ALTER TABLE under LOCK TABLES simply will not work (without a lot of effort) if the underlying table wasn't locked by LOCK TABLES. Same for the rest of DDL. > 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. > Source and sink is like TX and RX. https://en.wikipedia.org/wiki/Sources_and_sinks http://cs.emory.edu/~cheung/Courses/253/Syllabus/NetFlow/intro.html In CREATE OR REPLACE t SELECT * FROM s; `s ` depicts source, while the new version of t is sink. >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 how CREATE OR REPLACE should work in the example above if all the tables are GTT: 1. open parent (global) handle of the old table t 2. ensure there is no child handles open 3. create new table t (global/parent) 4. open it and put it in locked tables list 5. open CHILD (LOCAL) handle of s for read. 6. open CHILD (LOCAL) handle of new t for write. Tell me if that explains the confusion. 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! > Sure. There will be a bunch of commits joined into a main one (basically all that without MDEV in the caption). > 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 > If only the local handle in that list, then global handle will not be properly locked for CREATE OR REPLACE. Also that will mean we will have the local handle open. All my DDLs rely on that this never happens. > > Also fixes MDEV-37395 > > Please add the title for the MDEV > good idea, will do! > ------ > > > 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. > Well, I can imagine the case: if the application was used to emulate global temporary table with a real table and creating temporary table LIKE global one. CREATE TEMPORARY TABLE tmp like t1; CREATE OR REPLACE GLOBAL TEMPORARY TABLE t1 like tmp; // migrate to GTTs DROP TEMPORARY TABLE tmp; Now instances of an updated app may start working, while old instances may be terminated later. but anyway, replacing a normal table with GTT is not really problematic, so we can just have it. The problem was creating GTT and replacing GTT. It's good I can add independent tests for that, it will be easier to localize the problem, if any. 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. > Well, maybe there was some problem with making this work for views? for now it just seem like more (unimportant) work. > > 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); > How could you see this work??? https://dbfiddle.uk/Fo9JApfX -- it doesn't, at least in v21 What I implemented works in Oracle: https://dbfiddle.uk/O8KdF2kw > 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. > Which change? What is a proper error? 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. Means to drop two tables in one statement. If one drop fails, we'd have to rollback both. If needed, can be implemented whenever. > > ---- > 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. > We never discussed the terminology indeed. Should we use global/local, or parent/child? "global temporary share" is "global share of global temporary table"? I'd propose: global temporary table or GTT for what we implement local temporary table for usual temporary tables. child share -- connection-local share of GTT parent share -- global share of GTT What do you say? 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()); > } > will do. > ----- > > 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; > Why to complicate things? There is begin and commit for that. This allows a myisam temporary table be used with innodb statements. There is no notion of transaction for myisam. What if an innodb table would be locked? Extra rules only complicate things, for users as well. Users never think "unlock is commit". Maybe "on commit" case just shouldn't be supported for non-transactional tables, if it causes confusion. > 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. > Ok, whatever. > 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' > Ok, will make a change. > > Please also change this to provide a proper error message. > Timeout is not that informative. > To which one? > 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 > I tried before, and couldn't. Tables are opened separately in each case, and there is the right moment to do so. > > > 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 > Can you clarify the bug? I see no difference with before and after. > > +++ 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) > Yes, good idea. -- Yours truly, Nikita Malyavin
_______________________________________________ developers mailing list -- [email protected] To unsubscribe send an email to [email protected]
