MySQL Lists are EOL. Please join:

List:Commits« Previous MessageNext Message »
From:Sven Sandberg Date:December 14 2007 10:37am
Subject:Re: bk commit into 5.1 tree (sven:1.2652) BUG#32407
View as plain text  
Tjaba Andrei!

(btw, just to avoid putting you in funny situations: tjaba [pronounced
"shaba"] is very informal - not exactly rude, but, you know, you
wouldn't usually say it to someone older than 30 or to people you don't
know. I think it's fine inside the team. I'd love to hear you say it to
Mårten :-)

Thanks for your comments! See my responses below.


Andrei Elkin wrote:
[...]
>>    - mysqlbinlog is now able to print FD events.  It has three modes:
> 
> Why not to call the option
> 
> --output

I thought so too first, but then Mats pointed out that we may in theory
add orthogonal output options in the future, like --gzip-output. Then
it's better to have --gzip-output in combination with --base64-output,
than to have
--output=[statement|base64|auto|gzip-statement|gzip-base64|gzip-auto]

> 
> ?
> 
> That would make
> 
>>       --base64-output=auto    Print row events in base64 output, and print
>>                               FD event.  The FD event is printed even if
>>                               it is outside the range specified with
>>                               --start-position, because it would not be
>>                               safe to read row events otherwise. This is
>>                               the default.
> 
> --output=auto
> 
>>   
>>       --base64-output=always  Like --base64-output=auto, but also print
>>                               base64 output for query events.  This is
>>                               like the old --base64-output flag, which
>>                               is also a shorthand for
>>                               --base64-output=always
> 
> --output=base64
> 
>>   
>>       --base64-output=never   Never print base64 output, generate error if
>>                               row events occur in binlog.  This is
>>                               useful to suppress the FD event in binlogs
>>                               known not to contain row events (e.g.,
>>                               because BINLOG statement is unsafe,
>>                               requires root privileges, is not SQL,
>>                               etc)
> 
> --output=query
> 
>   or
> 
> --output=statement
> 
> Btw, I think it's an overreaction to stop with an error upon
> appearence of a row-based event.
> I'd rather to  mean such option as a filter to throw away row-based
> events.

I think that would be a bit dangerous. Imagine the following user scenario:

 - User writes a script that pipes the output of mysqlbinlog to a client.
 - User notes the FD event, which looks strange, and forces the client
to be root. So uses passes --no-base64-output. This works fine, because
the user uses --binlog-format=statement.
 - Later, user changes to --binlog-format=mixed. Then the database is
corrupted because base64 output is filtered out!

If you really want to get rid of base64 statements, there is already the
(unsafe) --short-form flag. (But see my reply to Mats on commits@ for an
explanation why I think it should be deprecated, too.)

[...]
>> diff -Nrup a/sql/log_event.cc b/sql/log_event.cc
>> --- a/sql/log_event.cc	2007-11-28 12:11:13 +01:00
>> +++ b/sql/log_event.cc	2007-12-13 18:03:52 +01:00
>> @@ -470,9 +470,9 @@ static void print_set_option(IO_CACHE* f
>>    Log_event::get_type_str()
>>  */
>>  
>> -const char* Log_event::get_type_str()
>> +const char* Log_event::get_type_str(Log_event_type type)
>>  {
>> -  switch(get_type_code()) {
>> +  switch(type) {
>>    case START_EVENT_V3:  return "Start_v3";
>>    case STOP_EVENT:   return "Stop";
>>    case QUERY_EVENT:  return "Query";
>> @@ -493,6 +493,9 @@ const char* Log_event::get_type_str()
>>    case WRITE_ROWS_EVENT: return "Write_rows";
>>    case UPDATE_ROWS_EVENT: return "Update_rows";
>>    case DELETE_ROWS_EVENT: return "Delete_rows";
> 
>> +  case PRE_GA_WRITE_ROWS_EVENT: return "Write_rows_old";
> 
> I must have offered it before...
> I think we need to say that more specific - like
> "Write_rows_pre5_1_16" (or what was the version?).
> 
> Sure that applies to all "old" words used in that sense.

Maybe, but I think it should not be part of this bugfix. Is that ok with
you?

[...]
>> @@ -1024,94 +1032,112 @@ Log_event* Log_event::read_log_event(con
[...]
>> +  int event_type= buf[EVENT_TYPE_OFFSET];
> 
> If you typify event_type as uint event_type < 0 is not needed.
> The new `if' is essentially about that the preceding
> 
>   if (event_len < EVENT_LEN_OFFSET ...
> 
> is for - "general sanity check". I'd combine them that would also made
> less identation conflicts.

Sure, can do. (Plus, it may save a clock cycle :-)

[...]
>> +    if (exec_res == 2)
>> +      DBUG_RETURN(1);
> 
> why not to return exec_res? The only caller does not distinguish
> between non-zeros anyway... But should it need to know the number the value would
> be available.

I wanted the description of the function to be as simple as possible, so
it always returns 1 on error. I actually had some trouble writing the
patch because the previous code returned the return value of
ev->apply_event() on error, and I had to find out if it was necessary to
maintain this behavior (it was not). As soon our code does more than
necessary, there is a risk that it is taken by someone as a promise that
it will always work like that. It can easily be changed later if we ever
need to distinguish return values.

[...]

/Sven


-- 
Sven Sandberg, Software Engineer
MySQL AB, www.mysql.com
Thread
bk commit into 5.1 tree (sven:1.2652) BUG#32407Sven Sandberg13 Dec
  • Re: bk commit into 5.1 tree (sven:1.2652) BUG#32407Mats Kindahl13 Dec
  • Re: bk commit into 5.1 tree (sven:1.2652) BUG#32407Andrei Elkin14 Dec
    • Re: bk commit into 5.1 tree (sven:1.2652) BUG#32407Sven Sandberg14 Dec
      • Re: bk commit into 5.1 tree (sven:1.2652) BUG#32407Andrei Elkin14 Dec