Re: [Maria-developers] eeba5b2b158: Fixed close_cached_connection_tables() flushing
Hi, Sergey! On Apr 02, Sergey Vojtovich wrote:
revision-id: eeba5b2b158 (mariadb-10.5.0-72-geeba5b2b158) parent(s): a737b71295e author: Sergey Vojtovich <svoj@mariadb.org> committer: Sergey Vojtovich <svoj@mariadb.org> timestamp: 2019-12-25 20:24:26 +0400 message:
Fixed close_cached_connection_tables() flushing
Let DROP SERVER and ALTER SERVER perform fair affected tables flushing. That is acquire MDL_EXCLUSIVE and do tdc_remove_table(TDC_RT_REMOVE_ALL).
Aim of this patch is elimination of another inconsistent use of TDC_RT_REMOVE_UNUSED. It fixes (to some extent) a problem described in the beginning of sql_server.cc, when close_cached_connection_tables() interferes with concurrent transaction.
A better fix should probably introduce proper MDL locks for server objects?
Part of MDEV-17882 - Cleanup refresh version
diff --git a/sql/sql_base.cc b/sql/sql_base.cc index 9494c0b7bd2..5c6d366ce6f 100644 --- a/sql/sql_base.cc +++ b/sql/sql_base.cc @@ -689,33 +689,21 @@ static my_bool close_cached_connection_tables_callback( Close cached connections
Could you, perhaps, move this function to sql_server.cc ? Also it could be made static there. And please clarify the comment, "Close cached connections' was not helpful at all. May be "Close all tables with a given CONNECTION= value"
@return false ok - @return true If there was an error from closed_cached_connection_tables or - if there was any open connections that we had to force closed + @return true error, some tables may keep using old server info */
bool close_cached_connection_tables(THD *thd, LEX_CSTRING *connection) { - bool res= false; - close_cached_connection_tables_arg argument; + close_cached_connection_tables_arg argument= { thd, connection, 0 }; DBUG_ENTER("close_cached_connections"); - DBUG_ASSERT(thd); - - argument.thd= thd; - argument.connection= connection; - argument.tables= NULL;
if (tdc_iterate(thd, (my_hash_walk_action) close_cached_connection_tables_callback, &argument)) DBUG_RETURN(true);
- for (TABLE_LIST *table= argument.tables; table; table= table->next_local) - res|= tdc_remove_table(thd, TDC_RT_REMOVE_UNUSED, - table->db.str, - table->table_name.str); - - /* Return true if we found any open connections */ - DBUG_RETURN(res); + DBUG_RETURN(close_cached_tables(thd, argument.tables, true, + thd->variables.lock_wait_timeout));
are you sure you want to do it when argument.tables is empty? It'll do purge_tables() which seems to be a bit extreme for DROP SERVER
}
Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
Sergei, On Thu, Apr 02, 2020 at 11:44:04AM +0200, Sergei Golubchik wrote:
Hi, Sergey!
On Apr 02, Sergey Vojtovich wrote:
revision-id: eeba5b2b158 (mariadb-10.5.0-72-geeba5b2b158) parent(s): a737b71295e author: Sergey Vojtovich <svoj@mariadb.org> committer: Sergey Vojtovich <svoj@mariadb.org> timestamp: 2019-12-25 20:24:26 +0400 message:
Fixed close_cached_connection_tables() flushing
Let DROP SERVER and ALTER SERVER perform fair affected tables flushing. That is acquire MDL_EXCLUSIVE and do tdc_remove_table(TDC_RT_REMOVE_ALL).
Aim of this patch is elimination of another inconsistent use of TDC_RT_REMOVE_UNUSED. It fixes (to some extent) a problem described in the beginning of sql_server.cc, when close_cached_connection_tables() interferes with concurrent transaction.
A better fix should probably introduce proper MDL locks for server objects?
Part of MDEV-17882 - Cleanup refresh version
diff --git a/sql/sql_base.cc b/sql/sql_base.cc index 9494c0b7bd2..5c6d366ce6f 100644 --- a/sql/sql_base.cc +++ b/sql/sql_base.cc @@ -689,33 +689,21 @@ static my_bool close_cached_connection_tables_callback( Close cached connections
Could you, perhaps, move this function to sql_server.cc ? Also it could be made static there.
And please clarify the comment, "Close cached connections' was not helpful at all. May be "Close all tables with a given CONNECTION= value" Ok.
@return false ok - @return true If there was an error from closed_cached_connection_tables or - if there was any open connections that we had to force closed + @return true error, some tables may keep using old server info */
bool close_cached_connection_tables(THD *thd, LEX_CSTRING *connection) { - bool res= false; - close_cached_connection_tables_arg argument; + close_cached_connection_tables_arg argument= { thd, connection, 0 }; DBUG_ENTER("close_cached_connections"); - DBUG_ASSERT(thd); - - argument.thd= thd; - argument.connection= connection; - argument.tables= NULL;
if (tdc_iterate(thd, (my_hash_walk_action) close_cached_connection_tables_callback, &argument)) DBUG_RETURN(true);
- for (TABLE_LIST *table= argument.tables; table; table= table->next_local) - res|= tdc_remove_table(thd, TDC_RT_REMOVE_UNUSED, - table->db.str, - table->table_name.str); - - /* Return true if we found any open connections */ - DBUG_RETURN(res); + DBUG_RETURN(close_cached_tables(thd, argument.tables, true, + thd->variables.lock_wait_timeout));
are you sure you want to do it when argument.tables is empty? It'll do purge_tables() which seems to be a bit extreme for DROP SERVER
You're right. I'll get this fixed. Thanks, Sergey
participants (2)
-
Sergei Golubchik -
Sergey Vojtovich