[MDEV-17805] Do not add temporary tables to dict_sys->table_hash Created: 2018-11-22  Updated: 2023-12-22

Status: Stalled
Project: MariaDB Server
Component/s: Data Definition - Temporary, Storage Engine - InnoDB
Fix Version/s: 11.5

Type: Task Priority: Major
Reporter: Marko Mäkelä Assignee: Sergei Golubchik
Resolution: Unresolved Votes: 1
Labels: performance, temporary

Attachments: File MDEV-17805-wip.patch    
Issue Links:
Blocks
is blocked by MDEV-17794 Do not assign persistent ID for tempo... Closed
is blocked by MDEV-25064 rpl.rpl_parallel_temptable failed in ... Closed
Relates
relates to MDEV-26152 Assertion: prebuilt->template_type ==... Confirmed
relates to MDEV-26433 assertion: table->get_ref_count() == ... Closed
relates to MDEV-12459 The information_schema tables for get... Closed

 Description   

InnoDB is adding both persistent and temporary tables to a dict_sys->table_hash, which maps table names to dict_table_t.

We should only add persistent tables to the hash table, and we should not store any dict_table_t::name for temporary tables. It would be nice to store the user-specified table name instead of some #sql name, but RENAME TABLE is not being propagated to temporary tables.

The reason why we currently need the table name lookup is that handler::delete_table() can be invoked on temporary tables without prior handler::open(), so the only information that is available is the table name.
If THD::rm_temporary_table() were invoked with TABLE_SHARE, we could store the dict_table_t* or dict_table_t::id there by invoking handler::set_ha_share_ptr() and handler::get_ha_share_ptr().

For persistent tables, we will need both name and ID based lookups for the time being. (InnoDB rollback and purge will look up tables by ID, and the SQL layer will have to look up persistent tables by name.)

As part of this task, please ensure that the table name printed in errors will be the real table name, not the temporary table name:
InnoDB: Cannot add field `NEW` in table `tmp`.`#sql-temptable-10e7-7aa21-13bb` because after adding it, the row size is 8204 which is greater than maximum allowed size (8126 bytes) for a record



 Comments   
Comment by Marko Mäkelä [ 2018-11-22 ]

svoj provided an untested MDEV-17805-wip.patch, which should provide TABLE_SHARE when dropping temporary tables. The InnoDB part remains to be written.

Comment by Marko Mäkelä [ 2018-11-23 ]

We should also remove the global dict_sys->temp_id_hash that was introduced in MDEV-17794, and instead iterate the THD::temporary_tables, using something similar to THD::find_tmp_table_share_w_base_key(). This should also remove any need to hold dict_sys->mutex or dict_operation_lock during operations with temporary tables.

Comment by Marko Mäkelä [ 2018-12-18 ]

I pushed an initial, non-working version of this to the 10.4-MDEV-17805 branch. svoj, could you help debugging it?

Comment by Sergey Vojtovich [ 2019-02-06 ]

serg, please review 5 top patches in bb-10.4-svoj-MDEV17805.
Starting: "Removed redundant SE lock for tmp tables"
Ending: "Get rid of rea_create_table()"

Comment by Sergei Golubchik [ 2023-04-20 ]

marko what are user visible effects of this change?

Comment by Marko Mäkelä [ 2023-08-21 ]

serg, there should be no user visible effect of fixing this performance bug, other than the improved performance thanks to the removal of some shared data structures.

I believe that the bug MDEV-10356 a.k.a. MDEV-25064 must be fixed before this simplification can be implemented. Currently, parallel replication seems to assume that temporary tables are accessible from any thread. That is conflicting with the InnoDB assumption that each temporary table is private to the connection that is was created in.

Generated at Thu Feb 08 08:39:15 UTC 2024 using Jira 8.20.16#820016-sha1:9d11dbea5f4be3d4cc21f03a88dd11d8c8687422.