maria-developers team mailing list archive

[Sergey Vojtovich <svoj@xxxxxxxxxxx>]

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