Hi,
The patch looks ok. I have one request to remove redundant code change and
another to protect code with "#ifndef DBUG_OFF". Good to push if these requests
are implemented.
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-02-28 12:45:00-05:00, cbell@mysql_laptop. +4 -0
> BUG#33572 : Backup: no backup of merge tables
>
> Patch changes default driver to reject merge tables. Also changes
> to kernel to fix error condition for no driver error. Added new
> test to backup_errors test.
>
> mysql-test/r/backup_errors.result@stripped, 2008-02-28 12:44:42-05:00,
> cbell@mysql_laptop. +21 -0
> BUG#33572 : Backup: no backup of merge tables
>
> New result file.
>
> mysql-test/t/backup_errors.test@stripped, 2008-02-28 12:44:43-05:00,
> cbell@mysql_laptop. +36 -0
> BUG#33572 : Backup: no backup of merge tables
>
> New test added to the file.
>
> sql/backup/be_default.h@stripped, 2008-02-28 12:44:44-05:00, cbell@mysql_laptop. +5 -2
> BUG#33572 : Backup: no backup of merge tables
>
> Changed the default accept() method to reject merge tables.
>
> sql/backup/kernel.cc@stripped, 2008-02-28 12:44:45-05:00, cbell@mysql_laptop. +10 -4
> BUG#33572 : Backup: no backup of merge tables
>
> Changed kernel to properly handle the error when a table
> is rejected by all known drivers.
>
> diff -Nrup a/mysql-test/r/backup_errors.result b/mysql-test/r/backup_errors.result
> --- a/mysql-test/r/backup_errors.result 2008-02-15 15:44:59 -05:00
> +++ b/mysql-test/r/backup_errors.result 2008-02-28 12:44:42 -05:00
> @@ -95,3 +95,24 @@ Restoring the table
> CREATE TABLE mysql.online_backup_progress LIKE test.obp_copy;
> DROP TABLE test.obp_copy;
> DROP DATABASE test_ob_error;
> +Create database.
> +CREATE DATABASE test_merge;
> +Create base tables.
> +CREATE TABLE test_merge.t1 (a int, b char (10)) ENGINE=MYISAM;
> +CREATE TABLE test_merge.t2 (a int, b char (10)) ENGINE=MYISAM;
> +CREATE TABLE test_merge.t3 (a int, b char (10)) ENGINE=MYISAM;
> +CREATE TABLE test_merge.t4 (a int, b char (10)) ENGINE=MYISAM;
> +Insert data.
> +INSERT INTO test_merge.t1 VALUES (11, 'from t1'), (21, 'from t1'), (31, 'from t1'),
> (41, 'from t1');
> +INSERT INTO test_merge.t2 VALUES (12, 'from t2'), (22, 'from t2'), (32, 'from t2'),
> (42, 'from t2');
> +INSERT INTO test_merge.t3 VALUES (13, 'from t3'), (23, 'from t3'), (33, 'from t3'),
> (43, 'from t3');
> +INSERT INTO test_merge.t4 VALUES (14, 'from t4'), (24, 'from t4'), (34, 'from t4'),
> (44, 'from t4');
> +Create merge table.
> +CREATE TABLE test_merge.tmerge (a int, b char (10))
> +ENGINE=MERGE UNION=(test_merge.t1, test_merge.t2, test_merge.t3, test_merge.t4)
> +INSERT_METHOD=LAST;
> +Attempt backup.
> +BACKUP DATABASE test_merge TO 'test_merge.bak';
> +ERROR HY000: Can't find backup driver for table test_merge.tmerge
> +Cleanup
> +DROP DATABASE test_merge;
> diff -Nrup a/mysql-test/t/backup_errors.test b/mysql-test/t/backup_errors.test
> --- a/mysql-test/t/backup_errors.test 2008-02-15 15:44:59 -05:00
> +++ b/mysql-test/t/backup_errors.test 2008-02-28 12:44:43 -05:00
> @@ -209,3 +209,39 @@ CREATE TABLE mysql.online_backup_progres
> DROP TABLE test.obp_copy;
>
> DROP DATABASE test_ob_error;
> +
> +#
> +# BUG#33572 Test backup of merge table -- should fail with no engine
> +# supported error.
> +#
> +
> +--echo Create database.
> +CREATE DATABASE test_merge;
> +
> +--echo Create base tables.
> +CREATE TABLE test_merge.t1 (a int, b char (10)) ENGINE=MYISAM;
> +CREATE TABLE test_merge.t2 (a int, b char (10)) ENGINE=MYISAM;
> +CREATE TABLE test_merge.t3 (a int, b char (10)) ENGINE=MYISAM;
> +CREATE TABLE test_merge.t4 (a int, b char (10)) ENGINE=MYISAM;
> +
> +--echo Insert data.
> +INSERT INTO test_merge.t1 VALUES (11, 'from t1'), (21, 'from t1'), (31, 'from t1'),
> (41, 'from t1');
> +INSERT INTO test_merge.t2 VALUES (12, 'from t2'), (22, 'from t2'), (32, 'from t2'),
> (42, 'from t2');
> +INSERT INTO test_merge.t3 VALUES (13, 'from t3'), (23, 'from t3'), (33, 'from t3'),
> (43, 'from t3');
> +INSERT INTO test_merge.t4 VALUES (14, 'from t4'), (24, 'from t4'), (34, 'from t4'),
> (44, 'from t4');
> +
> +--echo Create merge table.
> +CREATE TABLE test_merge.tmerge (a int, b char (10))
> +ENGINE=MERGE UNION=(test_merge.t1, test_merge.t2, test_merge.t3, test_merge.t4)
> +INSERT_METHOD=LAST;
> +
> +--echo Attempt backup.
> +--error ER_BACKUP_NO_BACKUP_DRIVER
> +BACKUP DATABASE test_merge TO 'test_merge.bak';
> +
> +--echo Cleanup
> +DROP DATABASE test_merge;
> +
> +--error 0,1
> +--remove_file $MYSQLTEST_VARDIR/master-data/test_merge.bak
> +
OK, I think backup_errors.test is a good place to put this tests in.
> diff -Nrup a/sql/backup/be_default.h b/sql/backup/be_default.h
> --- a/sql/backup/be_default.h 2007-12-03 15:28:13 -05:00
> +++ b/sql/backup/be_default.h 2008-02-28 12:44:44 -05:00
> @@ -223,8 +223,11 @@ class Default_snapshot: public Snapshot_
> const char* name() const
> { return "Default"; }
>
> - bool accept(const Table_ref&, const ::handlerton*)
> - { return TRUE; }; // accept all tables
> + bool accept(const Table_ref&, const ::handlerton* h)
> + {
> + // accept all tables except merge tables until merge driver available
> + return (h->db_type != DB_TYPE_MRG_MYISAM) ? TRUE : FALSE;
> + };
>
OK.
> result_t get_backup_driver(Backup_driver* &ptr)
> { return (ptr= new default_backup::Backup(m_tables,::current_thd,
> diff -Nrup a/sql/backup/kernel.cc b/sql/backup/kernel.cc
> --- a/sql/backup/kernel.cc 2008-02-15 10:57:49 -05:00
> +++ b/sql/backup/kernel.cc 2008-02-28 12:44:45 -05:00
> @@ -561,11 +561,16 @@ int Backup_info::find_backup_engine(cons
> for (no=1; no >=0; --no)
> {
> if (!m_snap[no]->accept(tbl,hton))
> - continue;
> + {
> + if (no == 0) // if default doesn't accept, it's an error
> + goto error;
> + continue;
> + }
>
> DBUG_RETURN(no);
> }
>
> +error:
> report_error(ER_BACKUP_NO_BACKUP_DRIVER,tbl.describe(buf,sizeof(buf)));
> DBUG_RETURN(-1);
> }
This change is not needed. The logic, as it is now, looks as follows:
for (no=1; no >=0; --no)
{
if (!m_snap[no]->accept(tbl,hton))
continue;
DBUG_RETURN(no);
}
report_error(ER_BACKUP_NO_BACKUP_DRIVER,tbl.describe(buf,sizeof(buf)));
DBUG_RETURN(-1);
If neither the CS snapshot (located at m_snap[1]) nor the default one (located
at m_snap[0]) accepts the table, the for loop will finish after two iterations
and NO_BACKUP_DRIVER error will be reported.
> @@ -1015,9 +1020,10 @@ Backup_info::add_table(Db_item &dbi, con
>
> int no= find_backup_engine(tl->table,t); // Note: reports errors
>
> - DBUG_PRINT("backup",(" table %s backed-up with %s engine",
> - t.describe(buf),
> - m_snap[no]->name()));
> + if (no >= 0)
> + DBUG_PRINT("backup",(" table %s backed-up with %s engine",
> + t.describe(buf),
> + m_snap[no]->name()));
>
Good catch - trying to access m_snap[-1] would crash server most probably. Since
this is only relevant in debug build, the whole "if" construct should be guarded
by "#ifndef DBUG_OFF", I think.
> /*
> If error debugging is switched on (see debug.h) then any table whose
>
>
