List:Commits« Previous MessageNext Message »
From:Konstantin Osipov Date:July 16 2009 7:38pm
Subject:Re: bzr commit into mysql-6.0-backup branch (ingo.struewing:2825)
Bug#20667
View as plain text  
* Ingo Struewing <Ingo.Struewing@stripped> [09/06/19 19:12]:
> #At file:///home2/mydev/bzrroot/mysql-6.0-bug20667-1/ based on
> revid:charles.bell@stripped
> 
>  2825 Ingo Struewing	2009-06-19
>       Bug#20667 - Truncate table fails for a write locked table
>       
>       TRUNCATE TABLE was not allowed under LOCK TABLES.
>       
>       The patch removes this restriction. mysql_truncate()
>       does now handle that case.

The patch is, generally, OK. 

However, instead of get_table() method that you implemented,
I think one should use an already existing function
find_write_locked_table(), which is used in, e.g. DROP TABLE
under LOCK TABLES.

Note that we can't simply use open_and_lock_single_table(),
since we want to be able to truncate damaged tables.

I wrote a comment about that. Please see the patch at the end of
the reply. Note that my patch is a sketch, please feel free to
improve on it.

Some more comments inline. 

>      @ mysql-test/r/merge.result
>         Bug#20667 - Truncate table fails for a write locked table
>         Updated test result.

<cut>

> === modified file 'mysql-test/t/merge.test'
> --- a/mysql-test/t/merge.test	2009-04-22 10:02:28 +0000
> +++ b/mysql-test/t/merge.test	2009-06-19 14:45:15 +0000
> @@ -688,12 +688,16 @@ SELECT * FROM t3;
>  --echo # Truncate MERGE table under locked tables.
>  LOCK TABLE t1 WRITE, t2 WRITE, t3 WRITE;
>  INSERT INTO t1 VALUES (1);
> ---error ER_LOCK_OR_ACTIVE_TRANSACTION
>  TRUNCATE TABLE t3;
>  SELECT * FROM t3;
> +UNLOCK TABLES;
> +SELECT * FROM t1;
> +SELECT * FROM t2;
>  --echo #
>  --echo # Truncate child table under locked tables.
> ---error ER_LOCK_OR_ACTIVE_TRANSACTION
> +LOCK TABLE t1 WRITE, t2 WRITE, t3 WRITE;
> +INSERT INTO t1 VALUES (1);
> +INSERT INTO t2 VALUES (2);
>  TRUNCATE TABLE t1;
>  SELECT * FROM t3;
>  UNLOCK TABLES;
> @@ -719,14 +723,18 @@ SELECT * FROM t3;
>  INSERT INTO t1 VALUES (1);
>  CREATE TABLE t4 (c1 INT, INDEX(c1));
>  LOCK TABLE t4 WRITE;
> ---error ER_LOCK_OR_ACTIVE_TRANSACTION
>  TRUNCATE TABLE t3;
>  SELECT * FROM t3;
> +SELECT * FROM t1;
> +SELECT * FROM t2;
>  --echo #
>  --echo # Truncate temporary child table under locked tables.
> ---error ER_LOCK_OR_ACTIVE_TRANSACTION
> +INSERT INTO t1 VALUES (1);
> +INSERT INTO t2 VALUES (2);
>  TRUNCATE TABLE t1;
>  SELECT * FROM t3;
> +SELECT * FROM t1;
> +SELECT * FROM t2;
>  UNLOCK TABLES;
>  DROP TABLE t1, t2, t3, t4;

Good.

> === modified file 'mysql-test/t/truncate.test'
> --- a/mysql-test/t/truncate.test	2007-04-17 10:32:01 +0000
> +++ b/mysql-test/t/truncate.test	2009-06-19 14:45:15 +0000
> @@ -69,3 +69,29 @@ drop table t1;
>  
>  # End of 5.0 tests
>  
> +--echo #
> +--echo # Bug#20667 - Truncate table fails for a write locked table
> +--echo #
> +CREATE TABLE t1 (c1 INT) ENGINE=MyISAM;
> +LOCK TABLE t1 WRITE;
> +INSERT INTO t1 VALUES (1);
> +SELECT * FROM t1;
> +TRUNCATE TABLE t1;
> +SELECT * FROM t1;
> +UNLOCK TABLES;
> +#
> +LOCK TABLE t1 READ;
> +--error ER_TABLE_NOT_LOCKED_FOR_WRITE
> +TRUNCATE TABLE t1;
> +UNLOCK TABLES;
> +#
> +CREATE TABLE t2 (c1 INT) ENGINE=MyISAM;
> +LOCK TABLE t2 WRITE;
> +--error ER_TABLE_NOT_LOCKED
> +TRUNCATE TABLE t1;
> +UNLOCK TABLES;
> +DROP TABLE t1;
> +DROP TABLE t2;
> +
> +--echo # End of 6.0 tests
> +

Good.

> === modified file 'sql/sql_delete.cc'
> --- a/sql/sql_delete.cc	2009-05-31 12:05:01 +0000
> +++ b/sql/sql_delete.cc	2009-06-19 14:45:15 +0000

Please use the attached patch as a basis.

