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
