| List: | Commits | « Previous MessageNext Message » | |
| From: | Chuck Bell | Date: | January 22 2009 7:18pm |
| Subject: | Re: bzr commit into mysql-6.0-backup branch (Rafal.Somla:2741) Bug#32702 WL#4644 | ||
| View as plain text | |||
STATUS ------ Changes requested. REQUESTS -------- 1. The patch does not apply cleanly to bml.h or backup_logs.result. 2. The backup_errors test fails. 3. Inconsistent use of rename 'ddl_blocker' to 'bml_blocker'. 4. Unclear benefits of new 'prepared' state. 5. Move BML utility functions to si_objects. SUGGESTIONS ----------- 6. Please include the following renames in the patch comments. These must be applied to a branch before the patch can be applied using 'patch -p1 -u'. bzr mv mysql-test/suite/backup/r/backup_ddl_blocker.result mysql-test/suite/backup/r/backup_bml.result bzr mv mysql-test/suite/backup/t/backup_ddl_blocker.test mysql-test/suite/backup/t/backup_bml.test bzr mv sql/ddl_blocker.cc sql/bml.cc bzr mv sql/ddl_blocker.h sql/bml.h 7. Do not use 'online backup' in documentation, use 'MySQL Backup' instead. DETAILS ------- > 2741 Rafal Somla 2008-12-17 > BUG#32702 - Backup: DDL blocker does not block all it should > WL#4644 - Online Backup: Fix the DDL blocker > > After this patch, the DDL blocker will be renamed to Backup Metadata Lock and > will block all the statements listed in WL#4644. Test backup_ddl_blocker will > be > renamed to backup_bml and extended to test all these statements. > > This patch implements an alternative, simpler and I believe better solution > than > the one currently described in LLD. Namely, it marks all statements which need > > to be blocked by BML with special CF_BLOCKED_BY_BML flag. Then registering and > > unregistering marked statements with BML is done globally in > mysql_execute_command(). > > This makes changes in the parser code minimal, makes it easy to trace the list > > of statements which ale blocked and easily ensures that each bml_enter() call > is > matched with bml_leave(). [2] The backup_errors test fails. It looks like this was omitted from the patch. backup.backup_errors [ fail ] --- d:/source/bzr/mysql-6.0-review/mysql-test/suite/backup/r/backup_errors.resul t 2009-01-22 20:55:25.041149800 +0300 +++ d:\source\bzr\mysql-6.0-review\mysql-test\suite\backup\r\backup_errors.rejec t 2009-01-22 21:51:24.196349800 +0300 @@ -107,6 +107,7 @@ notes starting running +prepared Unknown database 'foo,bar' error # repeated database @@ -159,6 +160,7 @@ notes starting running +prepared Database 'mysql' cannot be included in a backup error Backup of mysql, information_schema scenario 2 @@ -181,6 +183,7 @@ notes starting running +prepared Database 'information_schema' cannot be included in a backup error Backup of mysql, information_schema scenario 3 @@ -203,6 +206,7 @@ notes starting running +prepared Database 'mysql' cannot be included in a backup error Backup of mysql, information_schema scenario 4 @@ -225,6 +229,7 @@ notes starting running +prepared Database 'mysql' cannot be included in a backup error Backup of mysql, information_schema scenario 5 @@ -254,6 +259,7 @@ notes starting running +prepared Database 'mysql' cannot be included in a backup error Making copies of progress tables. @@ -336,6 +342,7 @@ notes starting running +prepared Failed to obtain meta-data for trigger `db1`.`trg` error SET DEBUG_SYNC= 'reset'; mysqltest: Result content mismatch ... > === added directory 'mysql-test/suite/backup/include' > === added file 'mysql-test/suite/backup/include/bml_test.inc' > --- a/mysql-test/suite/backup/include/bml_test.inc 1970-01-01 00:00:00 +0000 > +++ b/mysql-test/suite/backup/include/bml_test.inc 2008-12-17 14:32:46 +0000 > @@ -0,0 +1,319 @@ > +# This is a "subroutine" for backup_ddl_blocker test. It executes DDL [3] Inconsistent use of renamed 'backup_ddl_blocker' -- please search and replace all occurrences of 'backup_ddl_blocker' with 'backup_bml_blocker'. > +# statements given by variables $DDL1-4 in parallel with BACKUP/RESTORE > +# statement and checks that they correctly block each other. > +# > +# If $backup_to has value, then BACKUP operation is performed and backup image > +# is stored at location $backup_to. > +# > +# If $restore_from has value, then RESTORE operation from the given image is > +# performed. > +# > +# To use this file the following conditions must be satisfied: > +# - five connections con1-5 must be set up, > +# - database bml_results must exist, > +# - procedure test.check_results(), used to check effects of DDLs must > +# be defined, > +# - If $restore_from is set, it must point at valid backup image. > +# > +# It is responsibility of the user of this file to populate buo_ddl_blocker [3] ddl_blocker? ... > === modified file 'mysql-test/suite/backup/r/backup_logs.result' > --- a/mysql-test/suite/backup/r/backup_logs.result 2008-11-17 09:57:51 +0000 > +++ b/mysql-test/suite/backup/r/backup_logs.result 2008-12-17 14:32:46 +0000 > @@ -78,6 +78,7 @@ SELECT notes FROM mysql.backup_progress > notes > starting > running > +prepared > con1: Let backup do the backup phase1. > SET DEBUG_SYNC= 'now SIGNAL backup WAIT_FOR validated'; > con1: Display progress [1] The patch does not apply cleanly to this file. patching file mysql-test/suite/backup/r/backup_logs.result Hunk #1 FAILED at 78. Hunk #2 FAILED at 86. Hunk #3 FAILED at 95. Hunk #4 succeeded at 273 (offset 142 lines). Hunk #5 FAILED at 297. Hunk #6 succeeded at 344 (offset 156 lines). 4 out of 6 hunks FAILED -- saving rejects to file mysql-test/suite/backup/r/backup_logs.result.rej ... > === renamed file 'sql/ddl_blocker.h' => 'sql/bml.h' > --- a/sql/ddl_blocker.h 2008-11-21 19:35:55 +0000 > +++ b/sql/bml.h 2008-12-17 14:32:46 +0000 > @@ -6,117 +6,117 @@ > #include "mysql_priv.h" > > /** > - @class DDL_blocker_class [1] The patch does not apply cleanly to this file. patching file sql/bml.h Hunk #1 FAILED at 6. 1 out of 1 hunk FAILED -- saving rejects to file sql/bml.h.rej ... > === modified file 'sql/si_logs.cc' > --- a/sql/si_logs.cc 2008-10-30 17:53:24 +0000 > +++ b/sql/si_logs.cc 2008-12-17 14:32:46 +0000 > @@ -208,6 +208,7 @@ const char *Backup_log::get_state_string > case BUP_RUNNING: return("running"); > case BUP_ERRORS: return("error"); > case BUP_CANCEL: return("cancel"); > + case BUP_PREPARED: return("prepared"); > default: return("unknown"); > } > } [4] I still do not see the need or the usefulness of this 'prepared' state. Please more documentation to describe a) how useful it is to the user, and b) what/how a user would use it to aide in using backup. If there is no usefulness to the user, please remove this state or make it debug only. > === modified file 'sql/si_logs.h' > --- a/sql/si_logs.h 2008-11-14 14:49:09 +0000 > +++ b/sql/si_logs.h 2008-12-17 14:32:46 +0000 > @@ -30,7 +30,8 @@ enum enum_backup_state > BUP_VALIDITY_POINT, > BUP_RUNNING, > BUP_ERRORS, > - BUP_CANCEL > + BUP_CANCEL, > + BUP_PREPARED [4] See above. ... > === modified file 'sql/si_objects.cc' > --- a/sql/si_objects.cc 2008-12-06 00:02:44 +0000 > +++ b/sql/si_objects.cc 2008-12-17 14:32:46 +0000 ... > /** > - Turn on the ddl blocker > + Get the backup metadata lock. > > - This method is used to start the ddl blocker blocking DDL commands. > + This method is used to start the ddl blocker blocking DDL commands. [3] Here's another one. ... > === modified file 'sql/sql_class.cc' > --- a/sql/sql_class.cc 2008-12-16 11:51:34 +0000 > +++ b/sql/sql_class.cc 2008-12-17 14:32:46 +0000 > @@ -415,12 +415,13 @@ THD::THD() > spcont(NULL), > m_parser_state(NULL), > /* > - @todo The following is a work around for online backup and the DDL blocker. > - It should be removed when the generalized solution is in place. > - This is needed to ensure the restore (which uses DDL) is not blocked > - when the DDL blocker is engaged. > + @todo The following is a work around for online backup and the > + backup metadata lock (BML). It should be removed when the generalized > + solution is in place. > + This is needed to ensure the restore thread (which uses BML) is not > + blocked by the lock. [7] We have generally accepted that our project is 'MySQL Backup' and we should refrain from naming it 'online backup'. I suggest we use the new name. ... > === modified file 'sql/sql_class.h' > --- a/sql/sql_class.h 2008-12-05 23:47:51 +0000 > +++ b/sql/sql_class.h 2008-12-17 14:32:46 +0000 > @@ -1788,12 +1788,13 @@ public: > Parser_state *m_parser_state; > > /* > - @todo The following is a work around for online backup and the DDL blocker. > - It should be removed when the generalized solution is in place. > - This is needed to ensure the restore (which uses DDL) is not blocked > - when the DDL blocker is engaged. > + @todo The following is a work around for online backup and the > + backup metadata lock (BML). It should be removed when the generalized > + solution is in place. > + This is needed to ensure the restore thread (which uses BML) is not > + blocked by the lock. [7] Again. > */ > - my_bool DDL_exception; // Allow some DDL if there is an exception > + my_bool BML_exception; // Allow some DDL if there is an exception > ulong backup_wait_timeout; > > Locked_tables_list locked_tables_list; > @@ -2955,6 +2956,12 @@ public: > modifies our currently non-transactional system tables. > */ > #define CF_AUTO_COMMIT_TRANS (CF_IMPLICT_COMMIT_BEGIN | CF_IMPLICIT_COMMIT_END) > +/** > + Mark statements which shuld be blocked when the Backup Metadata Lock is > + active. See bml.cc. > +*/ > +#define CF_BLOCKED_BY_BML (1U << 8) > + > > /* Bits in server_command_flags */ > > > === modified file 'sql/sql_parse.cc' > --- a/sql/sql_parse.cc 2008-12-04 23:14:30 +0000 > +++ b/sql/sql_parse.cc 2008-12-17 14:32:46 +0000 ... > @@ -351,6 +392,13 @@ bool is_log_table_write_query(enum enum_ > return (sql_command_flags[command] & CF_WRITE_LOGS_COMMAND) != 0; > } > > +static > +bool is_bml_blocked(const enum enum_sql_command command) > +{ > + DBUG_ASSERT(command <= SQLCOM_END); > + return (sql_command_flags[command] & CF_BLOCKED_BY_BML) != 0; > +} > + [5] I think this belongs in si_objects. It could be useful in other parts of backup code. Chuck