Re: bk commit into 5.1 tree (cbell:1.2540)

[Rafal Somla]

Chuck,

I've checked this one. It looks OK. Please check my comments below.

Rafal

cbell@stripped wrote:
(...)
> --- New file ---
> +++ mysql-test/t/backup_snapshot.test	07/06/21 10:56:37
(...)
> 
> # While a consistent snapshot backup is executed,
> # no external inserts should be visible to the transaction.

While doing proper testing of the on-line nature of the backup will require 
other techniques, it is not so difficult to modify the test so that inserts have 
a chance to run in parallel with backup - see below.

> 
> BACKUP DATABASE bup_snapshot TO "bup_snapshot.bak";

Replace above with

--send BACKUP DATABASE bup_snapshot TO "bup_snapshot.bak";

> 
> connection con2;
> 
> INSERT INTO bup_snapshot.t1 VALUES("- Dave Mathews");
> INSERT INTO bup_snapshot.t1 VALUES("- Yes");
> INSERT INTO bup_snapshot.t1 VALUES("- Jethro Tull");
> DELETE FROM bup_snapshot.t1 WHERE word LIKE '10%';
> 
> SELECT * FROM bup_snapshot.t1 WHERE word LIKE '-%';
> SELECT COUNT(*) FROM bup_snapshot.t1;
> 
> connection con1;

Insert here

--reap

> 
> # Now restore the database and then check to make sure the new rows
> # were not backed up.
> 
> DROP TABLE bup_snapshot.t1;
> 
> RESTORE FROM "bup_snapshot.bak";
> 
> SELECT * FROM bup_snapshot.t1 WHERE word LIKE '-%';
> SELECT COUNT(*) FROM bup_snapshot.t1;
> 
> --disable_warnings
> DROP DATABASE IF EXISTS bup_snapshot;
> --enable_warnings
> 
> --exec rm $MYSQLTEST_VARDIR/master-data/bup_snapshot.bak
> 
> 
> 
> --- New file ---
> +++ sql/backup/be_snapshot.cc	07/06/21 10:56:37
(...)
> /**
>  * @brief Start backup process.
>  *
>  * This method locks all of the tables for reading.

It doesn't currently. Perhaps move the trivial definition to the header file.

>  *
>  * @retval backup::OK     all tables locked properly.
>  * @retval backup::Error  problem with locking tables.
>  */
> result_t Backup::begin(const size_t)
> {
>   DBUG_ENTER("Snapshot_backup::start_backup");
>   DBUG_RETURN(backup::OK);
> }
(...)
> result_t Backup::lock()
> {
>   DBUG_ENTER("Snapshot_backup::lock()");
>   /*
>     We must fool the locking code to think this is a select because
>     any other command type places the engine in a non-consistent read
>     state. 
>   */
>   m_thd->lex->sql_command= SQLCOM_SELECT; 
>   m_thd->lex->start_transaction_opt|=
> MYSQL_START_TRANS_OPT_WITH_CONS_SNAPSHOT;
>   begin_trans(m_thd);
>   DBUG_RETURN(backup::OK);
> }
> 

Shall/can we test result of begin_trans() to see if it was successfully started?

> result_t Backup::unlock()
> {
>   DBUG_ENTER("Snapshot_backup::unlock()");
>   DBUG_RETURN(backup::OK);
> }

If method definition is trivial, maybe better to put it in the header.

> --- New file ---
> +++ sql/backup/be_snapshot.h	07/06/21 10:56:37
> #ifndef _SNAPSHOT_BACKUP_H
> #define _SNAPSHOT_BACKUP_H
> 
> #include "archive.h"        
> #include "buffer_iterator.h"
> #include "be_default.h"
> 
> namespace snapshot_backup {
> 
> using backup::byte;
> using backup::result_t;
> using backup::version_t;
> using backup::Table_list;
> using backup::Table_ref;
> using backup::Buffer;
> 
> const uint META_SIZE= 8;

Are you using different size than in default_backup (which is 1)? If not, then I 
propose to import constant from default_backup namespace:

using default_backup::META_SIZE;

or

const uint META_SIZE= default_backup::META_SIZE;

> @@ -304,22 +312,37 @@
>  
>     #endif
>  
> -   // Point 4: try default backup engine.
> +   // Points 4 & 5: try consistent snapshot and default drivers..
>  
> -   if (default_image_no < 0)
> +   // try snapshot driver first
> +   int ino= snapshot_image_no;
> +   if (hton->start_consistent_snapshot != NULL)
>     {
> -     default_image_no= img_count;
> +     if (snapshot_image_no < 0) //image doesn't exist
> +     {
> +       ino= img_count;
> +       snapshot_image_no= img_count;
> +       images[snapshot_image_no]= new Snapshot_image(*this);
> +       img_count++;
> +       DBUG_PRINT("backup",("Snapshot image added to archive"));
> +     }
> +   }
> +   else
> +     ino= default_image_no; //now try default driver
>  
> +   if (ino < 0) //image doesn't exist
> +   {
> +     ino= img_count;
> +     default_image_no= img_count;
>       images[default_image_no]= new Default_image(*this);
> -     DBUG_PRINT("backup",("Default image added to archive"));
>       img_count++;
> +     DBUG_PRINT("backup",("Default image added to archive"));
>     }
>  
> -   img= images[default_image_no];
> +   img= images[ino];
>     DBUG_ASSERT(img);
> -
>     if (img->accept(tbl,hton))
> -    DBUG_RETURN(default_image_no);
> +     DBUG_RETURN(ino); // table accepted
>  
>     DBUG_PRINT("backup",("Table ignored."));
>     DBUG_RETURN(-1);

I checked the image selection logic and it should be OK. It has two "flaws" though:

- if CS driver rejects a table then the default driver is not tried after that 
(I know it should not happen).

- there is still a theoretical possibility that one of the created images is 
empty (haven't accepted any tables) and I'd like to take care of that in the future.