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
