Re: bzr commit into mysql-6.0-backup branch (jorgen.loland:2746) Bug#38426

[Rafal Somla]

Hi Jorgen,

STATUS
------
Patch approved.

COMMENTARY
----------
See minor remarks/suggestions below.

Jorgen Loland wrote:
> #At file:///localhome/jl208045/mysql/mysql-6.0-backup-38426/
> 
>  2746 Jorgen Loland	2008-11-28
>       Bug#38426 - Backup of partitioned Falcon table will not use snapshot driver
>             
>       Before, the Default backup driver was used when backing up a partitioned table.
> However, the Snapshot driver is capable of backing up partitioned tables if the underlying
> storage engine is InnoDB or Falcon.
>       
>       With this patch, the Snapshot backup driver is used if the underlying storage
> engine of a partitioned table is InnoDB or Falcon
> added:
>   mysql-test/suite/backup_engines/r/backup_partition.result
>   mysql-test/suite/backup_engines/t/backup_partition.test
> modified:
>   sql/backup/backup_aux.h
>   sql/backup/backup_info.cc
> 
> per-file messages:
>   mysql-test/suite/backup_engines/r/backup_partition.result
>     Test for checking that backup of partitioned tables uses correct backup driver
>   mysql-test/suite/backup_engines/t/backup_partition.test
>     Test for checking that backup of partitioned tables uses correct backup driver
>   sql/backup/backup_aux.h
>     Add inline method get_hton_se which returns storage engine type of a handlerton
>   sql/backup/backup_info.cc
>     Modify method get_storage_engine to return underlying storage engine of
> partitioned tables iff the storage engine is InnoDB or Falcon.
...
> === added file 'mysql-test/suite/backup_engines/t/backup_partition.test'
> --- a/mysql-test/suite/backup_engines/t/backup_partition.test	1970-01-01 00:00:00
> +0000
> +++ b/mysql-test/suite/backup_engines/t/backup_partition.test	2008-11-28 09:05:46
> +0000
> @@ -0,0 +1,68 @@
> +#
> +# This test is designed to test that BACKUP selects the best available
> +# driver for partitioned tables. The storage engines should use these
> +# drivers:
> +#
> +#   Partitioned Falcon table      -> Snapshot
> +#   Partitioned InnoDB table      -> Snapshot
> +#   All other partitioned tables  -> Default
> +#
> +
> +# Find expected backup driver for the current storage engine.
> +
> +let $exp_drv= Default;
> +if (`select @@storage_engine = 'falcon'`) { let $exp_drv= Snapshot; }
> +if (`select @@storage_engine = 'innodb'`) { let $exp_drv= Snapshot; }
> +
> +# Uncomment the line below and run the test with --force flag
> +# if you want to verify that expected driver is correct
> +#echo $exp_drv;
> +
> +--echo #
> +--echo # Creating databases
> +--echo #
> +CREATE DATABASE backup_part;
> +
> +CREATE TABLE backup_part.t1 (
> +  t1_autoinc INTEGER NOT NULL AUTO_INCREMENT,
> +  t1_uuid CHAR(36),
> +  PRIMARY KEY (t1_autoinc)
> +);
> +ALTER TABLE backup_part.t1 PARTITION BY HASH(t1_autoinc);
> +
> +INSERT INTO backup_part.t1(t1_uuid) VALUES ('aaa'),('bbb'),('ccc'),('ddd'); 
> +
> +--echo # Show content
> +SELECT * FROM backup_part.t1 ORDER BY t1_autoinc;
> +
> +--echo #
> +--echo # Perform backup
> +--replace_column 1 #
> +let $bup_id= `BACKUP DATABASE backup_part TO 'partition.bak'`;
> +
> +--echo #
> +--echo # Show drivers. 
> +
> +--echo
> +# Count of expected driver for this backup_id should be 1
> +# Replace exp_drv and backup_id in query because it will vary
> +replace_regex /'.*'.*/'FILTERED DRIVER' AND backup_id=FILTERED ID/ ;
> +--eval SELECT count(*) FROM mysql.backup_history WHERE drivers LIKE '$exp_drv' AND
> backup_id=$bup_id

You could do it without "replace_regex" as follows:

--eval SELECT drivers LIKE '$exp_drv' FROM mysql.backup_history
                                       WHERE backup_id=$bup_id;

or even:

--eval SELECT drivers = '$exp_drv' FROM mysql.backup_history
                                    WHERE backup_id=$bup_id;

> +--echo
> +
> +--echo #
> +--echo # Restore and show content
> +--echo #  
> +
> +--replace_column 1 #
> +RESTORE FROM 'partition.bak' OVERWRITE;
> +--echo
> +SELECT * FROM backup_part.t1 ORDER BY t1_autoinc;
> +
> +--echo #
> +--echo # Cleanup
> +--echo #  
> +
> +DROP DATABASE backup_part;
> +
> +--remove_file $MYSQLTEST_VARDIR/master-data/partition.bak
...
> === modified file 'sql/backup/backup_info.cc'
> --- a/sql/backup/backup_info.cc	2008-11-05 09:41:15 +0000
> +++ b/sql/backup/backup_info.cc	2008-11-28 09:05:46 +0000
> @@ -8,6 +8,7 @@
>  */
>  
>  #include "../mysql_priv.h"
> +#include "../ha_partition.h"
>  
>  #include "backup_info.h"
>  #include "backup_kernel.h"
> @@ -34,6 +35,50 @@ storage_engine_ref get_storage_engine(TH
>    if (table)
>    {
>      se= plugin_ref_to_se_ref(table->s->db_plugin);
> +
> +  /*
> +    Further check for underlying storage engine is needed
> +    if table is partitioned
> +  */
> +
> +  storage_engine_ref se_tmp= NULL;
> +
> +  if (table->part_info)
> +    {

Wrong indentation: { shuld be directly below if

> +      partition_info *p_info=  table->part_info;
> +      List_iterator<partition_element> p_it(p_info->partitions);
> +      partition_element *p_el;
> +      
> +      while ((p_el= p_it++))
> +        {
> +          if (!se_tmp)
> +            {
> +              se_tmp= get_hton_se(p_el->engine_type);
> +              ::handlerton *h= se_hton(se_tmp);
> +
> +              /* 
> +                 Only snapshot driver supports partitioning. Use
> +                 default driver for all non-MVCC storage engines.
> +               */
> +              if (h->start_consistent_snapshot == NULL) 
> +              {
> +                se_tmp= NULL;

The assignment is redundant.

> +                goto close;
> +              }
> +
> +              continue;
> +            }
> +
> +          // use default driver if partitions have different storage engines
> +          if (se_tmp != get_hton_se(p_el->engine_type))
> +            goto close;
> +        };
> +      
> +      se= se_tmp;
> +    }
> +
> +  close:
> +  
>      ::intern_close_table(table);
>      my_free(table, MYF(0));
>    }
> 
> 

Rafal