maria-developers team mailing list archive

[Sergey Vojtovich <svoj@xxxxxxxxxxx>]

Hi Sergei,

On Mon, Sep 24, 2018 at 03:02:40PM +0200, Sergei Golubchik wrote:
> Hi, Sergey!
> 
> On Sep 12, Sergey Vojtovich wrote:
> > revision-id: e11e0e6c8b9a85ca7f16c6b9b28fb505d464a96e (mariadb-10.3.7-186-ge11e0e6c8b9)
> > parent(s): f2f661b848c02ebd7c444ba645b26416d3e4d2ad
> > author: Sergey Vojtovich
> > committer: Sergey Vojtovich
> > timestamp: 2018-09-12 16:36:45 +0400
> > message:
> > 
> > MDEV-17167 - InnoDB: Failing assertion: table->get_ref_count() == 0 upon
> >              truncating a temporary table
> > 
> > TRUNCATE expects only one TABLE instance (which is used by TRUNCATE
> > itself) to be open. However this requirement wasn't enforced after
> > "MDEV-5535: Cannot reopen temporary table".
> > 
> > Fixed by closing unused table instances before performing TRUNCATE.
> 
> > diff --git a/sql/sql_class.h b/sql/sql_class.h
> > index 6256522bf5c..e7dcd997e05 100644
> > --- a/sql/sql_class.h
> > +++ b/sql/sql_class.h
> > @@ -4612,6 +4612,7 @@ class THD :public Statement,
> >  
> >    TMP_TABLE_SHARE* save_tmp_table_share(TABLE *table);
> >    void restore_tmp_table_share(TMP_TABLE_SHARE *share);
> > +  void close_unused_temporary_table_instances(const TABLE_LIST *tl);
> >  
> >  private:
> >    /* Whether a lock has been acquired? */
> > diff --git a/sql/sql_truncate.cc b/sql/sql_truncate.cc
> > index 201825d4593..695fcb538f9 100644
> > --- a/sql/sql_truncate.cc
> > +++ b/sql/sql_truncate.cc
> > @@ -401,6 +401,8 @@ bool Sql_cmd_truncate_table::truncate_table(THD *thd, TABLE_LIST *table_ref)
> >      /* In RBR, the statement is not binlogged if the table is temporary. */
> >      binlog_stmt= !thd->is_current_stmt_binlog_format_row();
> >  
> > +    thd->close_unused_temporary_table_instances(table_ref);
> 
> Is it only TRUNCATE that needs it? What about ALTER? REPAIR? Other
> similar commands?
Right, do you think it is worth to exapnd scope of this bug and look for
other possible issues?

> > +*/
> > +
> > +void THD::close_unused_temporary_table_instances(const TABLE_LIST *tl)
> > +{
> > +  TMP_TABLE_SHARE *share= find_tmp_table_share(tl);
> > +
> > +  if (share)
> > +  {
> > +    Share_free_tables::List purge_tables;
> > +    All_share_tables_list::Iterator tables_it(share->all_tmp_tables);
> > +
> > +     while (TABLE *table= tables_it++)
> > +     {
> > +       if (table->query_id == 0)
> > +         purge_tables.push_front(table);
> > +     }
> > +
> > +     while (TABLE *table= purge_tables.pop_front())
> > +     {
> > +       share->all_tmp_tables.remove(table);
> > +       free_temporary_table(table);
> > +     }
> 
> Why are you doing it in two loops? Because free_temporary_table()
> invalidates the iterator?
Not free_temporary_table(), but rather share->all_tmp_tables.remove(table).

OTOH it seems to be safe to remove current element with it++, but not with ++it.
I'll try to remove extra loop if it works.

Thanks,
Sergey