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
