RE: bk commit into 6.0 tree (og136792:1.2622) BUG#34210

[Chuck Bell]

Oystein,

I think we need some changes to this patch. See below. Portions removed for
space consideration. 

...

>   mysql-test/t/backup_commit_restore.test@stripped, 2008-05-28 
> 13:46:31+02:00, og136792@stripped +47 -0
>     Test that restore is automatically committed (i.e., that 
> it is not rolled
>     back.  Tests added for both InnoDB and Falcon, but Falcon 
> test is commented
>     out because of bug 34205.

Missing ending ) from comment.

...

> diff -Nrup a/mysql-test/t/backup_commit_restore.test 
> b/mysql-test/t/backup_commit_restore.test
> --- /dev/null	Wed Dec 31 16:00:00 196900
> +++ b/mysql-test/t/backup_commit_restore.test	2008-05-28 
> 13:46:31 +02:00
> @@ -0,0 +1,47 @@
> +--source include/have_innodb.inc
> +--source include/not_embedded.inc
> +
> +# Test that it is not possible to rollback restore.  That 
> is, that an # 
> +automatic commit is performed as part of restore.
> +# This test the behavior reported in BUG#34210
> +
> +CREATE DATABASE commit_test;
> +
> +# Test for InnoDB
> +USE commit_test;
> +SET @@autocommit=0;
> +CREATE TABLE t (s1 CHAR(1)) ENGINE=innodb; INSERT INTO t 
> VALUES ('a'); 
> +COMMIT;
> +
> +replace_column 1 #;
> +BACKUP DATABASE commit_test TO '82';
> +
> +replace_column 1 #;
> +RESTORE FROM '82';
> +
> +SELECT * FROM t;
> +ROLLBACK;
> +SELECT * FROM t;
> +COMMIT;
> +

I think this test should include tests for:

1) running autocommit=0 with only the CS driver, both CS and default driver,
and a native driver (see below)
2) running tests with autocommit=1 after the autocommit=0 tests
3) running backup with autocommit=0 but turn it back on before restore
(maybe). Basically, mix and match autocommit settings to make sure it all
works as expected.

...

> diff -Nrup a/sql/backup/be_default.cc b/sql/backup/be_default.cc
> --- a/sql/backup/be_default.cc	2008-05-14 18:28:30 +02:00
> +++ b/sql/backup/be_default.cc	2008-05-28 13:46:30 +02:00
> @@ -622,6 +622,8 @@ result_t Restore::truncate_table(TABLE *  
> result_t Restore::end()  {
>    DBUG_ENTER("Restore::end");
>    close_thread_tables(m_thd);
>    DBUG_RETURN(OK);
>  }

This code needs to be guarded like other parts of the code. For example:

  if (thd->options & (OPTION_NOT_AUTOCOMMIT | OPTION_BEGIN))
  {
    ha_autocommit_or_rollback(m_thd, 0);
    end_active_trans(m_thd);
  } 

Also, placing the code in this location forces it to be called twice if you
use both CS and default drivers. While this may not have any side effects,
the code should only be called once.

Which brings me to another obscure point about the default and CS drivers
WRT restore... The open_and_lock_tables() call for these drivers is called
once using a combined table list in data_backup.cc @line#1420. I would
recommend placing the patched code in data_backup.cc @line#1597 after the
close_thread_tables() for the default and CS drivers and remove it from
be_default.cc as shown:

===== data_backup.cc 1.20 vs edited =====
--- 1.20/sql/backup/data_backup.cc      2008-05-05 11:03:15 -04:00
+++ edited/data_backup.cc       2008-05-28 11:45:37 -04:00
@@ -1595,7 +1595,19 @@
     Close all tables if default or snapshot driver used.
   */
   if (table_list)
-    close_thread_tables(::current_thd);
+  {
+    THD *thd= ::current_thd;
+    close_thread_tables(thd);
+    /*
+      If autocommit is turned off, be sure to commit the transaction.
+    */
+    if (thd->options & (OPTION_NOT_AUTOCOMMIT | OPTION_BEGIN))
+    {
+      ha_autocommit_or_rollback(thd, 0);
+      end_active_trans(thd);
+    }
+  }
+

One more thing: "What if a restore uses only a native driver (thus no
default or CS driver and thus no call to ha_auto...)? Does this bug still
apply?" 

We should check it with the mysql-6.0-backup-myisam code.

Chuck