maria-developers team mailing list archive

[Nirbhay Choubey <nirbhay@xxxxxxxxxxx>]

Hi Serg!

On Fri, Jun 3, 2016 at 4:10 PM, Sergei Golubchik <serg@xxxxxxxxxxx> wrote:

> Hi, Nirbhay!
>
> Just a couple of questions/comments, see below.
>
> And please pay attention to Kristian's email about locking and parallel
> replication. I don't think I can confidently review it in a patch, so
> you may want to spend more time testing or analyzing that code.
>

Yes, I did add some related rpl test cases.


>
> Ok to push, when you're ready! Thanks!
>
> On May 26, Nirbhay Choubey wrote:
> > revision-id: 56a2835872c4ac7296ec0ae2ff618822855b0fc0
> (mariadb-10.1.8-82-g56a2835)
> > parent(s): 28c289626f318631d707f85b057a90af99018b06
> > author: Nirbhay Choubey
> > committer: Nirbhay Choubey
> > timestamp: 2016-05-26 20:42:47 -0400
> > message:
> >
> > MDEV-5535: Cannot reopen temporary table
> >
> > mysqld maintains a list of TABLE objects for all temporary
> > tables in THD, where each table is represented by a TABLE
> > object. A query referencing a temporary table for more than
> > once, however, failed with ER_CANT_REOPEN_TABLE error.
>
> "because a TABLE_SHARE was allocate together with the TABLE, so
> temporary tables always had one TABLE per TABLE_SHARE"
>


> > This patch lift this restriction by preserving the TABLE_SHARE
> > object of each created temporary table, so that multiple instances
> > of TABLE objects can be created.
>
> better "by separating TABLE and TABLE_SHARE objects and storing
> TABLE_SHARE's in the list in a THD, and TABLE's in the list in
> the corresponding TABLE_SHARE"
>

That's better indeed. I will update that when I merge the consolidated
patches to the main branch.


