From: Date: July 6 2007 5:15pm
Subject: Re: bk commit into 5.1 tree (rafal:1.2531) BUG#21842
List-Archive: http://lists.mysql.com/commits/30444
Message-Id: <468E5C99.1000808@mysql.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit

Chuck Bell wrote:
> Rafal,
> 
> I will look for your email when I get in and make your response to this
> email and the bug review my first action for the morning.
> 
> General Comments
> ----------------
> There are some spelling and grammatical errors in both patches. Mostly in
> the patch comments, but there are some in the code as well. For instance:
>   "The changes seems to..." --> "The changes seem to"
>   "chose" --> "choose"
>   ...etc.
> 
> In the comments for the second patch you list steps 1,2,3,4,7. What happened
> to 5 and 6?
>

Good question... I renumbered the steps.

> Some method calls do not adhere to the parameter spacing guideline.
> 

> Questions
> ---------
> What is the purpose of the .orig file? Are we saving that for something? If
> this is deliberate, can you put some comments in the patch(es) to explain
> it?
> 
> Why are there merges included in the first patch? Can that be cleaned up so
> that it includes only the changes? It is difficult to determine if any of
> the merges are applicable or a consequence of the commit process.
>

My mistake - they should not be there.


> Concerns
> --------
> I'd like to be able to see the problem and how the solution fixes it, but I
> haven't been able to get the tests outlined in the bug report to work. Can
> you please tell me how I can test for the bug? I am using Linux (SUSE) to
> test.

See the REFINED PROBLEM DESCRIPTION in the bug report. To repeat incorrect 
behaviour one has to setup ndb->myisam row-based replication and replicate some 
updates on a table which uses keys (so that ndb doesn't send all the columns in 
rows events). Unfortunately such replication setup doesn't work because of new 
bug#29569 introduced recently. Before, I could create such setup and observe 
that my patched code worked correctly, while old code not.

What you can do now is to run ndb_extraCol test and see the difference before 
and after patch (note, the results stored for this test are wrong! my patch 
fixes them).

> 
> There are several methods that have THD as one of the parameters but it
> isn't used in the method. Please remove any unnecessary parameters from the
> methods.
> 

We don't know it THD is not necessary since it is an argument to a virtual 
function which is supposed to be overwritten in derived classes. While current 
descendants don't use it, future ones might need it. If this is do_exec_row() 
method which is supposed to apply a row to the table, it seems reasonable to 
assume that the THD context could be needed for such operation.

> Suggestions
> -----------
> The open_and_lock_tables() method seems confusing. I would suggest changing
> the name to something that makes it less confusing with the
> open_and_lock_tables() as called from other places in the code (e.g.,
> sql_parse.cc).
> 

Trying to make the patch more minimal I removed this function - the code is back 
in Rows_log_event::do_apply_event() method.

Rafal