Re: [Maria-developers] bc5c062b1d1: Don't try to open temporary tables if there are no temporary tables
Hi, Michael! On Apr 13, Michael Widenius wrote:
revision-id: bc5c062b1d1 (mariadb-10.5.2-124-gbc5c062b1d1) parent(s): fb29c886701 author: Michael Widenius <monty@mariadb.com> committer: Michael Widenius <monty@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?
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: if (thd->has_temporary_tables()) error= thd->open_temporary_table(tables); if (!error && !tables->table)
error= open_table(thd, tables, ot_ctx);
thd->pop_internal_handler(); @@ -3723,9 +3723,9 @@ open_and_process_table(THD *thd, TABLE_LIST *tables, uint *counter, uint flags, Repair_mrg_table_error_handler repair_mrg_table_handler; thd->push_internal_handler(&repair_mrg_table_handler);
- error= thd->open_temporary_table(tables); - - if (!error && !tables->table) + if (!thd->has_temporary_tables() || + (!(error= thd->open_temporary_table(tables)) && + !tables->table)) error= open_table(thd, tables, ot_ctx);
thd->pop_internal_handler(); @@ -3740,7 +3740,8 @@ open_and_process_table(THD *thd, TABLE_LIST *tables, uint *counter, uint flags, still might need to look for a temporary table if this table list element corresponds to underlying table of a merge table. */ - error= thd->open_temporary_table(tables); + if (thd->has_temporary_tables()) + error= thd->open_temporary_table(tables); }
if (!error && !tables->table) diff --git a/sql/temporary_tables.cc b/sql/temporary_tables.cc index 407072a7b49..553460729a1 100644 --- a/sql/temporary_tables.cc +++ b/sql/temporary_tables.cc @@ -338,9 +338,9 @@ bool THD::open_temporary_table(TABLE_LIST *tl) have invalid db or table name. Instead THD::open_tables() should be used. */ - DBUG_ASSERT(!tl->derived && !tl->schema_table); + DBUG_ASSERT(!tl->derived && !tl->schema_table && has_temporary_tables());
please, never do asserts with &&-ed conditions, this should be DBUG_ASSERT(!tl->derived); DBUG_ASSERT(!tl->schema_table); DBUG_ASSERT(has_temporary_tables());
- if (tl->open_type == OT_BASE_ONLY || !has_temporary_tables()) + if (tl->open_type == OT_BASE_ONLY) { DBUG_PRINT("info", ("skip_temporary is set or no temporary tables")); DBUG_RETURN(false);
Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
Hi! On Thu, Apr 16, 2020 at 1:17 PM Sergei Golubchik <serg@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 <monty@mariadb.com> committer: Michael Widenius <monty@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. 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))
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.
--- a/sql/temporary_tables.cc +++ b/sql/temporary_tables.cc @@ -338,9 +338,9 @@ bool THD::open_temporary_table(TABLE_LIST *tl) have invalid db or table name. Instead THD::open_tables() should be used. */ - DBUG_ASSERT(!tl->derived && !tl->schema_table); + DBUG_ASSERT(!tl->derived && !tl->schema_table && has_temporary_tables());
please, never do asserts with &&-ed conditions, this should be
DBUG_ASSERT(!tl->derived); DBUG_ASSERT(!tl->schema_table); DBUG_ASSERT(has_temporary_tables());
ok, I can change that. Regards, Monty
Hi, Michael! On Apr 16, Michael Widenius wrote:
Hi!
On Thu, Apr 16, 2020 at 1:17 PM Sergei Golubchik <serg@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 <monty@mariadb.com> committer: Michael Widenius <monty@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 security@mariadb.org
participants (2)
-
Michael Widenius -
Sergei Golubchik