Re: bk commit into 6.0 tree (anozdrin:1.2613) BUG#35395

[Dmitri Lenev]

Hello Alik!

Here are my comments about your patch:

* Alexander Nozdrin <alik@stripped> [08/04/28 16:57]:
> ChangeSet@stripped, 2008-04-28 17:04:12+04:00, anozdrin@quad. +1 -0
>   A fix for Bug#35395: Object services for enumerating view
>   dependencies make server deadlock.
>   
>   The problem was that THD::~THD() does not always close open tables.

I think such description of the problem is a bit misleading.
If it was correct description then a natural solution for the
problem would have been fixing THD::~THD() to close all open
tables. But we have choosen another option. So I think this
description should be replaced with something like:

  The problem was that ViewBaseObjectsIterator::create() method
  opened view and its underlying tables in order to get list of
  all tables on which view depends without closing them afterwards.
  This happened due to false assumption that THD::~THD() closes
  all tables which are open for THD object being destroyed.

>   
>   The fix is to call close_thread_tables() explicitly. Also, since
>   we're interested only in meta-data, there is no need to lock tables.
>   So, open_tables() can be used instead of open_and_lock_tables().
> 

Please add short sentence explaining why we don't provide test
case for this fix.

...

>   sql/si_objects.cc@stripped, 2008-04-28 17:04:11+04:00, anozdrin@quad. +3 -1
>     1. Use open_tables() instead of open_and_lock_tables().
>     2. Explicitly close open tables.
> 
> diff -Nrup a/sql/si_objects.cc b/sql/si_objects.cc
> --- a/sql/si_objects.cc	2008-04-24 13:18:47 +04:00
> +++ b/sql/si_objects.cc	2008-04-28 17:04:11 +04:00
> @@ -1318,6 +1318,7 @@ ViewBaseObjectsIterator::create(THD *thd
>                                 const String *view_name,
>                                 IteratorType iterator_type)
>  {
> +  uint table_count; // Passed to open_tables(). Not used.
>    THD *my_thd= new THD();

You can get rid of this comment and make the code self-explanatory
by calling this variable 'not_used' instead of 'table_count' (we use
this convention in several other places in server).

>  
>    my_thd->security_ctx= thd->security_ctx;
> @@ -1333,7 +1334,7 @@ ViewBaseObjectsIterator::create(THD *thd
>                             ((String *) view_name)->c_ptr_safe(),
>                             TL_READ);
>  
> -  if (open_and_lock_tables(my_thd, tl))
> +  if (open_tables(my_thd, &tl, &table_count, 0))
>    {

You should call close_thread_tables() even after failed call to
open_tables() since even in such case it is possible that this
call has managed to open some of the tables.

>      delete my_thd;
>      thd->store_globals();
> @@ -1376,6 +1377,7 @@ ViewBaseObjectsIterator::create(THD *thd
>      }
>    }
>  
> +  close_thread_tables(my_thd);
>    delete my_thd;
>  
>    thd->store_globals();
> 

I think it is OK to push this patch after addressing/considering
above issues.

-- 
Dmitri Lenev, Software Developer
MySQL AB, www.mysql.com

Are you MySQL certified?  http://www.mysql.com/certification