Hi, Aleksey! Just a question for now (and a couple of style comments, I didn't look at the logic yet) On Dec 04, Aleksey Midenkov wrote:
revision-id: c91ec05e01b (mariadb-10.4.4-427-gc91ec05e01b) parent(s): e6d653b448d author: Aleksey Midenkov <midenok@gmail.com> committer: Aleksey Midenkov <midenok@gmail.com> timestamp: 2019-11-26 13:04:07 +0300 message:
MDEV-20865 Store foreign key info in TABLE_SHARE
1. Access foreign keys via TABLE_SHARE::foreign_keys and TABLE_SHARE::referenced_keys;
2. Remove handler FK interface:
- get_foreign_key_list() - get_parent_foreign_key_list() - referenced_by_foreign_key()
Good, that was the goal
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? 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.
+}; typedef bool (stat_print_fn)(THD *thd, const char *type, size_t type_len, const char *file, size_t file_len, const char *status, size_t status_len); diff --git a/sql/lex_string.h b/sql/lex_string.h index a62609c6b60..769f4dcbf5e 100644 --- a/sql/lex_string.h +++ b/sql/lex_string.h @@ -41,11 +41,47 @@ class Lex_cstring : public LEX_CSTRING str= start; length= end - start; } + Lex_cstring(const LEX_CSTRING &src) + { + str= src.str; + length= src.length; + } void set(const char *_str, size_t _len) { str= _str; length= _len; } + Lex_cstring *strdup_root(MEM_ROOT &mem_root)
The way you use it, it looks like you really need a constructor, not a strdup.
+ { + Lex_cstring *dst= + (Lex_cstring *) alloc_root(&mem_root, sizeof(Lex_cstring)); + if (!dst) + return NULL; + if (!str) + { + dst->str= NULL; + dst->length= 0; + return dst; + } + dst->str= (const char *) memdup_root(&mem_root, str, length + 1); + if (!dst->str) + return NULL; + dst->length= length; + return dst; + } + bool operator< (const Lex_cstring& rhs) const + { + return length < rhs.length || (length == rhs.length && memcmp(str, rhs.str, length) < 0); + } + bool operator== (const Lex_cstring& rhs) const + { + return length == rhs.length && 0 == memcmp(str, rhs.str, length); + } + bool operator> (const Lex_cstring& rhs) const + { + return length > rhs.length || (length == rhs.length && memcmp(str, rhs.str, length) > 0); + }
Nope. We've been here before, haven't we? Don't overload operators. If you want a method to compare, call it cmp(), or greater_than(), or something btw your comparison has quite weird and unconventional semantics
+ };
Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org