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

Obsolete internal parser for FK in InnoDB

Details

    Description

      Currently InnoDB uses internal parser for adding foreign keys. Remove internal parser and use data parsed by SQL parser (sql_yacc) for adding foreign keys. This allows to pass some additional data from SQL layer like whether the referenced table is SYSTEM_TIME partitioned (MDEV-19191) and is required for further improvements (MDEV-16417, MDEV-10393, MDEV-12483).

      Attachments

        Issue Links

          Activity

            I think that as part of this, we should fix the SQL parser (both sql_yacc.yy and sql_yacc_ora.yy) so that also the following form of REFERENCES constraints will be recognized and not ignored by the parser:

            column_def:
                      field_spec
                      { $$= $1; }
                    | field_spec references
                      { $$= $1; }
                    ;
            

            Currently, the grammar rule is ignoring the semantic value of the REFERENCES clause. The non-terminal references is being more correctly used in the key_def rule. Also, I wonder whether the second production should include opt_constraint:

            column_def:
                      field_spec
                      { $$= $1; }
                    | field_spec opt_constraint references
                      { $$= … $1 … $2 … $3 …; }
                    ;
            

            marko Marko Mäkelä added a comment - I think that as part of this, we should fix the SQL parser (both sql_yacc.yy and sql_yacc_ora.yy ) so that also the following form of REFERENCES constraints will be recognized and not ignored by the parser: column_def: field_spec { $$= $1; } | field_spec references { $$= $1; } ; Currently, the grammar rule is ignoring the semantic value of the REFERENCES clause. The non-terminal references is being more correctly used in the key_def rule. Also, I wonder whether the second production should include opt_constraint : column_def: field_spec { $$= $1; } | field_spec opt_constraint references { $$= … $1 … $2 … $3 …; } ;

            Made as separate MDEV-20729.

            midenok Aleksey Midenkov added a comment - Made as separate MDEV-20729 .

            bar thinks and I agree that the FOREIGN KEY metadata would better be stored in TABLE_SHARE. Sure, it would still not be stored anywhere outside InnoDB (until MDEV-16417 is implemented), but TABLE_SHARE would be the logical place for it.

            Note: Until MDEV-16417, we will need some interface that allows the storage engine to fill in the FOREIGN KEY metadata. That might be some kind of an extension to handler::open(). This is a bit tricky, because there could be multiple concurrent calls to that function for the same table, while at the same time there was no prior call of handler::open() for that table. Inside ha_innobase::open() there is some distinction of ‘first-time open’ and ‘reopen’. If the ‘first-time open’ would initialize the FOREIGN KEY metadata in TABLE_SHARE, we would need some locking to make sure that the concurrent calls that would reach the ‘reopen’ code path will not return before that metadata has been initialized.

            I sent some lower-level review comments on GitHub.

            Please rebase the work on the latest 10.5 and remember to update the commit date when updating the patch.

            marko Marko Mäkelä added a comment - bar thinks and I agree that the FOREIGN KEY metadata would better be stored in TABLE_SHARE . Sure, it would still not be stored anywhere outside InnoDB (until MDEV-16417 is implemented), but TABLE_SHARE would be the logical place for it. Note: Until MDEV-16417 , we will need some interface that allows the storage engine to fill in the FOREIGN KEY metadata. That might be some kind of an extension to handler::open() . This is a bit tricky, because there could be multiple concurrent calls to that function for the same table, while at the same time there was no prior call of handler::open() for that table. Inside ha_innobase::open() there is some distinction of ‘first-time open’ and ‘reopen’. If the ‘first-time open’ would initialize the FOREIGN KEY metadata in TABLE_SHARE , we would need some locking to make sure that the concurrent calls that would reach the ‘reopen’ code path will not return before that metadata has been initialized. I sent some lower-level review comments on GitHub. Please rebase the work on the latest 10.5 and remember to update the commit date when updating the patch.
            midenok Aleksey Midenkov added a comment - - edited

            Refactor InnoDB to use foreign keys from TABLE_SHARE is large subtask of MDEV-16417. There are at least 16 functions that need to be remade or refactored including ~35 occurrences of `SYS_FOREIGN` and ~27 occurrences of `SYS_FOREIGN_COLS` in InnoDB SQL code.

            What is possible in scope of this task is to pass via `TABLE_SHARE` foreign keys *only* for `create_table_info_t::create_table()`. There is no much importance in that for now as there is raw data in `alter_info` and there is processed data in `TABLE_SHARE` for `create_table_info_t::create_table()`, so we just switch from one to another. And there is no final design of foreign keys structure in `TABLE_SHARE` until MDEV-16417, so IMO passing them now via `TABLE_SHARE` is a bit premature.

            midenok Aleksey Midenkov added a comment - - edited Refactor InnoDB to use foreign keys from TABLE_SHARE is large subtask of MDEV-16417 . There are at least 16 functions that need to be remade or refactored including ~35 occurrences of `SYS_FOREIGN` and ~27 occurrences of `SYS_FOREIGN_COLS` in InnoDB SQL code. What is possible in scope of this task is to pass via `TABLE_SHARE` foreign keys * only * for `create_table_info_t::create_table()`. There is no much importance in that for now as there is raw data in `alter_info` and there is processed data in `TABLE_SHARE` for `create_table_info_t::create_table()`, so we just switch from one to another. And there is no final design of foreign keys structure in `TABLE_SHARE` until MDEV-16417 , so IMO passing them now via `TABLE_SHARE` is a bit premature.

            midenok, you mean, refactor InnoDB to use foreign keys from TABLE_SHARE? I don't think this is particularly important. InnoDB has dict_table_t and dict_col_t and dict_index_t despite the fact that table metadata, and all columns and indexes are stored in the TABLE_SHARE too.

            But the server does not use InnoDB data structures, the server uses what's in the TABLE_SHARE. Only for foreign keys the server asks InnoDB for FK information. Instead it should store all FK metadata in the TABLE_SHARE and use them from there.

            While an engine can still store them internally and not peek into TABLE_SHARE every time.

            serg Sergei Golubchik added a comment - midenok , you mean, refactor InnoDB to use foreign keys from TABLE_SHARE ? I don't think this is particularly important. InnoDB has dict_table_t and dict_col_t and dict_index_t despite the fact that table metadata, and all columns and indexes are stored in the TABLE_SHARE too. But the server does not use InnoDB data structures, the server uses what's in the TABLE_SHARE . Only for foreign keys the server asks InnoDB for FK information. Instead it should store all FK metadata in the TABLE_SHARE and use them from there. While an engine can still store them internally and not peek into TABLE_SHARE every time.
            midenok Aleksey Midenkov added a comment - - edited

            This additional subtask requires several days to finish as it is required to keep in sync referenced TABLE_SHARE objects (TABLE_SHARE::referenced_keys list) after DROP TABLE, ALTER ADD/DROP FOREIGN KEY, RENAME TABLE. It was expected to be a subtask of MDEV-16417, but I already started it here and implemented a draft for RENAME TABLE: https://github.com/MariaDB/server/commit/abcce38a623d86bdd9e27cfd0b04ae9698cfef0b

            And the above fix should be remade for avoiding TABLE_SHARE invalidation.

            MDEV-20865 will track further progress.

            P.S. Actually this big enough subtask took more than several days.

            midenok Aleksey Midenkov added a comment - - edited This additional subtask requires several days to finish as it is required to keep in sync referenced TABLE_SHARE objects (TABLE_SHARE::referenced_keys list) after DROP TABLE, ALTER ADD/DROP FOREIGN KEY, RENAME TABLE. It was expected to be a subtask of MDEV-16417 , but I already started it here and implemented a draft for RENAME TABLE: https://github.com/MariaDB/server/commit/abcce38a623d86bdd9e27cfd0b04ae9698cfef0b And the above fix should be remade for avoiding TABLE_SHARE invalidation. MDEV-20865 will track further progress. P.S. Actually this big enough subtask took more than several days.

            People

              midenok Aleksey Midenkov
              midenok Aleksey Midenkov
              Votes:
              1 Vote for this issue
              Watchers:
              7 Start watching this issue

              Dates

                Created:
                Updated:
                Resolved:

                Git Integration

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