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

> ?
> 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/ b/sql/
>> --- a/sql/	2007-11-28 12:11:13 +01:00
>> +++ b/sql/	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

>> @@ -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 Sandberg, Software Engineer
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