maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Michael!

On Apr 16, Michael Widenius wrote:
> Hi!
> 
> On Thu, Apr 16, 2020 at 1:17 PM Sergei Golubchik <serg@xxxxxxxxxxx> wrote:
> >
> > Hi, Michael!
> >
> > On Apr 13, Michael Widenius wrote:
> > > revision-id: bc5c062b1d1 (mariadb-10.5.2-124-gbc5c062b1d1)
> > > parent(s): fb29c886701
> > > author: Michael Widenius <monty@xxxxxxxxxxx>
> > > committer: Michael Widenius <monty@xxxxxxxxxxx>
> > > timestamp: 2020-04-09 01:37:02 +0300
> > > message:
> > >
> > > Don't try to open temporary tables if there are no temporary tables
> 
> > Why?
> 
> Because it's quite slow to loop over all tables to check if could be a
> temporary tables
> in the default case where we don't have any temporary tables at all.
> This will improve performance for almost any table open.

Ok, I see. You mean THD::open_temporary_tables().
Because THD::open_temporary_table() already had a check and was not
doing any loops. And you moved the check out, making it the
responsibility of a caller.

This increases copy-pasting and is very error-prone because now everyone
has to remember to call thd->has_temporary_tables() before calling
thd->open_temporary_table().

A better solution would be to have a private method

  THD::open_temporary_table_int()

that does what THD::open_temporary_table() does in your patch, and an
inline method

  inline bool THD::open_temporary_table(TABLE_LIST *tl)
  { return has_temporary_tables() && open_temporary_table_int(tl); }

This way callers wouldn't need to be changed at all, while
THD::open_temporary_tables() could use open_temporary_table_int()
directly.

> I have added this to the commit message
> 
> > > diff --git a/sql/sql_base.cc b/sql/sql_base.cc
> > > index 4d7a7606136..be19a2e1d82 100644
> > > --- a/sql/sql_base.cc
> > > +++ b/sql/sql_base.cc
> > > @@ -3704,9 +3704,9 @@ open_and_process_table(THD *thd, TABLE_LIST *tables, uint *counter, uint flags,
> > >          The problem is that since those attributes are not set in merge
> > >          children, another round of PREPARE will not help.
> > >      */
> > > -    error= thd->open_temporary_table(tables);
> > > -
> > > -    if (!error && !tables->table)
> > > +    if (!thd->has_temporary_tables() ||
> > > +        (!(error= thd->open_temporary_table(tables)) &&
> > > +         !tables->table))
> >
> > please don't write conditions like that. keep it as before, two
> > statements:
> 
> Why?  It's perfectly clear and not worse than
> 
>   if (!thd->has_temporary_tables())
>     if (!(error= thd->open_temporary_table(tables)) &&  !tables->table))

of course it's not worse. It's exactly identical. I said it's worse than
what's below.

> >    if (thd->has_temporary_tables())
> >      error= thd->open_temporary_table(tables);
> >    if (!error && !tables->table)
> 
> Sorry, the above is worse and more error prone as you error may no be
> set when you test it.

See open_and_process_table(), it is.

Regards,
Sergei
VP of MariaDB Server Engineering
and security@xxxxxxxxxxx