Re: bk commit into 6.0 tree (cbell:1.2605) BUG#33569

[Rafal Somla]

Chuck,

This is a very nice small patch but I'm afraid it won't work with the post 
WL#4212 code :(

The reason is that the WL#4212 refactoring consequently leaves all "low level" 
operations on objects such as tables to the si_objects functions. As a 
consequence, Backup_info::add_table() method doesn't open the table any more - 
you don't have access to open TABLE structure thus not to the handler and then 
you can't call get_tablespace_name() method...

I know it hurts but I suggest that this fix be re-designed and re-implemented 
based on the current, post WL#4212 code. Actually one can't avoid doing this - 
the issue will surface when merging with the current 6.0-backup tree.

As for the design, it is not clear how to do it best. An obvious solution would 
be to delegate the task of finding table's tablespace to a new si_objects 
function (as proposed in the bug report).

But one can quickly realize that in order to do that, the table would have to be 
opened temporarily. And then it is also opened for other purposes - for example 
to determine its storage engine and check if it has native backup. It starts to 
look like we are opening and closing a single table several times to collect 
different pieces of information about it - this is not very elegant and 
efficient. That makes me think that, perhaps, si_object should contain a single 
function for collecting all needed information about a table. Something 
analogous to stat() function for files.

I would imagine that si_objects is extended with, for example, get_table_info() 
function which will open a table, get all relevant information such as its 
storage engine, tablespace and so on, store it in some dedicated structure and 
then close the table. This way we could avoid opening and closing the table 
several times.

Having written that, I realize that the same problem occurs in other si_objects 
functions already. In order to obtain a CREATE statement for a table we open it 
temporarily, to get view dependencies we open it as well. So the existing 
si_objects implementation is not terribly efficient in that respect. So, perhaps 
one can go for the simple solution and ignore the fact that we open and close 
tables several times (as we do it now).

Rafal