> === modified file 'sql/sql_parse.cc'
> --- a/sql/sql_parse.cc	2009-06-12 02:01:08 +0000
> +++ b/sql/sql_parse.cc	2009-06-19 14:45:15 +0000
> @@ -3376,7 +3376,7 @@ end_with_restore_list:
>        Don't allow this within a transaction because we want to use
>        re-generate table
>      */
> -    if (thd->locked_tables_mode || thd->active_transaction())
> +    if (thd->active_transaction())
>      {
>        my_message(ER_LOCK_OR_ACTIVE_TRANSACTION,
>                   ER(ER_LOCK_OR_ACTIVE_TRANSACTION), MYF(0));

Correct.

Please add a test case for truncate table v1, where v1 is a view
(under LOCK TABLES).  

Please see the diff below.

Thank you so much for looking at this!

=== modified file 'sql/sql_delete.cc'
--- sql/sql_delete.cc	2009-06-19 09:28:44 +0000
+++ sql/sql_delete.cc	2009-07-16 19:27:12 +0000
@@ -1129,6 +1129,12 @@ bool mysql_truncate(THD *thd, TABLE_LIST
   if (!dont_send_ok)
   {
     enum legacy_db_type table_type;
+    /*
+      FIXME: Code of TRUNCATE breaks the meta-data
+      locking protocol since it tries to find out the table storage
+      engine and therefore accesses table in some way without holding
+      any kind of meta-data lock.
+    */
     mysql_frm_type(thd, path, &table_type);
     if (table_type == DB_TYPE_UNKNOWN)
     {
@@ -1143,24 +1149,46 @@ bool mysql_truncate(THD *thd, TABLE_LIST
 
     if (table_type == DB_TYPE_NDBCLUSTER)
       global_schema_lock_guard.lock();
-    /*
-      FIXME: Actually code of TRUNCATE breaks meta-data locking protocol since
-             tries to get table enging and therefore accesses table in some way
-             without holding any kind of meta-data lock.
-    */
-    mdl_request= MDL_request::create(0, table_list->db,
-                                     table_list->table_name, thd->mem_root);
-    mdl_request->set_type(MDL_EXCLUSIVE);
-    thd->mdl_context.add_request(mdl_request);
-    if (thd->mdl_context.acquire_exclusive_locks())
+
+    mysql_ha_rm_tables(thd, table_list);
+
+    if (thd->locked_tables_mode)
     {
-      thd->mdl_context.remove_request(mdl_request);
-      DBUG_RETURN(TRUE);
+      if (!(table= find_write_locked_table(thd->open_tables, table_list->db,
+                                           table_list->table_name)))
+        DBUG_RETURN(TRUE);
+      if (wait_while_table_is_used(thd, table, HA_EXTRA_FORCE_REOPEN))
+        goto end;
+      close_all_tables_for_name(thd, table->s, FALSE);
+    }
+    else
+    {
+      /*
+        Even though we could use the previous execution branch
+        here just as well, we must not try to open the table: 
+        MySQL manual documents that TRUNCATE can be used to 
+        repair a damaged table, i.e. a table that can not be
+        fully "opened". In particular MySQL manual says:
+
+        As long as the table format file tbl_name.frm  is valid,
+        the table can be re-created as an empty table with TRUNCATE
+        TABLE, even if the data or index files have become corrupted.
+      */
+
+      mdl_request= MDL_request::create(0, table_list->db,
+                                       table_list->table_name, thd->mem_root);
+      mdl_request->set_type(MDL_EXCLUSIVE);
+      thd->mdl_context.add_request(mdl_request);
+      if (thd->mdl_context.acquire_exclusive_locks())
+      {
+        thd->mdl_context.remove_request(mdl_request);
+        DBUG_RETURN(TRUE);
+      }
+      pthread_mutex_lock(&LOCK_open);
+      tdc_remove_table(thd, TDC_RT_REMOVE_ALL, table_list->db,
+                       table_list->table_name);
+      pthread_mutex_unlock(&LOCK_open);
     }
-    pthread_mutex_lock(&LOCK_open);
-    tdc_remove_table(thd, TDC_RT_REMOVE_ALL, table_list->db,
-                     table_list->table_name);
-    pthread_mutex_unlock(&LOCK_open);
   }
 
   /*
@@ -1178,6 +1206,12 @@ bool mysql_truncate(THD *thd, TABLE_LIST
 end:
   if (!dont_send_ok)
   {
+    if (thd->locked_tables_mode &&
thd->locked_tables_list.reopen_tables(thd))
+      thd->locked_tables_list.unlink_all_closed_tables();
+    /*
+      Even if we failed to reopen some tables,
+      the operation itself succeeded, write the binlog.
+    */
     if (!error)
     {
       /*
@@ -1193,14 +1227,6 @@ end:
       thd->mdl_context.remove_request(mdl_request);
     }
   }
-  else if (error)
-  {
-    if (mdl_request)
-    {
-      thd->mdl_context.release_lock(mdl_request->ticket);
-      thd->mdl_context.remove_request(mdl_request);
-    }
-  }
   DBUG_RETURN(error);
 
 trunc_by_del:

=== modified file 'sql/sql_parse.cc'
--- sql/sql_parse.cc	2009-07-10 12:31:32 +0000
+++ sql/sql_parse.cc	2009-07-16 17:13:55 +0000
@@ -3316,7 +3316,7 @@ end_with_restore_list:
       Don't allow this within a transaction because we want to use
       re-generate table
     */
-    if (thd->locked_tables_mode || thd->active_transaction())
+    if (thd->active_transaction())
     {
       my_message(ER_LOCK_OR_ACTIVE_TRANSACTION,
                  ER(ER_LOCK_OR_ACTIVE_TRANSACTION), MYF(0));

-- 
kostja
Thread
bzr commit into mysql-6.0-backup branch (ingo.struewing:2825) Bug#20667Ingo Struewing19 Jun
  • Re: bzr commit into mysql-6.0-backup branch (ingo.struewing:2825)Bug#20667Konstantin Osipov16 Jul
    • Re: bzr commit into mysql-6.0-backup branch (ingo.struewing:2825)Bug#20667Konstantin Osipov17 Jul