maria-developers team mailing list archive

[Aleksey Midenkov <midenok@xxxxxxxxx>]

Hi, Sergei!

On Fri, Dec 6, 2019 at 5:34 PM Sergei Golubchik <serg@xxxxxxxxxxx> 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@xxxxxxxxxxx
>


-- 
All the best,

Aleksey Midenkov
@midenok