Re: bzr commit into mysql-5.1 branch (hezx:2632) Bug#37051

[Sven Sandberg]

Ingo Strüwing wrote:
> Sven Sandberg, 30.07.2008 13:25:
>> He Zhenxing wrote:
[...]
>>>    /*
>>>      Here there could be code like
>>>      if (command-line-option-which-says-"log_this_variable" &&
> inited)
> 
> The cause of the forgotten buffer enlargement could be that this comment
> does not ask for it. To avoid future confusion, I suggest to add a hint
> here.

I agree.

>>> @@ -1709,7 +1715,8 @@ Query_log_event::Query_log_event(THD* th
>>>    
>>> auto_increment_increment(thd_arg->variables.auto_increment_increment),
>>>     auto_increment_offset(thd_arg->variables.auto_increment_offset),
>>>     lc_time_names_number(thd_arg->variables.lc_time_names->number),
>>> -   charset_database_number(0)
>>> +   charset_database_number(0),
>>> +  
>>> table_map_for_update((ulonglong)thd_arg->variables.table_map_for_update)
> 
> Below Sven says "There is no @@session.table_map_for_update variable".
> If that is true, this initialization won't compile. Indeed it is added
> by this patch in sql_class.h below.
> 
> ...
>>> @@ -2238,6 +2250,13 @@ void Query_log_event::print_query_header
>>>                    print_event_info->delimiter);
>>>      print_event_info->charset_database_number= charset_database_number;
>>>    }
>>> +  if (table_map_for_update)
>>> +  {
>>> +    char int_buf[22];
>>> +    longlong10_to_str(uint8korr(&table_map_for_update), int_buf, 10);
>>> +    my_b_printf(file, "SET @@session.table_map_for_update=%s%s\n",
>>> +                int_buf, print_event_info->delimiter);
>>> +  }
>> This must be a mistake? There is no @@session.table_map_for_update
>> variable. This method is invoked only from mysqlbinlog, which is not
>> subject to --replicate-* options. So I think the above 7 lines can just
>> be removed.
> 
> The variable exists. We need it to transport the table map to
> mysql_execute_command(), where we use its value.
>
> However if it is desirable to print the table map with the other
> elements of the header, I can't say. But for me it looks more complete
> and systematic to print all pieces of the header. Otherwise the table
> map would be even more special and at least a comment would be necessary
> why this is not printed.

The patch added the field table_map_for_update to 'struct 
system_variables', but it was not exported to the SQL language. So the 
variable doesn't actually exist (try 'SET 
@@session.table_map_for_update=1': it gives an error).

It seems that all other fields of Query_log_event are written: those 
that cannot be represented in SQL (such as exec_time) are written in a 
comment. So I agree, the table_map should be written in the same comment.

>>>  }
>>>  
>>>  
>>> @@ -2425,6 +2444,8 @@ int Query_log_event::do_apply_event(Rela
>>>        else
>>>          thd->variables.collation_database= thd->db_charset;
>>>        +      thd->variables.table_map_for_update=
>>> (table_map)table_map_for_update;
> 
> This proves the existence of the system variable again.
> 
>>> +             /* Execute the query (note that we bypass
>>> dispatch_command()) */
>>>        const char* found_semicolon= NULL;
>>>        mysql_parse(thd, thd->query, thd->query_length,
> &found_semicolon);
>>>
>>> === modified file 'sql/log_event.h'
>>> --- a/sql/log_event.h    2008-03-28 13:52:33 +0000
>>> +++ b/sql/log_event.h    2008-07-30 09:50:27 +0000
>>> @@ -242,6 +242,7 @@ struct sql_ex_info
>>>                                     1 + 1 + 255 /* catalog */ + \
>>>                                     4 /* autoinc */ + \
>>>                                     6 /* charset */ + \
>>> +                                   8 /* table_map_for_update */ + \
>>>                                     MAX_TIME_ZONE_NAME_LENGTH)
>> As mentioned above, this computation is wrong (before your patch)
>> because it does not account for the one-byte type codes.
> 
> ... and there are other inconsistencies. This define is "only" used for
> buffer allocation and security checks, as far as I can see. So the risk
> of fixing it is low.
> 
> Note that this is a difference against fixing the logic of
> Query_log_event::write(). I still think that the latter is more risky.

How do you mean? I don't think we need to change the logic of 
Query_log_event::write() in any other way than replace the long 
computation by a macro that expands to the same computation. It just 
needs double-checking that the macro expands to the same thing as the 
original computation. It would me more risky to try to maintain two 
copies of the same computation, IMHO.

-- 
Sven Sandberg, Software Engineer
MySQL AB, www.mysql.com