| List: | Commits | « Previous MessageNext Message » | |
| From: | Chuck Bell | Date: | December 17 2008 8:10pm |
| Subject: | Re: bzr commit into mysql-6.0-backup branch (Rafal.Somla:2741) Bug#32702 WL#4644 | ||
| View as plain text | |||
STATUS ------ Not approved -- new patch needed to complete review. Need patch that: - renames the files correctly - doesn't delete the files if rename fails REQUESTS -------- 1. Patch does not apply correctly to latest tree. Please fix. patching file sql/ddl_blocker.h Hunk #1 FAILED at 6. 1 out of 1 hunk FAILED -- saving rejects to file sql/ddl_blocker.h.rej 2. Patch does not compile on Windows. Indeed, the patch corrupts the project files when the cmake file is processed because the patch does not include the bml files. The following error has occurred during XML parsing: File: D:\source\bzr\mysql-6.0-review\sql\mysqld.vcproj Line: 269 Column: 20 Error Message: The parameter is incorrect. The file 'D:\source\bzr\mysql-6.0-review\sql\mysqld.vcproj' has failed to load. 3. Isn't there another bug associated with this? I think it is BUG#41262. Shouldn't that be referenced in the patch comments? 4. Minor comment change suggestion. 5. Not sure if the extra resets are needed. Please verify. 6. What about default connection? 7. Why such a short timeout on the debug synch call? 8. Why was include removed? 9. Why is the new state added? I do not see any reason to have it. Please enlighten us and/or remove it. 10. We must resolve the issue with the runtime team. COMMENTARY ---------- The rename did not apply correctly in the patch which is the likely cause for the project file corruption on Windows. Also, the rename for the tests did not apply either so they weren't run during the test suite. Once I manually fixed the problem with the corrupt project file, the code failed to link: 1>Linking... 1>mysqld.obj : error LNK2019: unresolved external symbol "public: static void __cdecl BML_class::destroy_BML_class_instance(void)" (?destroy_BML_class_instance@BML_class@@SAXXZ) referenced in function "void __cdecl clean_up_mutexes(void)" (?clean_up_mutexes@@YAXXZ) 1>mysqld.obj : error LNK2019: unresolved external symbol "public: static class BML_class * __cdecl BML_class::get_BML_class_instance(void)" (?get_BML_class_instance@BML_class@@SAPAV1@XZ) referenced in function "int __cdecl init_thread_environment(void)" (?init_thread_environment@@YAHXZ) 1>sql_parse.obj : error LNK2019: unresolved external symbol "public: void __thiscall BML_class::bml_leave(void)" (?bml_leave@BML_class@@QAEXXZ) referenced in function "int __cdecl mysql_execute_command(class THD *)" (?mysql_execute_command@@YAHPAVTHD@@@Z) 1>sql_parse.obj : error LNK2019: unresolved external symbol "public: char __thiscall BML_class::bml_enter(class THD *)" (?bml_enter@BML_class@@QAEDPAVTHD@@@Z) referenced in function "int __cdecl mysql_execute_command(class THD *)" (?mysql_execute_command@@YAHPAVTHD@@@Z) 1>si_objects.obj : error LNK2019: unresolved external symbol "public: char __thiscall BML_class::bml_get(class THD *)" (?bml_get@BML_class@@QAEDPAVTHD@@@Z) referenced in function "bool __cdecl obs::bml_get(class THD *)" (?bml_get@obs@@YA_NPAVTHD@@@Z) 1>si_objects.obj : error LNK2019: unresolved external symbol "public: void __thiscall BML_class::bml_release(void)" (?bml_release@BML_class@@QAEXXZ) referenced in function "void __cdecl obs::bml_release(void)" (?bml_release@obs@@YAXXZ) 1>D:\source\bzr\mysql-6.0-review\sql\Debug\mysqld.exe : fatal error LNK1120: 6 unresolved externals This is because the cmake file does not add the bml files to the mysqld project. A regeneration of the project file fixed this problem. DETAILS ------- > 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(). ... > mysql-test/suite/backup/t/backup_bml.test > Almost complete rewrite of the test. [4] Let's just say, "refactored test". ... > === 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 > +# 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, [6] What about the default connection? Do we need to specify that too? > +# - 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 > +# database with data prior to using it. ... > +# > +# Make bml_test the default database in all connections. > +# > +USE bml_test; > +connection con1; > +SET DEBUG_SYNC= 'reset'; > +USE bml_test; > +connection con2; > +SET DEBUG_SYNC= 'reset'; > +USE bml_test; > +connection con3; > +SET DEBUG_SYNC= 'reset'; > +USE bml_test; > +connection con4; > +SET DEBUG_SYNC= 'reset'; > +USE bml_test; > +connection con5; > +SET DEBUG_SYNC= 'reset'; > +USE bml_test; [5] I don't think you need to do all of these resets. I think once is enough. > +# Start BACKUP/RESTORE > + > + connection con5; > + --echo # con5: Wait for DDL2 to reach its synchronization point. > + SET DEBUG_SYNC= 'now WAIT_FOR ddl2_started TIMEOUT 3'; [7] Why such a short timeout? What purpose does this intend to fulfill? Note: this applies other places in the test. ... > + # To be safe clear before_execute_sql_command which is hit by > + # every executed statement > + SET DEBUG_SYNC='before_execute_sql_command CLEAR'; > + > + --echo # con5: Activate synchronization points for BACKUP/RESTORE. > + # > + # Make BACKUP/RESTORE to operation which should be blocked by ongoing > + # statements. Make it send 'bup_waiting' signal just before it checks > + # for registered statements inside BML. > + # > + SET DEBUG_SYNC= 'bml_get_check2 SIGNAL bup_waiting'; > + # > + # Whether BACKUP/RESTORE is blocked or not make it stop after it > + # completes preparations, including activating the BML. > + # > + SET DEBUG_SYNC= 'after_backup_start_backup WAIT_FOR continue_bup'; > + SET DEBUG_SYNC= 'after_backup_start_restore WAIT_FOR continue_bup'; > + # > + # Also make it to stop in the middle of operation after seeing > + # 'continue_bup' so that DDL3 and DDL4 can be started in parallel > + # > + SET DEBUG_SYNC= 'before_backup_meta SIGNAL bup_running WAIT_FOR finish_bup'; > + SET DEBUG_SYNC= 'start_do_restore SIGNAL bup_running WAIT_FOR finish_bup'; > + > + --echo # con5: Starting BACKUP/RESTORE operation -- should be blocked > + --echo # by ongoing DDLs. > + # arrange for deterministic backup_id = 500 > + SET SESSION debug="+d,set_backup_id"; > + --send > + eval $operation; comment: I don't like this indentation. ... > +# Ensure that BACKUP/RESTORE is in progress, after activating BML. > +SET DEBUG_SYNC= 'now SIGNAL continue_bup WAIT_FOR bup_running TIMEOUT 1'; [7] Why such a short timeout? What purpose does this intend to fulfill? Note: this applies other places in the test. ... > + > +--echo > +--echo == Checkpoint C == > +--echo # > +--echo # DDL1= $DDL1 > +--echo # DDL2= $DDL2 > +--echo # DDL3= $DDL3 > +--echo # DDL4= $DDL4 > +--echo # > +SELECT state, info FROM INFORMATION_SCHEMA.PROCESSLIST > + WHERE info LIKE "BACKUP DATABASE%" > + OR info LIKE "RESTORE FROM%"; > +--echo # Checking that DDL1 and DDL2 have executed. > +CALL test.check_results(); > +--echo ================== > +--echo > + > +# Start DDL3 and DDL4 while DDL blocker is active. > + > + connection con3; > + --echo # con3: Send DDL3 but it is blocked by BACKUP/RESTORE > + --echo # (will not be in backup) make it send signal when > + --echo # blocked on BML. > + SET DEBUG_SYNC= 'bml_enter_check SIGNAL ddl3_blocked'; > + --send > + eval $DDL3; > + > + connection con4; > + --echo # Wait for DDL3 to send its signal. > + SET DEBUG_SYNC= 'now WAIT_FOR ddl3_blocked TIMEOUT 3'; [7] Why such a short timeout? What purpose does this intend to fulfill? Note: this applies other places in the test. ... > + --echo # con3: Send DDL3 but it is blocked by BACKUP/RESTORE > + --echo # (will not be in backup) make it send signal when > + --echo # blocked on BML. > + SET DEBUG_SYNC= 'bml_enter_check SIGNAL ddl4_blocked'; > + --send > + eval $DDL4; > + > +# check that DDL3 and DDL4 are blocked > + > +connection default; > +--echo # Wait for DDL4 to send its signal. > +SET DEBUG_SYNC= 'now WAIT_FOR ddl4_blocked TIMEOUT 3'; [7] Why timeout 3? Is that better than 1 used before? We need consistency. ... > + > +--echo > +--echo == Checkpoint D == > +--echo # > +--echo # DDL1= $DDL1 > +--echo # DDL2= $DDL2 > +--echo # DDL3= $DDL3 > +--echo # DDL4= $DDL4 > +--echo # > +eval SELECT state, info FROM INFORMATION_SCHEMA.PROCESSLIST > + WHERE info LIKE "$DDL3%" > + OR info LIKE "$DDL4%"; > +--echo # Checking that DDL3 and DDL4 have not executed. > +CALL test.check_results(); > +--echo ================== > +--echo > + > +--echo # Resume BACKUP/RESTORE - this allows DDL3 and DDL4 to complete. > +SET DEBUG_SYNC= 'now SIGNAL finish_bup'; > + > + connection con5; > + --echo # con5: Reaping BACKUP/RESTORE > + reap; > + SET debug="-d"; > + > + connection con3; > + --echo # con3: Completing DDL3 > + reap; > + > + connection con4; > + --echo # con4: Completing DDL4 > + reap; > + > +connection default; > + > +--echo > +--echo == Checkpoint E == > +--echo # > +--echo # DDL1= $DDL1 > +--echo # DDL2= $DDL2 > +--echo # DDL3= $DDL3 > +--echo # DDL4= $DDL4 > +--echo # > +--echo # Checking that DDL3 and DDL4 have executed. > +CALL test.check_results(); > +--echo ================== > +--echo > + > +# Cleanup > + > +connection con1; > +SET DEBUG_SYNC= 'reset'; > +connection con2; > +SET DEBUG_SYNC= 'reset'; > +connection con3; > +SET DEBUG_SYNC= 'reset'; > +connection con4; > +SET DEBUG_SYNC= 'reset'; > +connection con5; > +SET DEBUG_SYNC= 'reset'; > +connection default; > +SET DEBUG_SYNC= 'reset'; [5] I don't think you need to do all of these resets. I think once is enough. ... > + > +let $backup_to=; > +let $restore_from=; > > === renamed file 'mysql-test/suite/backup/r/backup_ddl_blocker.result' => > 'mysql-test/suite/backup/r/backup_bml.result' > --- a/mysql-test/suite/backup/r/backup_ddl_blocker.result 2008-11-20 14:07:23 +0000 > +++ b/mysql-test/suite/backup/r/backup_bml.result 2008-12-17 14:32:46 +0000 Cannot review test until new patch is created. ... > === renamed file 'mysql-test/suite/backup/t/backup_ddl_blocker.test' => > 'mysql-test/suite/backup/t/backup_bml.test' > --- a/mysql-test/suite/backup/t/backup_ddl_blocker.test 2008-11-20 14:07:23 +0000 > +++ b/mysql-test/suite/backup/t/backup_bml.test 2008-12-17 14:32:46 +0000 This rename did not happen. Instead, it removed the file backup_ddl_blocker.test. No test to test with. ... > === modified file 'sql/CMakeLists.txt' > --- a/sql/CMakeLists.txt 2008-11-12 01:37:42 +0000 > +++ b/sql/CMakeLists.txt 2008-12-17 14:32:46 +0000 > @@ -68,7 +68,7 @@ ADD_EXECUTABLE(mysqld > partition_info.cc rpl_utility.cc rpl_injector.cc sql_locale.cc > rpl_rli.cc rpl_mi.cc sql_servers.cc sql_audit.cc > sql_connect.cc scheduler.cc transaction.cc > - ddl_blocker.cc si_objects.cc si_logs.cc > + bml.cc si_objects.cc si_logs.cc > sql_profile.cc event_parse_data.cc mdl.cc > rpl_handler.cc > ${PROJECT_SOURCE_DIR}/sql/sql_yacc.cc > Seems ok, it was just the rename that failed. ... > === modified file 'sql/backup/kernel.cc' > --- a/sql/backup/kernel.cc 2008-12-10 15:53:06 +0000 > +++ b/sql/backup/kernel.cc 2008-12-17 14:32:46 +0000 > @@ -71,7 +71,6 @@ > #include "be_default.h" > #include "be_snapshot.h" > #include "be_nodata.h" > -#include "ddl_blocker.h" [8] Why don't we need the include "bml.h"? ... > @@ -677,8 +678,9 @@ Backup_restore_ctx::prepare_for_backup(S > > info->save_start_time(when); > m_catalog= info; > - m_state= PREPARED_FOR_BACKUP; > - > + m_state= PREPARED_FOR_BACKUP; > + report_state(BUP_PREPARED); > + > return info; > } [9] What is this new state? Is it documented somewhere? Why do we need it? What does it tell the user? I think you probably have a good developer reason to have it but unless there is some profound goodness for the user, I must ask that it be removed. > > @@ -819,6 +821,8 @@ Backup_restore_ctx::prepare_for_restore( > obs::engage_binlog(FALSE); > } > > + report_state(BUP_PREPARED); > + > return info; > } ... > === 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"); > } > } [9] Sorry, just don't see the need for the extra state. > === 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 > }; [9] Sorry, just don't see the need for the extra state. ... > === 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 [10] For the record, we need to decide if we want to take responsibility for this or ask that runtime team take over the BML blocker and wait 2-3+ months for the patch to fix it. I vote we do both and hold the runtime team to their promise to replace it (if we can get them to agree to that). ... Chuck