>
> > diff --git a/sql/rpl_rli.cc b/sql/rpl_rli.cc
> > index 7bdd9c1..77f458b 100644
> > --- a/sql/rpl_rli.cc
> > +++ b/sql/rpl_rli.cc
> > @@ -1060,24 +1061,43 @@ void
> Relay_log_info::inc_group_relay_log_pos(ulonglong log_pos,
> >
> >  void Relay_log_info::close_temporary_tables()
> >  {
> > -  TABLE *table,*next;
> >    DBUG_ENTER("Relay_log_info::close_temporary_tables");
> >
> > -  for (table=save_temporary_tables ; table ; table=next)
> > +  TMP_TABLE_SHARE *share;
> > +  TABLE *table;
> > +
> > +  while ((share= save_temporary_tables.pop_front()))
> >    {
> > -    next=table->next;
> > +    /*
> > +      Iterate over the list of tables for this TABLE_SHARE and close
> them.
> > +    */
> > +    while ((table= share->all_tmp_tables.pop_front()))
> > +    {
> > +      DBUG_PRINT("tmptable", ("closing table: '%s'.'%s'",
> > +                              table->s->db.str,
> table->s->table_name.str));
> > +
> > +      /* Reset in_use as the table may have been created by another thd
> */
> > +      table->in_use= 0;
> > +      free_io_cache(table);
> > +      /*
> > +        Lets not free TABLE_SHARE here as there could be multiple
> TABLEs opened
> > +        for the same table (TABLE_SHARE).
> > +      */
> > +      closefrm(table, false);
> > +      my_free(table);
> > +    }
> >
> > -    /* Reset in_use as the table may have been created by another thd */
> > -    table->in_use=0;
> >      /*
> >        Don't ask for disk deletion. For now, anyway they will be deleted
> when
> >        slave restarts, but it is a better intention to not delete them.
> >      */
> > -    DBUG_PRINT("info", ("table: 0x%lx", (long) table));
> > -    close_temporary(table, 1, 0);
> > +
> > +    free_table_share(share);
> > +    my_free(share);
> >    }
> > -  save_temporary_tables= 0;
> > -  slave_open_temp_tables= 0;
> > +
> > +  save_temporary_tables.empty();
>
> Is it needed?
> you've popped all shares from the list, it's empty.
> I'd rather add an assert instead.
>

Right. I realized this later. Fixed.


>
> > +
> >    DBUG_VOID_RETURN;
> >  }
> >
> > diff --git a/sql/sql_class.h b/sql/sql_class.h
> > index ae240ae..a70c4a9 100644
> > --- a/sql/sql_class.h
> > +++ b/sql/sql_class.h
> > @@ -1247,6 +1247,61 @@ enum enum_locked_tables_mode
> >    LTM_PRELOCKED_UNDER_LOCK_TABLES
> >  };
> >
> > +/**
> > +  The following structure is an extension to TABLE_SHARE and is
> > +  exclusively for temporary tables.
> > +
> > +  @note:
> > +  Although, TDC_element has data members (like next, prev &
> > +  all_tables) to store the list of TABLE_SHARE & TABLE objects
> > +  related to a particular TABLE_SHARE, they cannot be moved to
> > +  TABLE_SHARE in order to be reused for temporary tables. This
> > +  is because, as concurrent threads iterating through hash of
> > +  TDC_element's may need access to all_tables, but if all_tables
> > +  is made part of TABLE_SHARE, then TDC_element->share->all_tables
> > +  is not always guaranteed to be valid, as TDC_element can live
> > +  longer than TABLE_SHARE.
> > +*/
> > +struct TMP_TABLE_SHARE : public TABLE_SHARE
> > +{
> > +private:
> > +  /*
> > +   Link to all temporary table shares. Declared as private to
> > +   avoid direct manipulation with those objects. One should
> > +   use methods of I_P_List template instead.
> > +  */
> > +  TMP_TABLE_SHARE *tmp_next;
> > +  TMP_TABLE_SHARE **tmp_prev;
> > +
> > +  friend struct All_tmp_table_shares;
> > +
> > +public:
> > +  /*
> > +    Doubly-linked (back-linked) lists of used and unused TABLE objects
> > +    for this share.
>
> and unused? you don't free them but keep in the list?
>

Yes. We did agree to not free/close but reuse them.


>
> > +  */
> > +  All_share_tables_list all_tmp_tables;
> > +};
> > +
> > +/**
> > +  Helper class which specifies which members of TMP_TABLE_SHARE are
> > +  used for participation in the list of temporary tables.
> > +*/
> > +
> > +struct All_tmp_table_shares
> > +{
> > +  static inline TMP_TABLE_SHARE **next_ptr(TMP_TABLE_SHARE *l)
> > +  {
> > +    return &l->tmp_next;
> > +  }
> > +  static inline TMP_TABLE_SHARE ***prev_ptr(TMP_TABLE_SHARE *l)
> > +  {
> > +    return &l->tmp_prev;
> > +  }
> > +};
> > +
> > +/* Also used in rpl_rli.h. */
> > +typedef I_P_List <TMP_TABLE_SHARE, All_tmp_table_shares>
> All_tmp_tables_list;
> >
> >  /**
> >    Class that holds information about tables which were opened and locked
> > diff --git a/sql/temporary_tables.cc b/sql/temporary_tables.cc
> > new file mode 100644
> > index 0000000..1514b72
> > --- /dev/null
> > +++ b/sql/temporary_tables.cc
> ...
> > +TABLE *THD::find_temporary_table(const char *db,
> > +                                 const char *table_name)
> > +{
> > +  DBUG_ENTER("THD::find_temporary_table");
> > +
> > +  TABLE *table;
> > +  char key[MAX_DBKEY_LENGTH];
> > +  uint key_length;
> > +  bool locked;
> > +
> > +  if (!(has_temporary_tables() || (rgi_slave &&
> has_slave_temporary_tables())))
>
> hmm, really? you had here is_empty(true) before
> and your is_empty() was working like this:
>
>   if (flag_is_true)
>   { do rgi_slave }
>   else
>   { do thd }
>
> your old is_empty was not checking thd if the argument was true,
> your new code does, as if your is_empty() was
>
>   do thd;
>   if (flag_is_true)
>   { do rgi_slave}
>
> > +  {
> > +    DBUG_RETURN(NULL);
> > +  }
> > +
> > +  key_length= create_tmp_table_def_key(key, db, table_name);
>

Yea, there was a problem here. It should have checked for !rgi_slave for
thd before
proceeding check slave temporary tables. Its now :

(!rgi_slave && has_temporary_tables()) || (rgi_slave &&
unlikely(has_slave_temporary_tables())))

Best,
Nirbhay


>
> Regards,
> Sergei
> Chief Architect MariaDB
> and security@xxxxxxxxxxx
>