maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Sergey!

On Sep 24, Sergey Vojtovich wrote:
> On Mon, Sep 24, 2018 at 03:02:40PM +0200, Sergei Golubchik wrote:
> > On Sep 12, Sergey Vojtovich wrote:
> > 
> > > 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?

Yes, absolutely

> > > +*/
> > > +
> > > +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. Please add a comment about it (in whatever code you'll end up
having, two loops or it++ - still this is something that needs a
comment).

Regards,
Sergei
Chief Architect MariaDB
and security@xxxxxxxxxxx