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 <mide...@gmail.com>
> committer: Aleksey Midenkov <mide...@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 secur...@mariadb.org
_______________________________________________
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help : https://help.launchpad.net/ListHelp
