bzr commit into MySQL/Maria:mysql-maria branch (guilhem:2659) Bug#37873

[Guilhem Bichot]

#At bzr+ssh://bk-internal.mysql.com/bzrroot/server/mysql-maria/

 2659 Guilhem Bichot	2008-07-09
      Fix for BUG#37873 "Client gets ok from INSERT VALUES before commit record is on disk":
      this was true also for INSERT SELECT, UPDATE, DELETE. This fix will be removed when Maria supports COMMIT/ROLLBACK
      ("Maria 2.0"), because then Maria will trans_register_ha() and so participate in ha_commit_trans().
      Without the fix, the assertion added to ha_maria::external_lock() fires many times in *maria*.test.
modified:
  sql/sql_parse.cc
  storage/maria/ha_maria.cc
  storage/maria/ha_maria.h

per-file messages:
  sql/sql_parse.cc
    Before sending OK to client, commit transaction in Maria.
  storage/maria/ha_maria.cc
    Assertion added to external_lock() when committing: OK should not have been sent yet (ACI*D*).
    In ha_maria::implicit_commit(), possibility to not create a new transaction, this is used
    before net_end_statement(), when the next step is close_thread_tables();
    in this use case, ha_maria::implicit_commit() should not commit if under LOCK TABLES.
  storage/maria/ha_maria.h
    new prototype
=== modified file 'sql/sql_parse.cc'
--- a/sql/sql_parse.cc	2008-07-09 12:07:38 +0000
+++ b/sql/sql_parse.cc	2008-07-09 20:47:46 +0000
@@ -127,7 +127,7 @@ bool end_active_trans(THD *thd)
     if (ha_commit(thd))
       error=1;
 #ifdef WITH_MARIA_STORAGE_ENGINE
-    ha_maria::implicit_commit(thd);
+    ha_maria::implicit_commit(thd, TRUE);
 #endif
   }
   thd->options&= ~(OPTION_BEGIN | OPTION_KEEP_LOG);
@@ -1132,6 +1132,10 @@ bool dispatch_command(enum enum_server_c
     {
       char *beginning_of_next_stmt= (char*) end_of_stmt;
 
+#ifdef WITH_MARIA_STORAGE_ENGINE
+      ha_maria::implicit_commit(thd, FALSE);
+#endif
+
       net_end_statement(thd);
       query_cache_end_of_result(thd);
       /*
@@ -1496,6 +1500,10 @@ bool dispatch_command(enum enum_server_c
     thd->mysys_var->abort= 0;
   }
 
+#ifdef WITH_MARIA_STORAGE_ENGINE
+  ha_maria::implicit_commit(thd, FALSE);
+#endif
+
   net_end_statement(thd);
   query_cache_end_of_result(thd);
 

=== modified file 'storage/maria/ha_maria.cc'
--- a/storage/maria/ha_maria.cc	2008-07-09 12:07:38 +0000
+++ b/storage/maria/ha_maria.cc	2008-07-09 20:47:46 +0000
@@ -2342,6 +2342,12 @@ int ha_maria::external_lock(THD *thd, in
       {
         if (!trnman_decrement_locked_tables(trn))
         {
+          /*
+            OK should not have been sent to client yet (ACID).
+            This is a bit excessive, ACID requires this only if there are some
+            changes to commit (rollback shouldn't be tested).
+          */
+          DBUG_ASSERT(!thd->main_da.is_sent);
           /* autocommit ? rollback a transaction */
 #ifdef MARIA_CANNOT_ROLLBACK
           if (ma_commit(trn))
@@ -2403,9 +2409,14 @@ int ha_maria::start_stmt(THD *thd, thr_l
   be participant in the connection's transaction and so the implicit commits
   (ha_commit()) (like in end_active_trans()) will do the implicit commit
   without need to call this function which can then be removed.
+
+  @param  thd              THD object
+  @param  new_trn          if a new transaction should be created; a new
+                           transaction is not needed when we know that the
+                           tables will be unlocked very soon.
 */
 
-int ha_maria::implicit_commit(THD *thd)
+int ha_maria::implicit_commit(THD *thd, bool new_trn)
 {
 #ifndef MARIA_CANNOT_ROLLBACK
 #error this method should be removed
@@ -2414,11 +2425,34 @@ int ha_maria::implicit_commit(THD *thd)
   int error= 0;
   TABLE *table;
   DBUG_ENTER("ha_maria::implicit_commit");
+  if (!new_trn && thd->locked_tables)
+  {
+    /*
+      "we are under LOCK TABLES" <=> "we shouldn't commit".
+      As thd->locked_tables is true, we are either under LOCK TABLES, or in
+      prelocking; prelocking can be under LOCK TABLES, or not (and in this
+      latter case only we should commit).
+      Note that we come here only at the end of the top statement
+      (dispatch_command()), we are never committing inside a sub-statement./
+    */
+    enum prelocked_mode_type prelocked_mode= thd->prelocked_mode;
+    if ((prelocked_mode == NON_PRELOCKED) ||
+        (prelocked_mode == PRELOCKED_UNDER_LOCK_TABLES))
+    {
+      DBUG_PRINT("info", ("locked_tables, skipping"));
+      DBUG_RETURN(0);
+    }
+  }
   if ((trn= THD_TRN) != NULL)
   {
     uint locked_tables= trnman_has_locked_tables(trn);
     if (unlikely(ma_commit(trn)))
       error= 1;
+    if (!new_trn)
+    {
+      THD_TRN= NULL;
+      goto end;
+    }
     /*
       We need to create a new transaction and put it in THD_TRN. Indeed,
       tables may be under LOCK TABLES, and so they will start the next
@@ -2458,6 +2492,7 @@ int ha_maria::implicit_commit(THD *thd)
       }
     }
   }
+end:
   DBUG_RETURN(error);
 }
 

=== modified file 'storage/maria/ha_maria.h'
--- a/storage/maria/ha_maria.h	2008-06-28 08:27:14 +0000
+++ b/storage/maria/ha_maria.h	2008-07-09 20:47:46 +0000
@@ -161,5 +161,5 @@ public:
   {
     return file;
   }
-  static int implicit_commit(THD *thd);
+  static int implicit_commit(THD *thd, bool new_trn);
 };