Details

    Description

      Add TABLE_SHARE::foreign_keys and TABLE_SHARE::referenced_keys as pointers to List<FOREIGN_KEY_INFO>.

      Remove handler foreign key interface: get_foreign_key_list(), get_parent_foreign_key_list(), referenced_by_foreign_key().

      Remove cached TABLE_SHARE objects of referenced tables when foreign table is dropped, renamed or altered (add foreign key, rename column of foreign key).

      Remove cached TABLE_SHARE objects of foreign tables when referenced table is renamed or altered (rename column of referenced key).

      Debug build: check consistency between opened foreign and referenced tables on table flush.

      Store foreign keys into FRM files

      1. Introduce Foreign_key_io class which creates/parses binary stream containing foreign key structures. Referenced tables store there only hints about foreign tables (their db and name), they restore full info from the corresponding tables.

      Foreign_key_io is stored under new EXTRA2_FOREIGN_KEY_INFO field in extra2 section of FRM file.

      2. Modify mysql_prepare_create_table() to generate names for foreign keys. Until InnoDB storage of foreign keys is removed, FK names must be unique across the database: the FK name must be based on table name.

      3. Keep stored data in sync on DDL changes. Referenced tables update their foreign hints after following operations on foreign tables:

      RENAME TABLE
      DROP TABLE
      CREATE TABLE
      ADD FOREIGN KEY
      DROP FOREIGN KEY
      Foreign tables update their foreign info after following operations on referenced tables:

      RENAME TABLE
      RENAME COLUMN

      4. To achieve 3. there must be ability to rewrite extra2 section of FRM file without full reparse. FRM binary is built from primary structures like HA_CREATE_INFO and cannot be built from TABLE_SHARE.

      Use shadow write and rename like fast_alter_partition_table() does.

      Converge Foreign_key and supplemental generated Key together

      mysql_prepare_create_table() does data validation and such utilities as automatic name generation. But it does that only for indexes and ignores Foreign_key objects. Now as Foreign_key data needs to be stored in FRM files as well this processing must be done for them like for any other Key objects.

      Replace Key::FOREIGN_KEY type with Key::foreign flag of type Key::MULTIPLE and Key::generated set to true. Construct one object with Key::foreign == true instead of two objects of type Key::FOREIGN_KEY and Key::MULTIPLE.

      Attachments

        Issue Links

          Activity

            Do I understand it correctly that this implementation is increasing the memory footprint when FOREIGN KEY constraints exist? If the metadata storage was moved to a higher layer, then the lower-level storage (such as dict_foreign_t) would have to be removed. Apart from increased memory usage, duplicated data structures would create a potential problem that the two data dictionary caches become inconsistent with each other.

            marko Marko Mäkelä added a comment - Do I understand it correctly that this implementation is increasing the memory footprint when FOREIGN KEY constraints exist? If the metadata storage was moved to a higher layer, then the lower-level storage (such as dict_foreign_t ) would have to be removed . Apart from increased memory usage, duplicated data structures would create a potential problem that the two data dictionary caches become inconsistent with each other.

            The current Description says what is being done but not why it needs to be done. I can imagine one possible motivation: making the implementation of MDEV-21051 easier. (It is currently marked as a duplicate of this task; I think that it might be more appropriate to say that it is blocked by this task.)

            I think that before this can be included in a release, we also need performance testing, which should include not only execution time but also the memory usage, including any fragmentation. We have dismissed such issues as "not a bug" in the past (for example, in MDEV-14050), but I do not think that it is acceptable to increase the memory usage or the amount of allocation anti-patterns.

            marko Marko Mäkelä added a comment - The current Description says what is being done but not why it needs to be done. I can imagine one possible motivation: making the implementation of MDEV-21051 easier. (It is currently marked as a duplicate of this task; I think that it might be more appropriate to say that it is blocked by this task.) I think that before this can be included in a release, we also need performance testing, which should include not only execution time but also the memory usage, including any fragmentation. We have dismissed such issues as "not a bug" in the past (for example, in MDEV-14050 ), but I do not think that it is acceptable to increase the memory usage or the amount of allocation anti-patterns.

            It is always percentage of overhead which is acceptable or not. 10% total increase is unacceptable. 0.5% is acceptable. What is the threshold between these two numbers is the subject of consideration. I think it should be 2% which is far sufficient for the task IMO.

            This subtask should not be merged alone due to MDEV-33741 and it seems to depend on the last subtask (crash safety), so it turns out the whole MDEV-16417 should be pushed at once. I'm currently updating MDEV-21053.

            midenok Aleksey Midenkov added a comment - It is always percentage of overhead which is acceptable or not. 10% total increase is unacceptable. 0.5% is acceptable. What is the threshold between these two numbers is the subject of consideration. I think it should be 2% which is far sufficient for the task IMO. This subtask should not be merged alone due to MDEV-33741 and it seems to depend on the last subtask (crash safety), so it turns out the whole MDEV-16417 should be pushed at once. I'm currently updating MDEV-21053 .

            I was asked to review MDEV-21652 in a branch that includes many other changes, including a version of this MDEV-20865, as well as MDEV-21052 which I have not been asked to review.

            I think that MDEV-21052 and MDEV-20865 needs to be in one atomic changeset, because InnoDB currently is the only storage engine that handles FOREIGN KEY metadata. Doing that would address my 2024-03-05 comment.

            The branch currently bundles too many changes that have a similar topic but should be fixable in isolation.

            I think that the changes to memory cache data structures (including MDEV-20865, MDEV-21052) need to be thoroughly tested and reviewed first, in a way that does not involve any file format changes. My point is that we may store FOREIGN KEY metadata in TABLE_SHARE, by populating the data structure both from a .frm file and the InnoDB SYS_FOREIGN and SYS_FOREIGN_COLS tables.

            marko Marko Mäkelä added a comment - I was asked to review MDEV-21652 in a branch that includes many other changes, including a version of this MDEV-20865 , as well as MDEV-21052 which I have not been asked to review. I think that MDEV-21052 and MDEV-20865 needs to be in one atomic changeset, because InnoDB currently is the only storage engine that handles FOREIGN KEY metadata. Doing that would address my 2024-03-05 comment. The branch currently bundles too many changes that have a similar topic but should be fixable in isolation. I think that the changes to memory cache data structures (including MDEV-20865 , MDEV-21052 ) need to be thoroughly tested and reviewed first, in a way that does not involve any file format changes. My point is that we may store FOREIGN KEY metadata in TABLE_SHARE , by populating the data structure both from a .frm file and the InnoDB SYS_FOREIGN and SYS_FOREIGN_COLS tables.

            As far as I understand, implementing this would remove a significant amount of contention on dict_sys.latch, with the following kind of stack traces:

            ssux_lock_impl<false>::rd_wait()
            ha_innobase::referenced_by_foreign_key()
            prepare_fk_prelocking_list(THD*, Query_tables_list*, TABLE_LIST*, bool*, unsigned char)
            DML_prelocking_strategy::handle_table(THD*, Query_tables_list*, TABLE_LIST*, bool*)
            extend_table_list(THD*, TABLE_LIST*, Prelocking_strategy*, bool)
            open_tables(THD*, DDL_options_st const&, TABLE_LIST**, unsigned int*, unsigned int, Prelocking_strategy*)
            open_and_lock_tables(THD*, DDL_options_st const&, TABLE_LIST*, bool, unsigned int, Prelocking_strategy*)
            

            Note that this affects any workloads, including those that do not use any FOREIGN KEY constraints at all.

            marko Marko Mäkelä added a comment - As far as I understand, implementing this would remove a significant amount of contention on dict_sys.latch , with the following kind of stack traces: ssux_lock_impl<false>::rd_wait() ha_innobase::referenced_by_foreign_key() prepare_fk_prelocking_list(THD*, Query_tables_list*, TABLE_LIST*, bool*, unsigned char) DML_prelocking_strategy::handle_table(THD*, Query_tables_list*, TABLE_LIST*, bool*) extend_table_list(THD*, TABLE_LIST*, Prelocking_strategy*, bool) open_tables(THD*, DDL_options_st const&, TABLE_LIST**, unsigned int*, unsigned int, Prelocking_strategy*) open_and_lock_tables(THD*, DDL_options_st const&, TABLE_LIST*, bool, unsigned int, Prelocking_strategy*) Note that this affects any workloads, including those that do not use any FOREIGN KEY constraints at all.

            People

              serg Sergei Golubchik
              midenok Aleksey Midenkov
              Votes:
              1 Vote for this issue
              Watchers:
              9 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.