Hello Kostja!
Here are my comments about your patch:
* Konstantin Osipov <konstantin@stripped> [08/06/06 15:35]:
> #At file:///opt/local/work/mysql-6.0-prelocked_mode/
>
> 2662 Konstantin Osipov 2008-06-06
> WL#3726: work on review comments.
> Remove thd->locked_tables. Always store MYSQL_LOCK instances in
> thd->lock. Use thd->prelocked_mode to determine if we
> are under LOCK TABLES. Update the code to not assume that
> if thd->lock is set, LOCK TABLES mode is off.
OK.
> Remove dead code in lock_tables() that would set thd->in_lock_tables
> and OPTION_LOCK_TABLES when locking tables for pre-locking.
IMO it is much better to move these two changes into a separate patch
to be discussed and reviewed separately. They are not directly required
for your current task and somewhat controversial.
...
> === modified file 'sql/handler.cc'
> --- a/sql/handler.cc 2008-05-27 10:44:38 +0000
> +++ b/sql/handler.cc 2008-06-06 11:27:03 +0000
> @@ -5182,17 +5182,14 @@ static bool check_table_binlog_row_based
> static int write_locked_table_maps(THD *thd)
> {
> DBUG_ENTER("write_locked_table_maps");
> - DBUG_PRINT("enter", ("thd: %p thd->lock: %p thd->locked_tables: %p "
> - "thd->extra_lock: %p",
> - thd, thd->lock,
> - thd->locked_tables, thd->extra_lock));
> + DBUG_PRINT("enter", ("thd: %p thd->lock: %p thd->extra_lock: %p",
> + thd, thd->lock, thd->extra_lock));
>
> if (thd->get_binlog_table_maps() == 0)
> {
> - MYSQL_LOCK *locks[3];
> + MYSQL_LOCK *locks[2];
> locks[0]= thd->extra_lock;
> locks[1]= thd->lock;
> - locks[2]= thd->locked_tables;
> for (uint i= 0 ; i < sizeof(locks)/sizeof(*locks) ; ++i )
> {
> MYSQL_LOCK const *const lock= locks[i];
Could you please fix comment for this function? Also please grep
through sources and fix other comments mentioning "locked_tables"...
...
> === modified file 'sql/sql_base.cc'
> --- a/sql/sql_base.cc 2008-06-05 15:33:38 +0000
> +++ b/sql/sql_base.cc 2008-06-06 11:27:03 +0000
> @@ -1001,7 +1001,7 @@ bool close_cached_tables(THD *thd, TABLE
> DBUG_ASSERT(!have_lock);
> pthread_mutex_unlock(&LOCK_open);
>
> - if (thd->locked_tables)
> + if (thd->prelocked_mode)
> {
> /*
> If we are under LOCK TABLES we need to reopen tables without
> @@ -1115,7 +1115,7 @@ bool close_cached_tables(THD *thd, TABLE
> }
>
> err_with_reopen:
> - if (thd->locked_tables)
> + if (thd->prelocked_mode)
> {
> pthread_mutex_lock(&LOCK_open);
> /*
Hmm... Having check for THD::prelocked_mode in close_cached_tables()
and in many other places is somewhat counter-intuitive from my point
of view... May be it makes sense to rename THD::prelocked_mode to
something like THD::locked_tables_mode ?
...
> @@ -1454,16 +1454,6 @@ void close_thread_tables(THD *thd,
> mdl_remove_all_locks(&thd->mdl_context);
> }
>
> - if (prelocked_mode == PRELOCKED)
> - {
> - /*
> - If we are here then we are leaving normal prelocked mode, so it is
> - good idea to turn off OPTION_TABLE_LOCK flag.
> - */
> - DBUG_ASSERT(thd->lex->requires_prelocking());
> - thd->options&= ~(OPTION_TABLE_LOCK);
> - }
> -
> DBUG_VOID_RETURN;
> }
See my comment above.
> @@ -5250,14 +5239,14 @@ int lock_tables(THD *thd, TABLE_LIST *ta
>
> /*
> We need this extra check for thd->prelocked_mode because we want to avoid
> - attempts to lock tables in substatements. Checking for thd->locked_tables
> + attempts to lock tables in substatements. Checking for thd->lock
> is not enough in some situations. For example for SP containing
> "drop table t3; create temporary t3 ..; insert into t3 ...;"
> - thd->locked_tables may be 0 after drop tables, and without this extra
> + thd->lock may be 0 after drop tables, and without this extra
> check insert will try to lock temporary table t3, that will lead
> to memory leak...
> */
I think this comment should be rephrased.
> - if (!thd->locked_tables && !thd->prelocked_mode)
> + if (!thd->prelocked_mode)
> {
> DBUG_ASSERT(thd->lock == 0); // You must lock everything at once
> TABLE **start,**ptr;
> @@ -5273,8 +5262,6 @@ int lock_tables(THD *thd, TABLE_LIST *ta
> /* We have to emulate LOCK TABLES if we are statement needs prelocking. */
> if (thd->lex->requires_prelocking())
> {
> - thd->in_lock_tables=1;
> - thd->options|= OPTION_TABLE_LOCK;
> /*
> If we have >= 2 different tables to update with auto_inc columns,
> statement-based binlogging won't work. We can solve this problem in
...
Again see my comment above.
Otherwise I am OK with your patch and think that it can be pushed
after addressing the above comments.
--
Dmitry Lenev, Software Developer
MySQL AB, www.mysql.com
Are you MySQL certified? http://www.mysql.com/certification
