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

[Ingo Strüwing]

Hi Zhenxing, Sven,

Sven Sandberg, 30.07.2008 13:25:
...
> He Zhenxing wrote:
...
>>  2632 He Zhenxing    2008-07-30
>>       BUG#37051 Replication rules not evaluated correctly
...
>> === added file 'mysql-test/suite/rpl/t/rpl_filter_tables_not_exist.test'
...
>> +# ==== Purpose ====
>> +#
>> +# Test if replication table filter rules are properly evaluated when
>> +# some of the tables referenced by the multiple-table update are not
>> +# exist on slave.

Please change "are not exist" to "do not exist" here and at another place.

...
>> +# Clean up
>> +connection master;
>> +echo [on master];
>> +drop table if exists t1,t2,t3,t4,t5,t6,t7,t8,t9;
>> +source include/master-slave-end.inc;

This test case is a great piece of work. These are very systematic
tests. Thank you very much. I must admit though, that I didn't go
through every test in detail.

>>
>> === modified file 'sql/log_event.cc'
>> --- a/sql/log_event.cc    2008-05-30 09:12:07 +0000
>> +++ b/sql/log_event.cc    2008-07-30 09:50:27 +0000
>> @@ -1632,6 +1632,12 @@ bool Query_log_event::write(IO_CACHE* fi
>>      int2store(start, charset_database_number);
>>      start+= 2;
>>    }
>> +  if (table_map_for_update)
>> +  {
>> +    *start++= Q_TABLE_MAP_FOR_UPDATE;
>> +    int8store(start, table_map_for_update);
>> +    start+= 8;
>> +  }
> 
> The patch does not increase the size of the buffer where the table map
> is stored, so there is a risk of buffer overflow here. See the
> definition of the buffer in the beginning of this function.

Agree. This needs to be fixed now.

> 
> Actually, there seems to be a few problems here even before your patch:
> 
>  - Query_log_event::write() should use MAX_LOG_EVENT_HEADER, instead of
> computing the length of the buffer. That would avoid duplicate code.
> 
>  - More importantly, MAX_LOG_EVENT_HEADER is wrongly computed, as it
> does not account for the type code. The formula used in
> Query_log_event::write() is probably more correct.
> 
>  - MAX_LOG_EVENT_HEADER and MAX_SIZE_LOG_EVENT_STATUS should probably be
> renamed to something like MAX_QUERY_LOG_EVENT_HEADER_LENGTH, and
> MAX_QUERY_LOG_EVENT_STATUS_VARIABLES_LENGTH, because they are specific
> to Query_log_events.

The above suggestions are good, but IMHO there is a remarkable risk in
missing something when refactoring this. I propose not to do this in a
bug fix, which is considered for 5.1, let alone 5.0.

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

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

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

...

Regards
Ingo
-- 
Ingo Strüwing, Database Group
Sun Microsystems GmbH, Sonnenallee 1, D-85551 Kirchheim-Heimstetten
Geschäftsführer: Thomas Schröder, Wolfgang Engels, Dr. Roland Bömer
Vorsitzender des Aufsichtsrates: Martin Häring   HRB München 161028