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

[Chuck Bell]

Rafal,

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

I disagree. I can/may use it to approve the architecture but I cannot 
accept it as a code review until I can successfully apply the patch, 
compile, and execute the code.

I have received other patches from other developers with renamed files 
(and even deleted ones) and they all worked fine. Perhaps it is how you 
are telling bzr to do the rename.

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

I have the latest tree. It still does not apply correctly. Here is the 
patch run:

$ patch -p1 -u < ../../patches/32702.patch
patching file mysql-test/suite/backup/include/bml_test.inc
patching file mysql-test/suite/backup/r/backup_ddl_blocker.result
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/back
up_logs.result.rej
patching file mysql-test/suite/backup/r/backup_logs_output.result
patching file mysql-test/suite/backup/r/backup_logs_purge.result
patching file mysql-test/suite/backup/r/backup_timeout.result
patching file mysql-test/suite/backup/t/backup_ddl_blocker.test
patching file mysql-test/suite/backup/t/backup_timeout.test
patching file sql/CMakeLists.txt
patching file sql/Makefile.am
patching file sql/backup/kernel.cc
Hunk #2 succeeded at 558 (offset -6 lines).
Hunk #3 succeeded at 672 (offset -6 lines).
Hunk #4 succeeded at 829 (offset 8 lines).
Hunk #5 succeeded at 962 (offset 13 lines).
Hunk #6 succeeded at 1209 (offset 13 lines).
patching file sql/ddl_blocker.cc
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
patching file sql/mysqld.cc
Hunk #3 succeeded at 1494 (offset 3 lines).
Hunk #4 succeeded at 3730 (offset 3 lines).
patching file sql/si_logs.cc
patching file sql/si_logs.h
patching file sql/si_objects.cc
patching file sql/si_objects.h
patching file sql/sql_class.cc
patching file sql/sql_class.h
patching file sql/sql_parse.cc

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

I simply use patch -p1 -u ...

Is there some other way I am not aware of?

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

Incidental but noted.

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

Ok.

>> 6. What about default connection?
>>
> 
> The default connection is always present - it is set up by MTR.

Ok.

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

I am merely concerned what will happen when the code runs on the 
unpredictable and notoriously slow pushbuild machines. If you think it's 
ok, let it rip as is and we'll see if it blows chunks on pushbuild (I 
think it might eventually).

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

No, that's ok. I think this is not very useful to the user and only 
useful to developers. I will not request its removal, but please 
document it using the text above.

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

Ok.

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

Yes, and I use bzr. I don't understand what you're suggesting here.

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

I disagree. The patch needs to be applied to all platforms. My 
description above was commentary on what I did to get around the problem.

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

Ok. See comments above WRT pushbuild machines.

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

Ok, I see.

Chuck
> 
> Rafal
>