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
