Hi, Michael!
On Apr 16, Michael Widenius wrote:
> Hi!
>
> On Thu, Apr 16, 2020 at 1:17 PM Sergei Golubchik <s...@mariadb.org> wrote:
> >
> > Hi, Michael!
> >
> > On Apr 13, Michael Widenius wrote:
> > > revision-id: bc5c062b1d1 (mariadb-10.5.2-124-gbc5c062b1d1)
> > > parent(s): fb29c886701
> > > author: Michael Widenius <mo...@mariadb.com>
> > > committer: Michael Widenius <mo...@mariadb.com>
> > > 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 secur...@mariadb.org
_______________________________________________
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help : https://help.launchpad.net/ListHelp
