Review of Global temporary tables, part 2 commit a9478ca44b44118cf70de493569d64afbb5cd925 Author: Nikita Malyavin <nikitamalyavin@gmail.com> Date: Fri Nov 22 02:15:56 2024 +0100 Fix deallocation of Sql_cmd_dml::result Made as a part of MDEV-34309 Sql_cmd_dml::result is using mem_root to control memory allocation, but Sql_alloc allows to mark the memory as freed via destructor. Normal runs have DTRACE disabled, so accesses in MYSQL_DML_DONE are turned into no-op, and that is the bug is not shown. Enabling DTRACE brings a crash yet in a bootstrap. The fix is to carefully set Sql_cmd_dml::result to nullptr whenever it is freed, or is marked as freed. Please include the title of MDEV-354309 in the commit description @@ -34253,6 +34253,7 @@ bool Sql_cmd_dml::prepare(THD *thd) MYSQL_DML_START(thd); + result= nullptr; // It could've been set since the last run. lex->context_analysis_only|= CONTEXT_ANALYSIS_ONLY_DERIVED; if (open_tables_for_query(thd, lex->query_tables, &table_count, 0, How +++ b/sql/sql_delete.cc @@ -1882,6 +1882,7 @@ bool Sql_cmd_delete::execute_inner(THD *thd) { select_result *sel_result= NULL; delete result; + result= nullptr; /* This is DELETE ... RETURNING. It will return output to the client */ if (thd->lex->analyze_stmt) { @@ -1929,10 +1930,7 @@ bool Sql_cmd_delete::execute_inner(THD *thd) } if (result) - { res= false; - delete result; - } Why remove the delete. If the delete may free wrong things, then the newly added result is also likely to free wrong things. Anyway, the code is clearly wrong here. One cannot set res to 0 if res is 1. This means we are ignoring a possible error from the upper level (probably OOM) and we are probably going to assert later in the code that checks if we can send ok. Try to remove the seteting of res= false and run mtr. Same logics goes for all changes in Sql_cmd_update::execute_inner(THD *thd diff --git a/sql/sql_update.cc b/sql/sql_update.cc if (unlikely(res)) + if (unlikely(res) && result) result->abort_result_set(); else { The above is wrong. Should be: + if (unlikely(res)) { if (result) result->abort_result_set(); } else { As otherwise you may call send_explain() when you shouldn't ------------------ Suggestion: - Move delete of result to unprepare. This will ensure that result is always 0 on Sql_cmd_dml::prepare() and excute_innert and you can replace delete result with 'DBUG_ASSERT(result == 0)' in most of your changes. - Remove setting res= 0 if we have a result and check with mtr. Conclusion: - As this is not cricial and you are going on vacation, I will take over this issue --------- commit 6a000835f5f58aba221ab89c120adae4be16abf3 Author: Nikita Malyavin <nikitamalyavin@gmail.com> Date: Thu Feb 20 16:12:37 2025 +0100 fix show create diff --git a/sql/sql_show.cc b/sql/sql_show.cc index da0081fd1f8..201aa8e3110 100644 --- a/sql/sql_show.cc +++ b/sql/sql_show.cc @@ -1413,8 +1413,13 @@ mysqld_show_create(THD *thd, TABLE_LIST *table_list) if (table_list->table && table_list->table->s->tmp_table != NO_TMP_TABLE) { TMP_TABLE_SHARE *share= (TMP_TABLE_SHARE*)table_list->table->s; - if (share->from_share && table_list->table->file->records() == 0) - thd->drop_temporary_table(table_list->table, NULL, true); + if (share->from_share) + { + if (table_list->table->file->info(HA_STATUS_VARIABLE) + || (table_list->table->file->records() == 0 + && thd->drop_temporary_table(table_list->table, NULL, true))) + error= TRUE; + } Please move || and && to previous lines so we have the test lined up nicely in the editor. This is how most of the MariaDB code is written. commit 55ff0fb18bbe4539db27ea73cc2b795e53521772 Author: Nikita Malyavin <nikitamalyavin@gmail.com> Date: Thu Feb 20 16:12:49 2025 +0100 add rpl assertions if (thd->temporary_tables - && thd->temporary_tables->global_temporary_tables_count) // TODO this may be buggy on slave + && thd->temporary_tables->global_temporary_tables_count) { move && up to previous line. @@ -6051,6 +6053,7 @@ my_bool open_global_temporary_table(THD *thd, TABLE_SHARE *source, if (thd->mdl_context.clone_ticket(&share->mdl_request)) return TRUE; table->reginfo.lock_type= TL_IGNORE; + share->table_creation_was_logged= 1; } The above means that we assume that slave have the same global temporary table, which is probably not the case. Better to set share->table_creation_was_logged=0 for the local temporary tables we are creating which means that any update of the table through the GTT will use row based replication. Please also add a function comment before my_bool open_global_temporary_table(THD *thd, TABLE_SHARE *source, TABLE_LIST *out_table, MDL_ticket *mdl_ticket) commit a64bbaf483ad714493db401f68049ce85a886c96 Author: Nikita Malyavin <nikitamalyavin@gmail.com> Date: Thu Feb 27 01:07:45 2025 +0100 CREATE: use frm header to store GTT bits diff --git a/sql/temporary_tables.cc b/sql/temporary_tables.cc index 45bca833d17..ea46c4035ba 100644 --- a/sql/temporary_tables.cc +++ b/sql/temporary_tables.cc @@ -1324,10 +1324,10 @@ int THD::commit_global_tmp_tables() All_share_tables_list::Iterator tables_it(share->all_tmp_tables); TABLE *table= tables_it++; DBUG_ASSERT(table); - if (!share->on_commit_delete) + if (!share->on_commit_delete()) table->file->info(HA_STATUS_VARIABLE); // update records() stat - if (share->on_commit_delete || table->file->records() == 0) + if (share->on_commit_delete() || table->file->records() == 0) error= drop_tmp_table_share(share, NULL, true); } I still think it would be better to not drop the temporary file based on file->records(). The reason is that some temporary table will be slow to drop and if there are users that is using these for many queries things will be slower than they need to be. - Not critical, can be discussed and changed later. commit 21f9ac1e56d6c81a431e5bdbc9b6b89c8b565ca2 Author: Nikita Malyavin <nikitamalyavin@gmail.com> Date: Thu Feb 27 01:12:34 2025 +0100 minor (typo, style) fixes -struct Closefrm_context +static my_bool open_gtt_on_error(TABLE *table) { - TABLE *table; - ~Closefrm_context() { closefrm(table); } -}; + closefrm(table); + return TRUE; +} Change return to void as this should never fail. Yes, it will require a few changes in callers, but it better to do return TRUE; in the caller than have the reviewer have to look at what another function is returning. +++ b/sql/temporary_tables.cc @@ -329,7 +329,9 @@ bool THD::internal_open_temporary_table(TABLE_LIST *tl, TABLE **table, { if (tmp_share->from_share && sql_command_flags() & (CF_ADMIN_COMMAND |CF_SCHEMA_CHANGE)) - DBUG_RETURN(false); + DBUG_RETURN(false); /* We want to use real global temporary table + when ALTER/DROP is executed. + */ Move the comment before the DBUG_RETURN() commit 9e1f5413f619bf4f88eef0f0a50fb7df4b8e7d44 Author: Nikita Malyavin <nikitamalyavin@gmail.com> Date: Sat Mar 1 01:27:04 2025 +0100 fix ALTER TABLE long wait @@ -11676,7 +11677,7 @@ do_continue:; */ if (alter_info->requested_lock != Alter_info::ALTER_TABLE_LOCK_EXCLUSIVE && thd->mdl_context.upgrade_shared_lock(mdl_ticket, MDL_SHARED_NO_WRITE, - lock_wait_timeout)) + thd->variables.lock_wait_timeout)) goto err_new_table_cleanup; Add a comment why we should not use lock_wait_timeout here. This is needed as we are using it a few lines above and it is important for coders to know why the value is not reused here. commit f7629d745fe729ea445688f5b710a97108da7c76 Author: Nikita Malyavin <nikitamalyavin@gmail.com> Date: Sat Mar 1 02:09:20 2025 +0100 Don't drop local data tables for ON COMMIT PRESERVE when records()==0 connect con1,localhost,root,,; +insert t values(1); +connection default; +alter table t add j int; +ERROR HY000: Lock wait timeout exceeded; try restarting transaction +set statement lock_wait_timeout=0 for drop table t; +ERROR HY000: Lock wait timeout exceeded; try restarting transaction We have to add better error messages for known cases. "Cannot alter global temporary tables as we have open instances of it" "Cannot alter global temporary tables as there are other users for it" Can be done after vacation. diff --git a/mysql-test/main/global_temporary_table.test b/mysql-test/main/global_temporary_table.test index 0edc6eee493..17195eaf3ea 100644 --- a/mysql-test/main/global_temporary_table.test +++ b/mysql-test/main/global_temporary_table.test +--} insert into t1 values(1, 'one'); + select * from t1; No reason for having an extra empty line here. This commit is using: bool THD::use_real_global_temporary_share() const { return (sql_command_flags() & (CF_ALTER_TABLE | CF_SCHEMA_CHANGE | CF_STATUS_COMMAND) && lex->sql_command != SQLCOM_CREATE_TABLE) || lex->sql_command == SQLCOM_CREATE_VIEW; } Please add a comment and move |, && and || to the end of lines. Please also fix the indentation. You can also add a new flag CF_USE_LOCAL_GTT table and mark those commands for which using GTT will open a local GTT copy. In sql_show.cc:get_all_tables(THD *thd, TABLE_LIST *tables, COND *cond) there is this code: if (thd->use_real_global_temporary_share()) continue; This looks wrong. We are ignoring all temporary tables if the above case is true. To ignore GTT ables we have to verify that the found share is a GTT table. Shouldn't we also test for tmp_share->from_share ? Why are we testing for use_real_global_temporary_share() in this code? This is called only be DML. Shouldn't that be safe for GTT tables? For what commands are thd->use_real_global_temporary_share() not true here? commit 718afd78dae4a37ed009ab349dfe9bca68fbc3d7 Author: Nikita Malyavin <nikitamalyavin@gmail.com> Date: Tue Mar 4 18:30:07 2025 +0100 Extract THD::has_open_global_temporary_tables() No comments commit cc35b9cb54e74a2f0fbbf4a2400599c7fddb750d Author: Nikita Malyavin <nikitamalyavin@gmail.com> Date: Tue Mar 4 21:01:07 2025 +0100 Add CF_ALTER_TABLE No comments commit 381243f4e75f61a31305c656f9b6be20cec042d2 Author: Nikita Malyavin <nikitamalyavin@gmail.com> Date: Wed Mar 5 00:18:42 2025 +0100 add global_temporary_tables plugin, move relevant code to temporary_tables.cc + MariaDB_PLUGIN_MATURITY_STABLE Should probably not be stable ;) Please change to BETA. We just have to ensure that this does not cause problems in Jenkin. If it does, you can change it to GAMMA or STABLE (depending on what Jenkins supports) commit a7fa6f835998bc33f1528420df5fe3f1b8a5dbfd Author: Nikita Malyavin <nikitamalyavin@gmail.com> Date: Wed Mar 5 00:50:50 2025 +0100 refactor drop_tmp_table_share + while ((tab= it++)) { + if (tab != table && tab->query_id != 0) + { + /* Found a table instance in use. This table cannot be dropped. */ + my_error(ER_CANT_REOPEN_TABLE, MYF(0), table->alias.c_ptr()); + result= true; + goto end; + } Suggested comment change: /* Found a table instance in use. This table cannot be dropped. */ -> /* Found a table instance in use (query_id != 0). This table cannot be dropped. */ ------- commit bb9ff1b2ec45b437bd39b6dacef8885fadeb818b Author: Nikita Malyavin <nikitamalyavin@gmail.com> Date: Wed Mar 5 02:51:23 2025 +0100 refactor THD::internal_open_temporary_table No comments commit 72914bfd92ae7390f8e7924944c92a1dc9591e1a Author: Nikita Malyavin <nikitamalyavin@gmail.com> Date: Wed Mar 5 22:06:02 2025 +0100 enforce limitations + if (create_info->table_options & HA_OPTION_GLOBAL_TEMPORARY_TABLE) + { + const char *error= NULL; + for (const Key &fk: create_info->alter_info->key_list) + if (fk.type == Key::FOREIGN_KEY) + { + error= "FOREIGN KEY"; + break; + } You can replace the above loop with: if (thd->lex->alter_info_flags & ALTER_ADD_FOREIGN_KEY) Note also that in your old code, later errors could overwrite earlier ones. Changing this to a normal if, like the others, will solve that. commit a2831f8e8efcfecd548b8ced6e19c3fc4d6966f9 Author: Nikita Malyavin <nikitamalyavin@gmail.com> Date: Thu Mar 6 20:55:00 2025 +0100 information_schema support mysql-test/main/global_temporary_table.result +GLOBAL TEMPORARY +show table status where temporary='Y'; +Name Engine Version Row_format Rows Avg_row_length Data_length Max_data_length Index_length Data_free Auto_increment Create_time Update_time Check_time Collation Checksum Create_options Comment Max_index_length Temporary Please add a test that show the output from information_schema.tables for all fields that are stable for mtr. By the way, why does the above show table status not show any data while +select TABLE_TYPE from information_schema.tables where table_name = 't1'; +TABLE_TYPE +TEMPORARY +GLOBAL TEMPORARY Show two rows just above show table status; At least 'TEMPORARY' should have 'Temporary' set to 'Y'' commit a580c6bc85791829c850d7eedc03d62a5ee99ffd Author: Nikita Malyavin <nikitamalyavin@gmail.com> Date: Thu Mar 6 23:43:38 2025 +0100 VIEW support Views already open the correct tables automatically, so it just works. Almost ;) + || lex->sql_command == SQLCOM_CREATE_VIEW ; .... commit d4c00419ebb39a07f87eabf0eee3660a768cbbed Author: Nikita Malyavin <nikitamalyavin@gmail.com> Date: Fri Mar 7 02:16:23 2025 +0100 AS SELECT support + if (table->s->tmp_table && + table->s->db_create_options & + (HA_OPTION_GLOBAL_TEMPORARY_TABLE|HA_OPTION_ON_COMMIT_DELETE_ROWS)) + table= NULL; // It will be deleted on commit + This will not work because of the following code: if (WSREP(thd) && table->file->ht->db_type == DB_TYPE_INNODB) You either have to add a goto or fix all references to 'table'. Note this code later in the same function. if (m_plock) { MYSQL_LOCK *lock= *m_plock; *m_plock= NULL; m_plock= NULL; if (create_info->pos_in_locked_tables) { /* If we are under lock tables, we have created a table that was originally locked. We should add back the lock to ensure that all tables in the thd->open_list are locked! */ table->mdl_ticket= create_info->mdl_ticket; This should be safe as long as on cannot use LOCK TABLES on gtt tables. Please add a check for that! Another thing to check is with GALERA that local gtt tables are not replicated. commit 45e2f14bcbdd61c1ea5712f5b79b07a096aa46d2 Author: Nikita Malyavin <nikitamalyavin@gmail.com> Date: Mon Mar 10 20:16:16 2025 +0100 CREATE TABLE ... LIKE support sql_table.cc: local_create_info.table_options&= ~HA_OPTIONS_TO_RESET_CREATE_LIKE; Looking at the code, I belive the above is always 0 here. Please replace with DBUG_ASSERT(local_create_info.table_options == 0); commit 1c44e4dabe22555a3b517db7f67c1e4d8b03fc70 Author: Nikita Malyavin <nikitamalyavin@gmail.com> Date: Mon Mar 10 21:49:11 2025 +0100 RENAME support +++ b/sql/sql_parse.cc @@ -6351,6 +6351,13 @@ check_rename_table(THD *thd, TABLE_LIST *first_table, check_grant(thd, INSERT_ACL | CREATE_ACL, &new_list, FALSE, 1, FALSE))) return 1; + + if (table->table && + table->table->s->db_create_options & HA_OPTION_GLOBAL_TEMPORARY_TABLE) + { + my_error(ER_LOCK_WAIT_TIMEOUT, MYF(0)); + return 1; + } We need a better error here. Something like: "Cannot rename a global temporary tables as this connection has open instances of it" commit b0f02644c22522c325bfbddd847030d61237b4e8 Author: Nikita Malyavin <nikitamalyavin@gmail.com> Date: Tue Mar 11 00:48:11 2025 +0100 FLUSH support, forbid LOCK FLUSH TABLE on Global temporary table is basically no-op, since it never has open instances. - It is ok to ignore flush table on GTT tables. There is no point in doing that. Same goes for temporary tables. +/** + Check if any global temporary tables are still opened in this session. +*/ +static int flush_check_for_global_temporary_tables(THD *thd, TABLE_LIST *tl) +{ + if (thd->rgi_slave || !thd->has_open_global_temporary_tables()) + return 0; + + for (;tl; tl= tl->next_global) + { + TABLE_SHARE *share= thd->find_tmp_table_share(tl); + if (share && share->db_create_options & HA_OPTION_GLOBAL_TEMPORARY_TABLE) + { + my_error(ER_LOCK_WAIT_TIMEOUT, MYF(0)); + return 1; + } + } + return 0; +} I think you can remove the above. +flush table t; +lock table t write; +ERROR 42S02: Unknown table 't' I think it is wrong that we get an err for the lock. We should fix it so that we open the table local GTT table and do not take a lock. (In other words, it should work as normal lock tables for tmp tables). @@ -5970,6 +5970,12 @@ my_bool open_global_temporary_table(THD *thd, TABLE_SHARE *source, { DBUG_ASSERT(!thd->rgi_slave); // slave won't use global temporary tables + if (thd->lex->sql_command == SQLCOM_LOCK_TABLES) + { + my_error(ER_BAD_TABLE_ERROR, MYF(0), source->table_name.str); + return TRUE; + } + See above. We should remove the above and instead open the table normally. Author: Nikita Malyavin <nikitamalyavin@gmail.com> Date: Tue Mar 11 02:08:10 2025 +0100 Replication support Please do not use 'show binlog events' as this will cause result conflicts with any changes in the binlog format and server changes. You have now things like Server ver: 11.8.0-MariaDB-debug-log, Binlog ver: 4 in the test output use instead source include/show_binlog_events.inc; One can also does not need to use ../.. to access main tests. Here is a diff for the test: +++ b/mysql-test/suite/binlog/t/global_temporary_table.test @@ -4,7 +4,7 @@ set @old_binlog_annotate_row_events= @@global.binlog_annotate_row_events; set global binlog_annotate_row_events= OFF; set session binlog_annotate_row_events= OFF; -source ../../main/global_temporary_table.test; +source main/global_temporary_table.test; +source include/show_binlog_events.inc; -show binlog events; set global binlog_annotate_row_events= @old_binlog_annotate_row_events; ----- +bool is_user_tmp_table(TMP_TABLE_SHARE *share) +{ + if (share->db_create_options & HA_OPTION_GLOBAL_TEMPORARY_TABLE) + return false; + return share->tmp_table == TRANSACTIONAL_TMP_TABLE || + share->tmp_table == NON_TRANSACTIONAL_TMP_TABLE; +} The normal way to test for temporary table is: share->tmp_table != NO_TMP_TABLE; see is_temporary_table() This should be safe also for the above function. Add () around the return functions, to get editors to indent it correctly by default. I think the above function needs some comments. Please explain how things are: if (share->db_create_options & HA_OPTION_GLOBAL_TEMPORARY_TABLE) is true, this is a GTT definition table and tmp_table would be NO_TMP_TABLE. If this is a local GTT tables, then (share->db_create_options & HA_OPTION_GLOBAL_TEMPORARY_TABLE) is false and share->from_share is true. It would be good if the above function would have good enough comments to explain all the above. We need to describe exactly how things are in some please in the source. Could you add the description how to detect GTT tables and local GTT tables and how they differ from normal temporary tables at the start of temporary_tables.cc Especially how share->db_create_options and share->tmp_table are set. If you add this function first in tempoary_table.cc, it may explain how things are: bool is_user_tmp_table(TMP_TABLE_SHARE *share) { if (share->db_create_options & HA_OPTION_GLOBAL_TEMPORARY_TABLE) { /* This is either a global temporary table (GTT) definition table or local GTT table created from the GTT definition. If (share->tmp_table != NO_TMP_TABLE) then this is local GTT table. In this case share->from_share is also set and points to the global GTT definition. */ return false; } /* Return true of the table is non GTT temporary table */ return (share->tmp_table != NO_TMP_TABLE) }