maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

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@xxxxxxxxx>
> committer: Aleksey Midenkov <midenok@xxxxxxxxx>
> 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@xxxxxxxxxxx