cbell@stripped wrote:
> Below is the list of changes that have just been committed into a local
> 6.0 repository of cbell.  When cbell does a push these changes
> will be propagated to the main repository and, within 24 hours after the
> push, to the public repository.
> For information on how to access the public repository
> see http://dev.mysql.com/doc/mysql/en/installing-source-tree.html
> 
> ChangeSet@stripped, 2008-03-22 21:20:58-04:00, cbell@mysql_cab_desk. +4 -0
>   BUG#33569 Backup: tablespace not restored 
>   
>   Added code to error on backup if table included in the backup
>   is a falcon table with a tablespace specified.
> 
>   mysql-test/r/backup_tablespace.result@stripped, 2008-03-22 21:20:46-04:00,
> cbell@mysql_cab_desk. +11 -0
>     BUG#33569 Backup: tablespace not restored 
>     
>     New result file.
> 
>   mysql-test/r/backup_tablespace.result@stripped, 2008-03-22 21:20:46-04:00,
> cbell@mysql_cab_desk. +0 -0
> 
>   mysql-test/t/backup_tablespace.test@stripped, 2008-03-22 21:20:46-04:00,
> cbell@mysql_cab_desk. +35 -0
>     BUG#33569 Backup: tablespace not restored 
>     
>     New test for tablespace error on backup.
> 
>   mysql-test/t/backup_tablespace.test@stripped, 2008-03-22 21:20:46-04:00,
> cbell@mysql_cab_desk. +0 -0
> 
>   sql/backup/kernel.cc@stripped, 2008-03-22 21:20:44-04:00, cbell@mysql_cab_desk. +11 -0
>     BUG#33569 Backup: tablespace not restored 
>     
>     Added code to check to see if table being backed up is a falcon
>     table and if it specifies a tablespace. Error if both are true.
> 
>   sql/share/errmsg.txt@stripped, 2008-03-22 21:20:45-04:00, cbell@mysql_cab_desk. +3 -0
>     BUG#33569 Backup: tablespace not restored 
>     
>     Added new error message.
> 
> diff -Nrup a/mysql-test/r/backup_tablespace.result
> b/mysql-test/r/backup_tablespace.result
> --- /dev/null	Wed Dec 31 16:00:00 196900
> +++ b/mysql-test/r/backup_tablespace.result	2008-03-22 21:20:46 -04:00
> @@ -0,0 +1,11 @@
> +Create the database, tablespace, and table and add data.
> +CREATE DATABASE test_falcon_ts;
> +CREATE TABLESPACE f_ts ADD DATAFILE 'f_ts.dat' USE LOGFILE GROUP f_ts
> ENGINE=FALCON;
> +CREATE TABLE test_falcon_ts.t1 (col_a INT) TABLESPACE f_ts ENGINE=FALCON;
> +INSERT INTO test_falcon_ts.t1 VALUES (1), (10), (100), (200), (300);
> +Attempt backup - should fail.
> +BACKUP DATABASE test_falcon_ts TO 'test_falcon_ts.bak';
> +ERROR HY000: Backup of tables with tablespaces is not supported. Table:
> test_falcon_ts.t1.
> +Cleanup: drop database, tablespace, and remove files (if exists).
> +DROP DATABASE test_falcon_ts;
> +DROP TABLESPACE f_ts ENGINE=FALCON;
> diff -Nrup a/mysql-test/t/backup_tablespace.test
> b/mysql-test/t/backup_tablespace.test
> --- /dev/null	Wed Dec 31 16:00:00 196900
> +++ b/mysql-test/t/backup_tablespace.test	2008-03-22 21:20:46 -04:00
> @@ -0,0 +1,35 @@
> +--source include/not_embedded.inc
> +--source include/have_falcon.inc
> +
> +#
> +# This test file contains tests for backing up and restoring tablespaces.
> +# Tablespaces are not supported yet so we just have a test for error
> +# condition.
> +#
> +# TODO: Add tests when WL#4240 is implemented.
> +
> +#
> +# BUG#33569 Test for tablespaces for falcon.
> +#
> +
> +--echo Create the database, tablespace, and table and add data.
> +CREATE DATABASE test_falcon_ts;
> +CREATE TABLESPACE f_ts ADD DATAFILE 'f_ts.dat' USE LOGFILE GROUP f_ts
> ENGINE=FALCON;
> +CREATE TABLE test_falcon_ts.t1 (col_a INT) TABLESPACE f_ts ENGINE=FALCON;
> +
> +INSERT INTO test_falcon_ts.t1 VALUES (1), (10), (100), (200), (300);
> +
> +--echo Attempt backup - should fail.
> +--error ER_BACKUP_TABLESPACE_ERROR
> +BACKUP DATABASE test_falcon_ts TO 'test_falcon_ts.bak';
> +
> +--echo Cleanup: drop database, tablespace, and remove files (if exists).
> +--error 0,1
> +--remove_file $MYSQLTEST_VARDIR/master-data/test_falcon_ts.bak
> +
> +DROP DATABASE test_falcon_ts; 
> +DROP TABLESPACE f_ts ENGINE=FALCON;
> +
> +--error 0,1
> +--remove_file $MYSQLTEST_VARDIR/master-data/f_ts.dat
> +
> diff -Nrup a/sql/backup/kernel.cc b/sql/backup/kernel.cc
> --- a/sql/backup/kernel.cc	2008-03-20 10:53:11 -04:00
> +++ b/sql/backup/kernel.cc	2008-03-22 21:20:44 -04:00
> @@ -996,6 +996,17 @@ Backup_info::add_table(Db_item &dbi, con
>    }
>  
>    /*
> +    Check to see if table uses tablespace. If it does, throw
> +    error and stop.
> +  */
> +  if ((tl->table->s->db_type()->db_type == DB_TYPE_FALCON) &&
> +      tl->table->file->get_tablespace_name()) 

I wonder why to constrain the check only to Falcon tables? Since we have the 
general get_tablespace_name() function in the handler, we can do the check for 
any table.

> +  {
> +    report_error(ER_BACKUP_TABLESPACE_ERROR, entry.db, entry.table_name);
> +    return NULL;
> +  }
> +
> +  /*
>      alternative way of opening a single tmp table - but it
>      doesn't initialize TABLE_LIST structure which we need for getting
>      CREATE statement.
> diff -Nrup a/sql/share/errmsg.txt b/sql/share/errmsg.txt
> --- a/sql/share/errmsg.txt	2008-03-20 10:53:12 -04:00
> +++ b/sql/share/errmsg.txt	2008-03-22 21:20:45 -04:00
> @@ -6251,3 +6251,6 @@ ER_SLAVE_HEARTBEAT_VALUE_OUT_OF_RANGE
>  
>  ER_BACKUP_LOG_WRITE_ERROR
>          eng "Can't write to the online backup progress log %-.64s."
> +ER_BACKUP_TABLESPACE_ERROR
> +        eng "Backup of tables with tablespaces is not supported. Table: %s.%s."
> +
> 
>