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

[Øystein Grøvlen]

Chuck,

Thanks for the review.  I now see what I have misunderstood.  Each 
backup engine has its own restore class.  Hence, I need to verify that 
this works correctly for restore of backups using different engines.
(I first thought that you wanted me to address commit after backup also 
in this patch.  Can you confirm that this is not the case?)

More comments/questions inline.

Thanks,

--
Øystein

Chuck Bell wrote:
> 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.

Will fix.

> 
> ...
> 
>> 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.
> 

OK. Is this still in the context of verifying that a rollback after 
restore does not undo the restore operation, or are you thinking of a 
broader test scope?

> ...
> 
>> 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);
>   } 

I will do this.  (I just ask myself what not all this could have been 
done within the end_active_trans function).  What about testing the 
return values of the functions I call for errors.  Seems like if the 
commit fails, this will silently be ignored.

> 
> 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.

I understand.  I guess I need to find out where the end functions are 
called from.

> 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:

Thanks for the tip.  I will look into this.

> 
> ===== 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?" 

Why not move the new code outside the "if (table_list)"?  Is there any 
problems with that?

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

Since MyISAM does not support transaction, I guess testing will not show 
any problems.

Thanks again,

-- 
Øystein