Re: bzr commit into mysql-6.0-backup branch (Rafal.Somla:2741) Bug#32702 WL#4644

[Rafal Somla]

Hi Chuck,

See my explanations below. None of your concerns will result in any meaningful 
changes of the code I submitted (except for updating one patch comment and 
removing wrong tabs from the test script). So I think you can continue reviewing 
the patch as it is now.

Chuck Bell wrote:
> 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
> 

I can apply the patch without any problems. My most recent mysql-6.0-backup tree 
has the following revision-id:

ingo.struewing@stripped

> 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.
>

This is not a problem with the patch but most probably with the way you are 
trying to apply it. Note that this is bzr patch, not one generated with diff. 
AFAIK the patch utility doesn't handle renames (at leas mine does not) and this 
is probably why you get problems when compiling.

> 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.

If they are suggestions, why you list them as a REQUEST? Please clearly split 
your concerns into requirements which you think must be addressed before you 
accept the patch and suggestions which you think will be good to implement but 
the patch is correct also without them.

However, I see no problem with changing the comment.

> 
> 5. Not sure if the extra resets are needed. Please verify.

Yes, they are needed. Without them sync points settings from the previous use of 
the "subroutine" might remain and spoil the test.

Also, cleaning the situation before starting testing makes the "subroutine" more 
robust (read: independent from the context in which it is "called").

> 
> 6. What about default connection?
> 

The default connection is always present - it is set up by MTR.

> 7. Why such a short timeout on the debug synch call?

The timeout is 3 seconds, which in my opinion should be enough for a DDL 
statement to start and reach the synchronization point (which is just before the 
big switch in mysql_execute_command()).

Do you think that a bigger timeout is needed? If yes, then how big it should be 
according to you?

See also my inline explanation.

> 
> 8. Why was include removed?
> 

Because it is not needed.
See also my inline explanation.

> 9. Why is the new state added? I do not see any reason to have it. 
> Please enlighten us and/or remove it.
> 

The new state gives more information about the progress of the operation. Now 
the user can see not only that the operation has started, but also that it has 
passed the initial preparation phase (where metadata is collected or objects are 
re-created).

Using this information, the test can distinguish between situation when BACKUP 
is stopped before finishing preparations or after that moment. The same will be 
true for any user who sees that BACKUP has hanged. Examining the backup_history 
log he can get more hints about where the problem has happened.

However, if you insist, I can remove the new state. One consequence will be that 
the test will be less sensitive to possible errors.

> 10. We must resolve the issue with the runtime team.
> 

I think there really is no "either this or that" issue. It is great that runtime 
offers to work on a better solution. At the same time, if you as a reviewer 
agree with the changes I proposed, we can just push them. This will not obstruct 
the development of a better solution, probably even help a bit.

Note that in irc discussion Kostja was objecting the whole idea of having "DDL 
blocker" not the changes I proposed. For the latter he agreed that they improve 
the code.

> COMMENTARY
> ----------
> The rename did not apply correctly in the patch which is the likely 
> cause for the project file corruption on Windows.

As said above - this is because patch utility does not perform file renames. You 
are reviewing patch generated by bzr and dedicated for bzr.

> 
> 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.
>

Of course you need to regenerate the project file if CMakeList.txt is changed as 
is the case for this patch. Similar as I need to recreate Makefiles when 
Makefile.am is changed (but this is done automatically by make).

The problems you are reporting here are problems with your working environment, 
not with my patch.

> DETAILS
> -------
...
>> === 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
...
>> +# 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.
> 

The purpose is as follows: Suppose that for some unknown reason, the started 
statement will not reach the synchronization point which fires 'ddl2_started' 
signal. Without the timeout, this 'WAIT_FOR ddl2_started' will hang for about 5 
minutes before test can continue and detect the error. Setting shorter timeout 
will improve test as it will report error much faster.

I think 3 seconds is more than enough for any statement to reach the specified 
synchronization point. On the other hand, we do not need to be very greedy here 
because it is expected that the signal will appear quick and the timeout will 
not be exercised.

...
>> === 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"?

Because kernel.cc does not use any of the functions or datatypes declared there. 
Instead, it is using the interface defined in si_objects.h.

Rafal