maria-developers team mailing list archive

[Aleksey Midenkov <midenok@xxxxxxxxx>]

Hi Sergei!

On Wed, Dec 4, 2019 at 10:10 PM Sergei Golubchik <serg@xxxxxxxxxxx> wrote:

> 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?
>

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.


> 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.


>
> > +};
> >  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.
>

Probably, yes.


>
> > +  {
> > +    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
>

Ok. I really like the idea of overloading the operators. Maybe one day
we'll adopt them...


>
> > +
> >  };
>
> Regards,
> Sergei
> VP of MariaDB Server Engineering
> and security@xxxxxxxxxxx
>


-- 
All the best,

Aleksey Midenkov
@midenok