MySQL Lists are EOL. Please join:

List:Commits« Previous MessageNext Message »
From:Andrei Elkin Date:December 14 2007 11:06am
Subject:Re: bk commit into 5.1 tree (sven:1.2652) BUG#32407
View as plain text  
Sven, tjena :)

> 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 :-)
>

Who knows one day ... :)


> 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]

okay, that's not the right time maybe.
>
>> 
>> ?
>> 
>> 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. 

Maybe I am missing something...

If we have --output=query (aka non-stopping --base64-output=never)
then this BINLOG FD won't be sent to the client program ...

If there is a security issue then to stop is necessary. But if not, i
would not, but I leave it to you to decide what's better.

> 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 :-)
>

thanks! So we have this only little issue. 

> [...]
>>> +    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.

sure.

>
> [...]
>
> /Sven
>
>
> -- 
> Sven Sandberg, Software Engineer
> MySQL AB, www.mysql.com

/andrei
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