Re: [Maria-developers] eeba5b2b158: Fixed close_cached_connection_tables() flushing

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 committer: Sergey Vojtovich 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