Uploaded image for project: 'MariaDB Server'
  1. MariaDB Server
  2. MDEV-17805

Do not add temporary tables to dict_sys->table_hash

Details

    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

      Attachments

        Issue Links

          is blocked by

          Bug - A problem which impairs or prevents the functions of the product. MDEV-17794 Do not assign persistent ID for temporary tables

          • Major - Major loss of function.
          • Closed

          Bug - A problem which impairs or prevents the functions of the product. MDEV-25064 rpl.rpl_parallel_temptable failed in bb, ASAN heap-use-after-free in std::__atomic_base<long>::store

          • Critical - Crashes, loss of data, severe memory leak.
          • Closed
          relates to

          Bug - A problem which impairs or prevents the functions of the product. MDEV-26152 Assertion: prebuilt->template_type == ROW_MYSQL_WHOLE_ROW fails in row0mysql.cc line 1752

          • Major - Major loss of function.
          • Confirmed

          Bug - A problem which impairs or prevents the functions of the product. MDEV-26433 assertion: table->get_ref_count() == 0 in dict0dict.cc line 1915

          • Blocker - Blocks development and/or testing work, production could not run.
          • Closed

          Bug - A problem which impairs or prevents the functions of the product. MDEV-34049 InnoDB: Failing assertion: table->get_ref_count() == 0 in dict_sys_t::remove on DROP

          • Critical - Crashes, loss of data, severe memory leak.
          • Closed

          Bug - A problem which impairs or prevents the functions of the product. MDEV-35154 dict_sys_t::load_table() is holding exclusive dict_sys.latch for unnecessarily long time

          • Major - Major loss of function.
          • Confirmed

          Task - A task that needs to be done. MDEV-12459 The information_schema tables for getting temporary tables info is missing, at least for innodb there is no INNODB_TEMP_TABLE_INFO

          • Critical - Crashes, loss of data, severe memory leak.
          • Closed

          Bug - A problem which impairs or prevents the functions of the product. MDEV-34051 SIGSEGV in dict_table_t::is_active_ddl during XA ... INSERT IGNORE

          • Major - Major loss of function.
          • Confirmed

          Activity

            Ascending order - Click to sort in descending order
            marko Marko Mäkelä created issue -
            marko Marko Mäkelä made changes -
            Field Original Value New Value
            marko Marko Mäkelä made changes -
            Status Open [ 1 ] Confirmed [ 10101 ]
            marko Marko Mäkelä made changes -
            Attachment MDEV-17805-wip.patch [ 46734 ]
            marko Marko Mäkelä added a comment - - edited

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

            marko Marko Mäkelä added a comment - - edited svoj provided an untested MDEV-17805-wip.patch , which should provide TABLE_SHARE when dropping temporary tables. The InnoDB part remains to be written.
            marko Marko Mäkelä added a comment -

            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.

            marko Marko Mäkelä added a comment - 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.
            marko Marko Mäkelä made changes -
            Status Confirmed [ 10101 ] In Progress [ 3 ]
            marko Marko Mäkelä added a comment -

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

            marko Marko Mäkelä added a comment - I pushed an initial, non-working version of this to the 10.4-MDEV-17805 branch. svoj , could you help debugging it?
            svoj Sergey Vojtovich made changes -
            Assignee Marko Mäkelä [ marko ] Sergey Vojtovich [ svoj ]
            svoj Sergey Vojtovich added a comment -

            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()"

            svoj Sergey Vojtovich added a comment - 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()"
            svoj Sergey Vojtovich made changes -
            Assignee Sergey Vojtovich [ svoj ] Sergei Golubchik [ serg ]
            Status In Progress [ 3 ] In Review [ 10002 ]
            serg Sergei Golubchik made changes -
            Assignee Sergei Golubchik [ serg ] Sergey Vojtovich [ svoj ]
            Status In Review [ 10002 ] Stalled [ 10000 ]
            svoj Sergey Vojtovich made changes -
            Fix Version/s 10.5 [ 23123 ]
            Fix Version/s 10.4 [ 22408 ]
            julien.fritsch Julien Fritsch made changes -
            Fix Version/s 10.6 [ 24028 ]
            Fix Version/s 10.5 [ 23123 ]
            serg Sergei Golubchik made changes -
            Assignee Sergey Vojtovich [ svoj ] Marko Mäkelä [ marko ]
            marko Marko Mäkelä made changes -
            marko Marko Mäkelä made changes -
            serg Sergei Golubchik made changes -
            Assignee Marko Mäkelä [ marko ] Sergei Golubchik [ serg ]
            ralf.gebhardt Ralf Gebhardt made changes -
            Fix Version/s 10.6 [ 24028 ]
            serg Sergei Golubchik made changes -
            Fix Version/s 10.6 [ 24028 ]
            ralf.gebhardt Ralf Gebhardt made changes -
            Affects Version/s 10.2.2 [ 22013 ]
            Affects Version/s 10.3.0 [ 22127 ]
            Affects Version/s 10.4.0 [ 23115 ]
            Issue Type Bug [ 1 ] Task [ 3 ]
            ralf.gebhardt Ralf Gebhardt made changes -
            Fix Version/s 10.6 [ 24028 ]
            marko Marko Mäkelä made changes -
            marko Marko Mäkelä made changes -
            serg Sergei Golubchik made changes -
            Workflow MariaDB v3 [ 90830 ] MariaDB v4 [ 131707 ]
            marko Marko Mäkelä made changes -
            serg Sergei Golubchik added a comment -

            marko what are user visible effects of this change?

            serg Sergei Golubchik added a comment - marko what are user visible effects of this change?
            marko Marko Mäkelä added a comment -

            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.

            marko Marko Mäkelä added a comment - 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.
            julien.fritsch Julien Fritsch made changes -
            Fix Version/s 11.4 [ 29301 ]
            julien.fritsch Julien Fritsch made changes -
            Priority Major [ 3 ] Critical [ 2 ]
            julien.fritsch Julien Fritsch made changes -
            serg Sergei Golubchik made changes -
            Priority Critical [ 2 ] Major [ 3 ]
            monty Michael Widenius made changes -
            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.)             		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
            monty Michael Widenius made changes -
            Labels performance temporary CS0443108 performance temporary
            julien.fritsch Julien Fritsch made changes -
            Labels CS0443108 performance temporary performance temporary
            julien.fritsch Julien Fritsch made changes -
            Status Stalled [ 10000 ] Needs Feedback [ 10501 ]
            julien.fritsch Julien Fritsch made changes -
            Status Needs Feedback [ 10501 ] Open [ 1 ]
            julien.fritsch Julien Fritsch made changes -
            Status Open [ 1 ] In Progress [ 3 ]
            julien.fritsch Julien Fritsch made changes -
            Assignee Sergei Golubchik [ serg ] Julien Fritsch [ julien.fritsch ]
            julien.fritsch Julien Fritsch made changes -
            Status In Progress [ 3 ] Stalled [ 10000 ]
            julien.fritsch Julien Fritsch made changes -
            Assignee Julien Fritsch [ julien.fritsch ] Sergei Golubchik [ serg ]
            serg Sergei Golubchik made changes -
            Fix Version/s 11.5 [ 29506 ]
            Fix Version/s 11.4 [ 29301 ]
            serg Sergei Golubchik made changes -
            Fix Version/s 11.6 [ 29515 ]
            Fix Version/s 11.5 [ 29506 ]
            marko Marko Mäkelä made changes -
            Roel Roel Van de Paar made changes -
            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             		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:
            {noformat}
            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
            {noformat}
            ralf.gebhardt Ralf Gebhardt made changes -
            Fix Version/s 11.7 [ 29815 ]
            Fix Version/s 11.6 [ 29515 ]
            mariadb-jira-automation Jira Automation (IT) made changes -
            Zendesk Related Tickets 202463
            serg Sergei Golubchik made changes -
            Fix Version/s 11.8 [ 29921 ]
            Fix Version/s 11.7 [ 29815 ]
            marko Marko Mäkelä made changes -
            marko Marko Mäkelä made changes -
            serg Sergei Golubchik made changes -
            Fix Version/s 11.9 [ 29945 ]
            Fix Version/s 11.8 [ 29921 ]
            julien.fritsch Julien Fritsch made changes -
            Priority Major [ 3 ] Minor [ 4 ]
            serg Sergei Golubchik made changes -
            Assignee Sergei Golubchik [ serg ] Marko Mäkelä [ marko ]
            serg Sergei Golubchik made changes -
            Priority Minor [ 4 ] Critical [ 2 ]
            marko Marko Mäkelä added a comment -

            serg, ideally, the storage engine needs to be able to attach its own table handle to handler or TABLE_SHARE, so that the table does not need to be looked up by a name in ha_innobase::delete_table().

            Recently, in MDEV-35000, I realized that some additional refactoring around Handler_share and Partition_share appears to be necessary.

            marko Marko Mäkelä added a comment - serg , ideally, the storage engine needs to be able to attach its own table handle to handler or TABLE_SHARE , so that the table does not need to be looked up by a name in ha_innobase::delete_table() . Recently, in MDEV-35000 , I realized that some additional refactoring around Handler_share and Partition_share appears to be necessary.
            marko Marko Mäkelä made changes -
            Assignee Marko Mäkelä [ marko ] Sergei Golubchik [ serg ]
            serg Sergei Golubchik made changes -
            Fix Version/s 12.1 [ 29992 ]
            Fix Version/s 12.0 [ 29945 ]

            People

              serg Sergei Golubchik
              marko Marko Mäkelä
              Votes:
              1 Vote for this issue
              Watchers:
              11 Start watching this issue

              Dates

                Created:
                Updated:

                Git Integration

                  Error rendering 'com.xiplink.jira.git.jira_git_plugin:git-issue-webpanel'. Please contact your Jira administrators.