Hi, Sergei! On Fri, Dec 6, 2019 at 5:34 PM Sergei Golubchik <serg@mariadb.org> wrote:
Hi, Aleksey!
On Dec 05, Aleksey Midenkov wrote:
3. Invalidate referenced shares on:
- RENAME TABLE - DROP TABLE - RENAME COLUMN - CREATE TABLE - ADD FOREIGN KEY
When foreign table is created or altered by the above operations all referenced shares are closed. This blocks the operation while any referenced shares are used (when at least one its TABLE instance is locked).
And this is the main question of this email: Why do you close referenced tables?
Table cannot operate with wrong TABLE_SHARE::referenced_keys, so it must be locked anyway whether referenced_keys are modified or reloaded. The choice was made to reload as the first simplest implementation. And unless we know real performance drawback I'd like to keep it that way. Modifying TABLE_SHARE::referenced_keys requires several different algorithms for the above operations.
I feel that closing tables is a bit too heavy.
By the way, do you open referenced tables for SELECT? Or does table->s->referenced_keys stay NULL?
Now it stays NULL, but next implementation with FRM files will see foreign tables list referencing it and it will need to open their shares to update its referenced_keys. I don't know if we need such level of independence as to not open foreign tables when not needed. But yes, that could be a feature to load foreign tables on demand.
Minor comments below.
diff --git a/sql/handler.h b/sql/handler.h index e913af1d15d..10984505f70 100644 --- a/sql/handler.h +++ b/sql/handler.h @@ -1030,6 +1031,15 @@ struct TABLE_SHARE; struct HA_CREATE_INFO; struct st_foreign_key_info; typedef struct st_foreign_key_info FOREIGN_KEY_INFO; +class Table_ident; +class FK_list : public List<FOREIGN_KEY_INFO> +{ +public: + /* Get all referenced tables for foreign key fk_name. */ + bool get(THD *thd, std::set<Table_ident> &result, LEX_CSTRING &fk_name, bool foreign); + /* Get all referenced or foreign tables. */ + bool get(THD *thd, std::set<Table_ident> &result, bool foreign);
Seems unnecessary. Copying a list into a std::set _could_ be justified, if you'd later used it for quick checks "if this table belongs to a set" - something you cannot quickly do with a List.
But as far as I can see you copy the List into std::set and then iterate this set. This doesn't make much sense, you can iterate the original list just fine.
std::set does duplicate elimination. Iterating a list would require an algorithm to skip the duplicates.
Duplicates? Can there be duplicate foreign keys?
No, but duplicates of Table_ident can be, because multiple foreign keys can reference same table.
Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
-- All the best, Aleksey Midenkov @midenok