MySQL Lists are EOL. Please join:

List:Commits« Previous MessageNext Message »
From:Mats Kindahl Date:December 13 2007 10:47am
Subject:Re: bk commit into 5.1 tree (sven:1.2651) BUG#32407
View as plain text  
Hi Sven!

Here are my comments regarding your patch.

Just my few cents,
Mats Kindahl

Sven Sandberg wrote:
> Below is the list of changes that have just been committed into a local
> 5.1 repository of sven. When sven does a push these changes will
> be propagated to the main repository and, within 24 hours after the
> push, to the public repository.
> For information on how to access the public repository
> see http://dev.mysql.com/doc/mysql/en/installing-source-tree.html
>
> ChangeSet@stripped, 2007-12-10 10:42:44+01:00, sven@riska.(none) +16 -0
>   BUG#32407: Impossible to do point-in-time recovery from older binlog
>   
>   Problem: it is unsafe to read base64-printed events without first
>   reading the Format_description_log_event (FD).  Currently, mysqlbinlog
>   cannot print the FD.
>   
>   As a side effect, another bug has also been fixed: When mysqlbinlog
>   --start-position=X was specified, no ROLLBACK was printed. I changed
>   this, so that ROLLBACK is always printed.
>   
>   This patch does several things:
>   
>    - Format_description_log_event (FD) now print themselves in base64
>      format.
>   

Good.

>   
>    - mysqlbinlog is now able to print FD events.  It has three modes:
>       --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.
>   
>       --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
>   
>       --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)
>   

Good.

>   
>    - the BINLOG statement now handles FD events correctly, by setting
>      the thread's rli's relay log's description_event_for_exec to the
>      loaded event.
>   

Good.

>   
>      In fact, executing a BINLOG statement is almost the same as reading
>      an event from a relay log.  Before my patch, the code for this was
>      separated (exec_relay_log_event in slave.cc executes events from
>      the relay log, mysql_client_binlog_statement in sql_binlog.cc
>      executes BINLOG statements).  I needed to augment
>      mysql_client_binlog_statement to do parts of what
>      exec_relay_log_event does.  Hence, I did a small refactoring and
>      moved parts of exec_relay_log_event to a new function, which I
>      named apply_event_and_update_pos.  apply_event_and_update_pos is
>      called both from exec_relay_log_event and from
>      mysql_client_binlog_statement.
>   

OK.

>   
>    - When a non-FD event is executed in a BINLOG statement, without
>      previously executing a FD event in a BINLOG statement, it generates
>      an error, because that's unsafe.  I took a new error code for that:
>      ER_NO_FORMAT_DESCRIPTION_EVENT_BEFORE_BINLOG_STATEMENTS.
>   

Yes, that is good.

>   
>      In order to get a decent error message containing the name of the
>      event, I added the class method char*
>      Log_event::get_type_str(Log_event_type type), which returns a
>      string name for the given Log_event_type.  This is just like the
>      existing char* Log_event::get_type_str(), except it is a class
>      method that takes the log event type as parameter.
>   

Did you refactor the code to implement Log_event::get_type_str() in 
terms of Log_event::get_type_str(Log_event_type) ?

"Class methods" are called "static member functions" in C++ lingo.

>   
>      I also added PRE_GA_*_ROWS_LOG_EVENT to Log_event::get_type_str(),
>      so that names of old rows event are properly printed.
>   

Good.

>   
>    - When reading an event, I added a check that the event type is known
>      by the current Format_description_log_event. Without this, it may
>      crash on bad input (and I was struck by this several times).
>   

Good.

>   
>    - I patched the following test cases, which all contain BINLOG
>      statements for row events which must be preceded by BINLOG
>      statements for FD events:
>       - rpl_bug31076
>   
>   While I was here, I fixed some small things in log_event.cc:
>   
>    - replaced hard-coded 4 by EVENT_TYPE_OFFSET in 3 places
>   
>    - replaced return by DBUG_VOID_RETURN in one place
>   
>    - The name of the logfile can be '-' to indicate stdin.  Before my
>      patch, the code just checked if the first character is '-'; now it
>      does a full strcmp().  Probably, all arguments that begin with a -
>      are already handled somewhere else as flags, but I still think it
>      is better that the code reflects what it is supposed to do, with as
>      little dependencies as possible on other parts of the code.  If we
>      one day implement that all command line arguments after -- are
>      files (as most unix tools do), then we need this.
>   
>   I also fixed the following in slave.cc:
>   
>    - next_event() was declared twice, and queue_event was not static but
>      should be static (not used outside the file).
>   

Please keep "fixes" to a minimal. Although it is good to get the things 
above in order, they can easily eat a lot of time for both you and the 
reviewer. In addition, they can cause extra problems with merges if done 
in a careless manner, and there is *always* a risk of introducing a bug 
when doing changes to the code (not for the changes above, but if you 
start getting bolder, there will be a significant risk).

No need to change this patch for this reason, though.

>   client/client_priv.h@stripped, 2007-12-10 10:42:42+01:00, sven@riska.(none) +1 -1
>     Declared the new option for base64 output.
>
>   client/mysqlbinlog.cc@stripped, 2007-12-10 10:42:42+01:00, sven@riska.(none) +105 -22
>      - Change from using the two-state command line option
>        "default/--base64-output" to the three-state
>        "--base64-output=[never|auto|always]"
>      - Print the FD event even if it is outside the --start-position range.
>      - Stop if a row event is about to be printed without a preceding FD
>        event.
>      - Minor fixes:
>         * changed 4 to EVENT_TYPE_OFFSET in some places
>         * Added comments
>         * before, "mysqlbinlog -xyz" read from stdin; now it does not
>           (only "mysqlbinlog -" reads stdin).
>
>   mysql-test/r/mysqlbinlog2.result@stripped, 2007-12-10 10:42:42+01:00, sven@riska.(none)
> +3 -0
>     Updated result file: mysqlbinlog now prints ROLLBACK always.
>
>   mysql-test/suite/binlog/r/binlog_base64_flag.result@stripped, 2007-12-10 10:42:43+01:00,
> sven@riska.(none) +46 -0
>     New test file needs new result file
>
>   mysql-test/suite/binlog/r/binlog_base64_flag.result@stripped, 2007-12-10 10:42:43+01:00,
> sven@riska.(none) +0 -0
>
>   mysql-test/suite/binlog/std_data/binlog-bug32407.000001@stripped, 2007-12-10
> 10:42:43+01:00, sven@riska.(none) +10 -0
>     New binlog needed by the test case.
>     
>
>   mysql-test/suite/binlog/std_data/binlog-bug32407.000001@stripped, 2007-12-10
> 10:42:43+01:00, sven@riska.(none) +0 -0
>
>   mysql-test/suite/binlog/t/binlog_base64_flag.test@stripped, 2007-12-10 10:42:43+01:00,
> sven@riska.(none) +107 -0
>     Added test case to verify that:
>      - my patch fixes the bug
>      - the new --base64-output flag works as expected
>      - base64 events not preceded by an FD event give an error
>      - an event of a type not known by the current FD event fails cleanly.
>     
>
>   mysql-test/suite/binlog/t/binlog_base64_flag.test@stripped, 2007-12-10 10:42:43+01:00,
> sven@riska.(none) +0 -0
>
>   mysql-test/suite/rpl/r/rpl_bug31076.result@stripped, 2007-12-10 10:42:42+01:00,
> sven@riska.(none) +4 -0
>     Updated result file
>
>   mysql-test/suite/rpl/r/rpl_row_mysqlbinlog.result@stripped, 2007-12-10 10:42:42+01:00,
> sven@riska.(none) +2 -0
>     Updated result file.
>
>   mysql-test/suite/rpl/t/rpl_bug31076.test@stripped, 2007-12-10 10:42:42+01:00,
> sven@riska.(none) +18 -0
>     The first BINLOG statement for a client must be a FD event.
>
>   mysql-test/t/mysqlbinlog2.test@stripped, 2007-12-10 10:42:42+01:00, sven@riska.(none)
> +1 -1
>     We must suppress base64 output because it contains a timestamp.
>
>   sql/log_event.cc@stripped, 2007-12-10 10:42:42+01:00, sven@riska.(none) +121 -87
>      - Made FD events able to print themselves
>      - Added check that the current FD event knows about the event type, when
>        an event is about to be read. (Hint to reviewers: I had to re-indent
>        a big block because of this; use diff -b)
>         * To get a decent error message, I also added a class method
>           const char* Log_event::get_type_str(Log_event_type)
>           which converts number to event type string without having a
>           Log_event object.
>         * Made Log_event::get_type_str aware of PRE_GA_*_ROWS_LOG_EVENT.
>      - Minor fixes:
>         * Changed return to DBUG_VOID_RETURN
>
>   sql/log_event.h@stripped, 2007-12-10 10:42:42+01:00, sven@riska.(none) +26 -3
>      - Declared enum to describe the three base64_output modes
>      - Use the enum instead of a flag
>      - Declare the new class method get_type_str (see log_event.cc)
>
>   sql/share/errmsg.txt@stripped, 2007-12-10 10:42:43+01:00, sven@riska.(none) +3 -0
>     Added error msg.
>
>   sql/slave.cc@stripped, 2007-12-10 10:42:42+01:00, sven@riska.(none) +207 -111
>      - Factored out part of exec_relay_log_event to the new function
>        apply_event_and_update_pos, because that code is needed when executing
>        BINLOG statements. (this should be functionally equivalent to the
>        previous code)
>   

"should"?!

If you are unsure if you can make the refactored code behave 
identically, then refactoring is dangerous.

>      - Changed returned error value for various errors from 1 to -1 in
>        exec_relay_log_event, because apply_event may return 1. This is mostly
>        cosmetic, because the return value from exec_relay_log_event is only
>        checked to be ==0 or !=0.
>   

Are you sure about this? Unless you are absolutely sure, these 
refactorings are dangerous (which is not the same as saying that you 
shouldn't do them, just that you should be aware of the fact and act 
accordingly).

>      - Added comments describing exec_relay_log_event and
>        apply_event_and_update_pos.
>      - Minor fixes:
>         * Removed duplicate declaration of next_event, made queue_event
>           static.
>         * Added doxygen code to include this file.
>
>   sql/slave.h@stripped, 2007-12-10 10:42:43+01:00, sven@riska.(none) +2 -0
>     Declared the new apply_event_and_update_pos
>
>   sql/sql_binlog.cc@stripped, 2007-12-10 10:42:43+01:00, sven@riska.(none) +57 -17
>      - Made mysql_binlog_statement set the current FD event when the given
>        event is an FD event. This entails using the new function
>        apply_event_and_update_pos from slave.cc instead of just calling the
>        ev->apply method.
>      - Made mysql_binlog_statement fail if the first BINLOG statement is not
>        an FD event.
>
> diff -Nrup a/client/client_priv.h b/client/client_priv.h
> --- a/client/client_priv.h	2007-10-23 16:02:23 +02:00
> +++ b/client/client_priv.h	2007-12-10 10:42:42 +01:00
> @@ -77,7 +77,7 @@ enum options_client
>    OPT_SLAP_POST_SYSTEM,
>    OPT_SLAP_COMMIT,
>    OPT_SLAP_DETACH,
> -  OPT_MYSQL_REPLACE_INTO, OPT_BASE64_OUTPUT, OPT_SERVER_ID,
> +  OPT_MYSQL_REPLACE_INTO, OPT_BASE64_OUTPUT_MODE, OPT_SERVER_ID,
>    OPT_FIX_TABLE_NAMES, OPT_FIX_DB_NAMES, OPT_SSL_VERIFY_SERVER_CERT,
>    OPT_DEBUG_INFO, OPT_DEBUG_CHECK, OPT_COLUMN_TYPES, OPT_ERROR_LOG_FILE,
>    OPT_WRITE_BINLOG, OPT_DUMP_DATE,
> diff -Nrup a/client/mysqlbinlog.cc b/client/mysqlbinlog.cc
> --- a/client/mysqlbinlog.cc	2007-09-15 05:10:31 +02:00
> +++ b/client/mysqlbinlog.cc	2007-12-10 10:42:42 +01:00
> @@ -63,7 +63,12 @@ void sql_print_error(const char *format,
>  
>  static bool one_database=0, to_last_remote_log= 0, disable_log_bin= 0;
>  static bool opt_hexdump= 0;
> -static bool opt_base64_output= 0;
> +const char *base64_output_mode_names[]= {"NEVER", "AUTO", "ALWAYS", NullS};
> +TYPELIB base64_output_mode_typelib=
> +  { array_elements(base64_output_mode_names) - 1, "",
> +    base64_output_mode_names, NULL };
> +static enum_base64_output_mode opt_base64_output_mode= BASE64_OUTPUT_UNSPEC;
> +static const char *opt_base64_output_mode_str= NullS;
>  static const char* database= 0;
>  static my_bool force_opt= 0, short_form= 0, remote_opt= 0;
>  static my_bool debug_info_flag, debug_check_flag;
> @@ -96,7 +101,7 @@ static my_bool file_not_closed_error= 0;
>    This is because the event will be created (alloced) in read_log_event()
>    (which returns a pointer) in check_header().
>  */
> -Format_description_log_event* glob_description_event; 
> +static Format_description_log_event* glob_description_event;
>   

Not necessary for the patch, but correcting the scope of variables is a 
good thing (when the scope is made more restrictive).
 
>  
>  static int dump_local_log_entries(PRINT_EVENT_INFO *print_event_info,
>                                    const char* logname);
> @@ -557,7 +562,7 @@ int process_event(PRINT_EVENT_INFO *prin
>      else
>        print_event_info->hexdump_from= pos;
>  
> -    print_event_info->base64_output= opt_base64_output;
> +    print_event_info->base64_output_mode= opt_base64_output_mode;
>   

In general, I approve of changing names of variables or functions when 
they change behavior, so I think this is good.

>  
>      DBUG_PRINT("debug", ("event_type: %s", ev->get_type_str()));
>  
> @@ -565,7 +570,7 @@ int process_event(PRINT_EVENT_INFO *prin
>      case QUERY_EVENT:
>        if (check_database(((Query_log_event*)ev)->db))
>   

That code is just awful. Not yours, I know, but nevertheless...

>          goto end;
> -      if (opt_base64_output)
> +      if (opt_base64_output_mode == BASE64_OUTPUT_ALWAYS)
>          write_event_header_and_base64(ev, result_file, print_event_info);
>        else
>          ev->print(result_file, print_event_info);
>   

This is for QUERY_EVENT, and it should only print them in base64 for 
ALWAYS mode, so this code is correct.

> @@ -589,7 +594,7 @@ int process_event(PRINT_EVENT_INFO *prin
>  	filename and use LOCAL), prepared in the 'case EXEC_LOAD_EVENT' 
>  	below.
>        */
> -      if (opt_base64_output)
> +      if (opt_base64_output_mode == BASE64_OUTPUT_ALWAYS)
>        {
>          write_event_header_and_base64(ce, result_file, print_event_info);
>        }
>   

This is for CREATE_FILE_EVENT and it should only print them in base64 
for ALWAYS mode, so this code is correct.

> @@ -678,6 +683,38 @@ Create_file event for file_id: %u\n",exv
>  Begin_load_query event for file_id: %u\n", exlq->file_id);
>        break;
>      }
> +    case TABLE_MAP_EVENT:
> +    case WRITE_ROWS_EVENT:
> +    case DELETE_ROWS_EVENT:
> +    case UPDATE_ROWS_EVENT:
> +    case PRE_GA_WRITE_ROWS_EVENT:
> +    case PRE_GA_DELETE_ROWS_EVENT:
> +    case PRE_GA_UPDATE_ROWS_EVENT:
> +      /*
> +        These events must be printed in base64 format, if printed.
> +        base64 format requires a FD event to be safe, so if no FD
> +        event has been printed, we give an error.  Except if user
> +        passed --short-form, because --short-form disables printing
> +        row events.
> +      */
> +      if (!print_event_info->printed_fd_event && !short_form)
>   

Instead of introducing a new variable, you could check the mode. In AUTO 
and ALWAYS mode, a FD event shall have been printed earlier, so by just 
checking the mode, you can decide if you should print an error or not.

> +      {
> +        /*
> +          todo: a lot to clean up here
> +        */
> +        const char* type_str= ev->get_type_str();
> +        delete ev;
> +        if (opt_base64_output_mode == BASE64_OUTPUT_NEVER)
> +          die("--base64-output=never specified, but binlog contains a "
> +              "%s event which must be printed in base64.",
> +              type_str);
> +        else
> +          die("malformed binlog: it does not contain any "
> +              "Format_description_log_event. I now found a %s event, which is "
> +              "not safe to process without a Format_description_log_event.",
> +              type_str);
> +      }
> +      /* FALL THROUGH */
>      default:
>        ev->print(result_file, print_event_info);
>      }
> @@ -707,12 +744,17 @@ static struct my_option my_long_options[
>    {"autoclose", OPT_AUTO_CLOSE, "Auto close the screen on exit for Netware.",
>     0, 0, 0, GET_NO_ARG, NO_ARG, 0, 0, 0, 0, 0, 0},
>  #endif
> -  {"base64-output", OPT_BASE64_OUTPUT,
> -   "Print all binlog entries using base64 encoding. "
> -   "This is for debugging only. Logs produced using this option "
> -   "should not be applied on production systems.",
> -   (uchar**) &opt_base64_output, (uchar**) &opt_base64_output, 0, GET_BOOL,
> -   NO_ARG, 0, 0, 0, 0, 0, 0},
> +  {"base64-output", OPT_BASE64_OUTPUT_MODE,
> +   "Determine when the output statements should be base64-encoded BINLOG "
> +   "statements: 'never' disables it and works only for binlogs without "
> +   "row-based events; 'auto' is the default and prints base64 only when "
> +   "necessary (i.e., for row-based events and format description events); "
> +   "'always' prints base64 whenever possible (it is currently not always "
> +   "possible). 'always' is for debugging only and should not be used in a "
> +   "production system. The default is 'auto'."
>   

Remove the "(it is ...)", and don't give advice on how the option should 
be used in the description of the option, since this makes it difficult 
to maintain. The documentation is for giving suggestions on how and when 
to use options, not the code. IMHO, the option description should 
contain what the option does, and nothing else.

> +   ,(uchar**) &opt_base64_output_mode_str,
> +   (uchar**) &opt_base64_output_mode_str,
> +   0, GET_STR, OPT_ARG, 0, 0, 0, 0, 0, 0},
>    /*
>      mysqlbinlog needs charsets knowledge, to be able to convert a charset
>      number found in binlog to a charset name (to be able to print things
> @@ -788,7 +830,10 @@ static struct my_option my_long_options[
>    {"set-charset", OPT_SET_CHARSET,
>     "Add 'SET NAMES character_set' to the output.", (uchar**) &charset,
>     (uchar**) &charset, 0, GET_STR, REQUIRED_ARG, 0, 0, 0, 0, 0, 0},
> -  {"short-form", 's', "Just show the queries, no extra info.",
> +  {"short-form", 's', "Just show regular queries: no extra info and no "
> +   "row-based events. This is for testing only, and should not be used in "
> +   "production systems. If you want to suppress base64-output, consider "
> +   "using --base64-output=never instead.",
>   

See above.

>     (uchar**) &short_form, (uchar**) &short_form, 0, GET_BOOL, NO_ARG, 0, 0,
> 0, 0,
>     0, 0},
>    {"socket", 'S', "Socket file to use for connection.",
> @@ -973,6 +1018,15 @@ get_one_option(int optid, const struct m
>    case OPT_STOP_DATETIME:
>      stop_datetime= convert_str_to_timestamp(stop_datetime_str);
>      break;
> +  case OPT_BASE64_OUTPUT_MODE:
> +    if (argument == NULL)
> +      opt_base64_output_mode= BASE64_OUTPUT_ALWAYS;
> +    else
> +    {
> +      opt_base64_output_mode= (enum_base64_output_mode)
> +        (find_type_or_exit(argument, &base64_output_mode_typelib,
> opt->name)-1);
> +    }
> +    break;
>    case 'V':
>      print_version();
>      exit(0);
> @@ -1305,8 +1359,21 @@ err:
>  }
>  
>  
> +/**
> +  Reads the Format_description_log_event from the beginning of the
> +  input file.
> +
> +  The Format_description_log_event is only read if it is outside the
> +  range specified with --start-position; otherwise, it will be seen
> +  later.  If this is an old binlog, a fake Format_description_event is
> +  created.  This also prints a Format_description_log_event to the
> +  output, unless we reach the --start-position range. In this case, it
> +  is assumed that a Format_description_log_event will be found when
> +  reading events the usual way.
>   

Please add parameters to the Doxygen comment.

> +*/
>  static void check_header(IO_CACHE* file, 
> -                        Format_description_log_event **description_event) 
> +                         Format_description_log_event **description_event,
> +                         PRINT_EVENT_INFO *print_event_info)
>  {
>    uchar header[BIN_LOG_HEADER_SIZE];
>    uchar buf[PROBE_HEADER_LEN];
> @@ -1369,10 +1436,11 @@ Could not read entry at offset %lu : Err
>      }
>      else
>      {
> -      DBUG_PRINT("info",("buf[4]=%d", buf[4]));
> +      DBUG_PRINT("info",("buf[EVENT_TYPE_OFFSET]=%d", buf[EVENT_TYPE_OFFSET]));
>   

It is more useful to have the actual value of the event type offset in 
the debug output.

>        /* always test for a Start_v3, even if no --start-position */
> -      if (buf[4] == START_EVENT_V3)       /* This is 3.23 or 4.x */
> +      if (buf[EVENT_TYPE_OFFSET] == START_EVENT_V3)
>        {
> +        /* This is 3.23 or 4.x */
>          if (uint4korr(buf + EVENT_LEN_OFFSET) < 
>              (LOG_EVENT_MINIMAL_HEADER_LEN + START_V3_HEADER_LEN))
>          {
> @@ -1384,8 +1452,9 @@ Could not read entry at offset %lu : Err
>        }
>        else if (tmp_pos >= start_position)
>          break;
> -      else if (buf[4] == FORMAT_DESCRIPTION_EVENT) /* This is 5.0 */
> +      else if (buf[EVENT_TYPE_OFFSET] == FORMAT_DESCRIPTION_EVENT)
>        {
> +        /* This is 5.0 */
>          Format_description_log_event *new_description_event;
>          my_b_seek(file, tmp_pos); /* seek back to event's start */
>          if (!(new_description_event= (Format_description_log_event*) 
> @@ -1397,11 +1466,22 @@ Could not read entry at offset %lu : Err
>  at offset %lu ; this could be a log format error or read error",
>                tmp_pos); 
>          }
> -        delete *description_event;
> -        *description_event= new_description_event;
> +        if (opt_base64_output_mode == BASE64_OUTPUT_AUTO
> +            || opt_base64_output_mode == BASE64_OUTPUT_ALWAYS)
> +          /*
> +            process_event will delete *description_event and set it to
> +            the new one, so we should not do it ourselves in this
> +            case.
> +          */
>   

If feels kind of strange to have one branch of the if-statement turning 
over the responsibility of deleting the event to another function and 
the other one not. Makes maintenance harder.

> +          process_event(print_event_info, new_description_event, tmp_pos);
> +        else
> +        {
> +          delete *description_event;
> +          *description_event= new_description_event;
> +        }
>   

Don't you need the FD event event even for NEVER mode? Don't if work if 
the FD event is always available? If you always set the FD event, it 
makes this code easier and more generic and also gives the possibility 
of using the FD event for checking versions even in NEVER mode.

Also, it seems that you are using the existence of the FD event as an 
indicator for whether to print an error or not, and this logic will 
break down if the FD event for some reason is needed in the future; for 
example, if there is a need to do version checking even in NEVER mode.

I would suggest to always set the FD event and look at the mode instead 
to decide when an error should be printed. This makes the code simpler, 
more generic, and less prone to introducing bugs with future changes. 
This also rhymes well with the fact that preconditions should be as weak 
as possible, but not weaker.

>          DBUG_PRINT("info",("Setting description_event"));
>        }
> -      else if (buf[4] == ROTATE_EVENT)
> +      else if (buf[EVENT_TYPE_OFFSET] == ROTATE_EVENT)
>        {
>          Log_event *ev;
>          my_b_seek(file, tmp_pos); /* seek back to event's start */
> @@ -1430,7 +1510,7 @@ static int dump_local_log_entries(PRINT_
>    uchar tmp_buff[BIN_LOG_HEADER_SIZE];
>    int error= 0;
>  
> -  if (logname && logname[0] != '-')
> +  if (logname && strcmp(logname, "-") != 0)
>    {
>      if ((fd = my_open(logname, O_RDONLY | O_BINARY, MYF(MY_WME))) < 0)
>        return 1;
> @@ -1440,7 +1520,7 @@ static int dump_local_log_entries(PRINT_
>        my_close(fd, MYF(MY_WME));
>        return 1;
>      }
> -    check_header(file, &glob_description_event);
> +    check_header(file, &glob_description_event, print_event_info);
>    }
>    else // reading from stdin;
>    {
> @@ -1462,7 +1542,7 @@ static int dump_local_log_entries(PRINT_
>      if (init_io_cache(file, fileno(stdin), 0, READ_CACHE, (my_off_t) 0,
>  		      0, MYF(MY_WME | MY_NABP | MY_DONT_CHECK_FILESIZE)))
>        return 1;
> -    check_header(file, &glob_description_event);
> +    check_header(file, &glob_description_event, print_event_info);
>      if (start_position)
>      {
>        /* skip 'start_position' characters from stdin */
> @@ -1553,6 +1633,9 @@ int main(int argc, char** argv)
>      free_defaults(defaults_argv);
>      exit(1);
>    }
> +
> +  if (opt_base64_output_mode == BASE64_OUTPUT_UNSPEC)
> +    opt_base64_output_mode= BASE64_OUTPUT_AUTO;
>  
>    my_set_max_open_files(open_files_limit);
>  
> diff -Nrup a/mysql-test/r/mysqlbinlog2.result b/mysql-test/r/mysqlbinlog2.result
> --- a/mysql-test/r/mysqlbinlog2.result	2007-08-08 18:20:59 +02:00
> +++ b/mysql-test/r/mysqlbinlog2.result	2007-12-10 10:42:42 +01:00
> @@ -106,6 +106,7 @@ ROLLBACK /* added by mysqlbinlog */;
>  /*!40019 SET @@session.max_insert_delayed_threads=0*/;
>  /*!50003 SET @OLD_COMPLETION_TYPE=@@COMPLETION_TYPE,COMPLETION_TYPE=0*/;
>  DELIMITER /*!*/;
> +ROLLBACK/*!*/;
>  SET INSERT_ID=4/*!*/;
>  use test/*!*/;
>  SET TIMESTAMP=1579609946/*!*/;
> @@ -152,6 +153,7 @@ ROLLBACK /* added by mysqlbinlog */;
>  /*!40019 SET @@session.max_insert_delayed_threads=0*/;
>  /*!50003 SET @OLD_COMPLETION_TYPE=@@COMPLETION_TYPE,COMPLETION_TYPE=0*/;
>  DELIMITER /*!*/;
> +ROLLBACK/*!*/;
>  SET INSERT_ID=4/*!*/;
>  use test/*!*/;
>  SET TIMESTAMP=1579609946/*!*/;
> @@ -299,6 +301,7 @@ ROLLBACK /* added by mysqlbinlog */;
>  /*!40019 SET @@session.max_insert_delayed_threads=0*/;
>  /*!50003 SET @OLD_COMPLETION_TYPE=@@COMPLETION_TYPE,COMPLETION_TYPE=0*/;
>  DELIMITER /*!*/;
> +ROLLBACK/*!*/;
>  SET INSERT_ID=4/*!*/;
>  use test/*!*/;
>  SET TIMESTAMP=1579609946/*!*/;
> diff -Nrup a/mysql-test/suite/binlog/r/binlog_base64_flag.result
> b/mysql-test/suite/binlog/r/binlog_base64_flag.result
> --- /dev/null	Wed Dec 31 16:00:00 196900
> +++ b/mysql-test/suite/binlog/r/binlog_base64_flag.result	2007-12-10 10:42:43 +01:00
> @@ -0,0 +1,46 @@
> +==== Test BUG#32407 ====
> +select * from t1;
> +a
> +1
> +1
> +==== Test BINLOG statement w/o FD event ====
> +BINLOG '
> +SVtYRxMBAAAAKQAAADQBAAAAABAAAAAAAAAABHRlc3QAAnQxAAEDAAE=
> +SVtYRxcBAAAAIgAAAFYBAAAQABAAAAAAAAEAAf/+AgAAAA==
> +';
> +ERROR HY000: The BINLOG statement of type `Table_map` was not preceded by a format
> description BINLOG statement.
> +select * from t1;
> +a
> +1
> +1
> +==== Test BINLOG statement with FD event ====
> +BINLOG '
> +ODdYRw8BAAAAZgAAAGoAAAABAAQANS4xLjIzLXJjLWRlYnVnLWxvZwAAAAAAAAAAAAAAAAAAAAAA
> +AAAAAAAAAAAAAAAAAAA4N1hHEzgNAAgAEgAEBAQEEgAAUwAEGggAAAAICAgC
> +';
> +BINLOG '
> +TFtYRxMBAAAAKQAAAH8BAAAAABAAAAAAAAAABHRlc3QAAnQxAAEDAAE=
> +TFtYRxcBAAAAIgAAAKEBAAAQABAAAAAAAAEAAf/+AwAAAA==
> +';
> +select * from t1;
> +a
> +1
> +1
> +3
> +==== Test --base64-output=never on a binlog with row events ====
> +==== Test non-matching FD event and Row event ====
> +BINLOG '
> +4CdYRw8BAAAAYgAAAGYAAAAAAAQANS4xLjE1LW5kYi02LjEuMjQtZGVidWctbG9nAAAAAAAAAAAA
> +AAAAAAAAAAAAAAAAAADgJ1hHEzgNAAgAEgAEBAQEEgAATwAEGggICAg=
> +';
> +BINLOG '
> +Dl1YRxMBAAAAKQAAADQBAAAAABAAAAAAAAAABHRlc3QAAnQxAAEDAAE=
> +Dl1YRxcBAAAAIgAAAFYBAAAQABAAAAAAAAEAAf/+BQAAAA==
> +';
> +ERROR 42000: You have an error in your SQL syntax; check the manual that corresponds
> to your MySQL server version for the right syntax to use
> +select * from t1;
> +a
> +1
> +1
> +3
> +drop table t1;
> Binary files a/mysql-test/suite/binlog/std_data/binlog-bug32407.000001 and
> b/mysql-test/suite/binlog/std_data/binlog-bug32407.000001 differ
> diff -Nrup a/mysql-test/suite/binlog/t/binlog_base64_flag.test
> b/mysql-test/suite/binlog/t/binlog_base64_flag.test
> --- /dev/null	Wed Dec 31 16:00:00 196900
> +++ b/mysql-test/suite/binlog/t/binlog_base64_flag.test	2007-12-10 10:42:43 +01:00
> @@ -0,0 +1,107 @@
> +# This test case verifies that the mysqlbinlog --base64-output=X flags
> +# work as expected, and that BINLOG statements with row events fail if
> +# they are not preceded by BINLOG statements with Format description
> +# events.
> +#
> +# See also BUG#32407.
> +#
> +# Created by Sven.
>   

You can remove your names from the test cases, which reminds me that you 
should add your name to the AUTHORS output.

I know some add themselves as creator in the test case file, but over 
time this file will not be anything like the original, and I really do 
not think you should take responsibility for what happens with this file 
in the future. If anybody really want to know who created it, there is a 
revision history maintained by BitKeeper for this purpose.

> +
> +# Test to show BUG#32407.  This reads a binlog created with the
> +# mysql-5.1-telco-6.1 tree, specifically at the tag
> +# mysql-5.1.15-ndb-6.1.23, and applies it to the database.  The test
> +# should fail before BUG#32407 was fixed and succeed afterwards.
> +--echo ==== Test BUG#32407 ====
> +
> +# The binlog contains row events equivalent to:
> +# CREATE TABLE t1 (a int) engine = myisam
> +# INSERT INTO t1 VALUES (1), (1)
> +--exec $MYSQL_BINLOG suite/binlog/std_data/binlog-bug32407.000001 | $MYSQL
> +# The above line should succeed and t1 should contain two ones
> +select * from t1;
> +
> +
> +# Test that a BINLOG statement encoding a row event fails unless a
> +# Format_description_event as been supplied with an earlier BINLOG
> +# statement.
> +--echo ==== Test BINLOG statement w/o FD event ====
> +
> +# This is a binlog statement consisting of one Table_map_log_event and
> +# one Write_rows_log_event.  Together, they correspond to the
> +# following query:
> +# INSERT INTO TABLE test.t1 VALUES (2)
> +
> +--error ER_NO_FORMAT_DESCRIPTION_EVENT_BEFORE_BINLOG_STATEMENT
>   

Use non -- form of the test statement: that means that you will see any 
errors in the command. (I usually only use -- for echo since the ; last 
makes one believe that the ; will be written as well.)

> +BINLOG '
> +SVtYRxMBAAAAKQAAADQBAAAAABAAAAAAAAAABHRlc3QAAnQxAAEDAAE=
> +SVtYRxcBAAAAIgAAAFYBAAAQABAAAAAAAAEAAf/+AgAAAA==
> +';
> +# The above line should fail and 2 should not be in the table
> +select * from t1;
> +
> +
> +# Test that it works to read a Format_description_log_event with a
> +# BINLOG statement, followed by a row-event in base64 from the same
> +# version.
> +--echo ==== Test BINLOG statement with FD event ====
> +
> +# This is a binlog statement containing a Format_description_log_event
> +# from the same version as the Table_map and Write_rows_log_event.
> +BINLOG '
> +ODdYRw8BAAAAZgAAAGoAAAABAAQANS4xLjIzLXJjLWRlYnVnLWxvZwAAAAAAAAAAAAAAAAAAAAAA
> +AAAAAAAAAAAAAAAAAAA4N1hHEzgNAAgAEgAEBAQEEgAAUwAEGggAAAAICAgC
> +';
> +
> +# This is a Table_map_log_event+Write_rows_log_event corresponding to:
> +# INSERT INTO TABLE test.t1 VALUES (3)
> +BINLOG '
> +TFtYRxMBAAAAKQAAAH8BAAAAABAAAAAAAAAABHRlc3QAAnQxAAEDAAE=
> +TFtYRxcBAAAAIgAAAKEBAAAQABAAAAAAAAEAAf/+AwAAAA==
> +';
> +# The above line should succeed and 3 should be in the table
> +select * from t1;
> +
> +
> +# Test that mysqlbinlog stops with an error message when the
> +# --base64-output=never flag is used on a binlog with base64 events.
> +#
> +# This test is currently commented out, because mysqlbinlog is not
> +# able to clean up after itself when stopped, so in debug mode it
> +# produces lots of messages about memory leaks.
> +--echo ==== Test --base64-output=never on a binlog with row events ====
> +#
> +# mysqlbinlog should fail
> +#--replace_regex /exec of .*suite/binlog/std_data/binlog-bug32407.000001/#/
> +#--exec $MYSQL_BINLOG --base64-output=never
> suite/binlog/std_data/binlog-bug32407.000001
> +# the above line should output the query log event and then stop
>   

Please enable the test. If mysqlbinlog contain a bug, we should report 
it and fix it. If there is no bug reported for this, please create one.

> +
> +
> +# Test that the following fails cleanly: "First, read a
> +# Format_description event which has N event types. Then, read an
> +# event of type M>N"
> +--echo ==== Test non-matching FD event and Row event ====
> +
> +# This is the Format_description_log_event from
> +# binlog-bug32407.000001, encoded in base64. It contains only the old
> +# row events (number of event types is 22)
> +
> +BINLOG '
> +4CdYRw8BAAAAYgAAAGYAAAAAAAQANS4xLjE1LW5kYi02LjEuMjQtZGVidWctbG9nAAAAAAAAAAAA
> +AAAAAAAAAAAAAAAAAADgJ1hHEzgNAAgAEgAEBAQEEgAATwAEGggICAg=
> +';
> +
> +# The following is a Write_rows_log_event with event type 23, i.e.,
> +# not supported by the Format_description_log_event above.  It
> +# corresponds to the following query:
> +# INSERT INTO t1 VALUES (5)
> +--error 1149
> +BINLOG '
> +Dl1YRxMBAAAAKQAAADQBAAAAABAAAAAAAAAABHRlc3QAAnQxAAEDAAE=
> +Dl1YRxcBAAAAIgAAAFYBAAAQABAAAAAAAAEAAf/+BQAAAA==
> +';
> +# the above line should fail and 5 should not be in the binlog.
> +select * from t1;
> +
> +
> +# clean up
> +drop table t1;
> diff -Nrup a/mysql-test/suite/rpl/r/rpl_bug31076.result
> b/mysql-test/suite/rpl/r/rpl_bug31076.result
> --- a/mysql-test/suite/rpl/r/rpl_bug31076.result	2007-11-14 11:01:40 +01:00
> +++ b/mysql-test/suite/rpl/r/rpl_bug31076.result	2007-12-10 10:42:42 +01:00
> @@ -40,6 +40,10 @@ KEY `data` (`data`)
>  /*!40019 SET @@session.max_insert_delayed_threads=0*/;
>  /*!50003 SET @OLD_COMPLETION_TYPE=@@COMPLETION_TYPE,COMPLETION_TYPE=0*/;
>  BINLOG '
> +O1ZVRw8BAAAAZgAAAGoAAAAAAAQANS4xLjIzLXJjLWRlYnVnLWxvZwAAAAAAAAAAAAAAAAAAAAAA
> +AAAAAAAAAAAAAAAAAAA7VlVHEzgNAAgAEgAEBAQEEgAAUwAEGggAAAAICAgC
> +'/*!*/;
> +BINLOG '
>  Bk3vRhO0AQAAOAAAALcLyQkAAJlXFwIAAAAABXRyYWNrAA12aXNpdHNfZXZlbnRzAAYJAwcPDwM=
>  Bk3vRhe0AQAAWgAAABEMyQkQAJlXFwIAAAEABv/AIE4AvvVDAQZN70YAK0Rvd25sb2Fkcy9NeVNR
>  TC00LjEvbXlzcWwtNC4xLjEyYS13aW4zMi56aXBPaAIC
> diff -Nrup a/mysql-test/suite/rpl/r/rpl_row_mysqlbinlog.result
> b/mysql-test/suite/rpl/r/rpl_row_mysqlbinlog.result
> --- a/mysql-test/suite/rpl/r/rpl_row_mysqlbinlog.result	2007-06-27 14:27:32 +02:00
> +++ b/mysql-test/suite/rpl/r/rpl_row_mysqlbinlog.result	2007-12-10 10:42:42 +01:00
> @@ -155,6 +155,7 @@ c1	c3	c4	c5
>  /*!40019 SET @@session.max_insert_delayed_threads=0*/;
>  /*!50003 SET @OLD_COMPLETION_TYPE=@@COMPLETION_TYPE,COMPLETION_TYPE=0*/;
>  DELIMITER /*!*/;
> +ROLLBACK/*!*/;
>  use test/*!*/;
>  SET TIMESTAMP=1000000000/*!*/;
>  SET @@session.foreign_key_checks=1, @@session.sql_auto_is_null=1,
> @@session.unique_checks=1/*!*/;
> @@ -295,6 +296,7 @@ ROLLBACK /* added by mysqlbinlog */;
>  /*!40019 SET @@session.max_insert_delayed_threads=0*/;
>  /*!50003 SET @OLD_COMPLETION_TYPE=@@COMPLETION_TYPE,COMPLETION_TYPE=0*/;
>  DELIMITER /*!*/;
> +ROLLBACK/*!*/;
>  use test/*!*/;
>  SET TIMESTAMP=1000000000/*!*/;
>  SET @@session.foreign_key_checks=1, @@session.sql_auto_is_null=1,
> @@session.unique_checks=1/*!*/;
> diff -Nrup a/mysql-test/suite/rpl/t/rpl_bug31076.test
> b/mysql-test/suite/rpl/t/rpl_bug31076.test
> --- a/mysql-test/suite/rpl/t/rpl_bug31076.test	2007-11-14 11:01:40 +01:00
> +++ b/mysql-test/suite/rpl/t/rpl_bug31076.test	2007-12-10 10:42:42 +01:00
> @@ -39,6 +39,24 @@ CREATE TABLE `visits_events` (
>  /*!40019 SET @@session.max_insert_delayed_threads=0*/;
>  /*!50003 SET @OLD_COMPLETION_TYPE=@@COMPLETION_TYPE,COMPLETION_TYPE=0*/;
>  --delimiter /*!*/;
> +
> +# at 4 (0x4)
> +#071204 14:29:31 server id 1  end_log_pos 106 
> +# Position  Timestamp   Type   Master ID        Size      Master Pos    Flags
> +#        4 3b 56 55 47   0f   01 00 00 00   66 00 00 00   6a 00 00 00   00 00
> +#       17 04 00 35 2e 31 2e 32 33  2d 72 63 2d 64 65 62 75 |..5.1.23.rc.debu|
> +#       27 67 2d 6c 6f 67 00 00 00  00 00 00 00 00 00 00 00 |g.log...........|
> +#       37 00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00 |................|
> +#       47 00 00 00 00 3b 56 55 47  13 38 0d 00 08 00 12 00 |.....VUG.8......|
> +#       57 04 04 04 04 12 00 00 53  00 04 1a 08 00 00 00 08 |.......S........|
> +#       67 08 08 02                                         |...|
> +#       Start: binlog v 4, server v 5.1.23-rc-debug-log created 071204 14:29:31 at
> startup
> +
> +BINLOG '
> +O1ZVRw8BAAAAZgAAAGoAAAAAAAQANS4xLjIzLXJjLWRlYnVnLWxvZwAAAAAAAAAAAAAAAAAAAAAA
> +AAAAAAAAAAAAAAAAAAA7VlVHEzgNAAgAEgAEBAQEEgAAUwAEGggAAAAICAgC
> +'/*!*/;
> +
>  # at 164170623
>  # at 164170679
>  #7918 3:59:2 server id 436  end_log_pos 164170679 
> diff -Nrup a/mysql-test/t/mysqlbinlog2.test b/mysql-test/t/mysqlbinlog2.test
> --- a/mysql-test/t/mysqlbinlog2.test	2007-08-08 18:20:59 +02:00
> +++ b/mysql-test/t/mysqlbinlog2.test	2007-12-10 10:42:42 +01:00
> @@ -42,7 +42,7 @@ select "--- Local --" as "";
>  #
>  
>  --replace_regex /[[:<:]][0-9]{6} [0-9 ][0-9]:[0-9]{2}:[0-9]{2}[[:>:]]/{yymmdd}
> {HH:MM:SS}/  /=[0-9]+	/={integer}	/  /# at [0-9]+/# at {pos}/  /(pos:?) [0-9]+/\1 {pos}/ 
> /binlog v [0-9]+, server v [^ ]* created/binlog v #, server v ## created/
> ---exec $MYSQL_BINLOG $MYSQLTEST_VARDIR/log/master-bin.000001 
> +--exec $MYSQL_BINLOG --base64-output=never $MYSQLTEST_VARDIR/log/master-bin.000001 
>  
>  --disable_query_log
>  select "--- offset --" as "";
> 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-10 10:42:42 +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";
> +  case PRE_GA_UPDATE_ROWS_EVENT: return "Update_rows_old";
> +  case PRE_GA_DELETE_ROWS_EVENT: return "Delete_rows_old";
>    case BEGIN_LOAD_QUERY_EVENT: return "Begin_load_query";
>    case EXECUTE_LOAD_QUERY_EVENT: return "Execute_load_query";
>    case INCIDENT_EVENT: return "Incident";
> @@ -500,6 +503,11 @@ const char* Log_event::get_type_str()
>    }
>  }
>  
> +const char* Log_event::get_type_str()
> +{
> +  return get_type_str(get_type_code());
> +}
>   

Ah, you did not duplicate the code. Excellent!

> +
>  
>  /*
>    Log_event::Log_event()
> @@ -1024,94 +1032,112 @@ Log_event* Log_event::read_log_event(con
>      DBUG_RETURN(NULL); // general sanity check - will fail on a partial read
>    }
>  
> -  switch(buf[EVENT_TYPE_OFFSET]) {
> -  case QUERY_EVENT:
> -    ev  = new Query_log_event(buf, event_len, description_event, QUERY_EVENT);
> -    break;
> -  case LOAD_EVENT:
> -    ev = new Load_log_event(buf, event_len, description_event);
> -    break;
> -  case NEW_LOAD_EVENT:
> -    ev = new Load_log_event(buf, event_len, description_event);
> -    break;
> -  case ROTATE_EVENT:
> -    ev = new Rotate_log_event(buf, event_len, description_event);
> -    break;
> +  Log_event_type event_type= (Log_event_type)buf[EVENT_TYPE_OFFSET];
> +  if (event_type < 0 ||
> +      (event_type > description_event->number_of_event_types &&
> +       event_type != FORMAT_DESCRIPTION_EVENT))
>   

Since you are treating the event_type as an integer, I suggest you use 
an int instead of Log_event_type. The implicit cast of 
FORMAT_DESCRIPTION_EVENT does not produce an error (AIUI), so that is 
safe, and the other comparisons will do an implicit cast of the variable 
to an int anyway. The explicit cast might potentially hide errors, and 
using Log_event_type does not provide any safety anyway, so there is no 
value in using Log_event_type.

> +  {
> +    /*
> +      It is unsafe to use the description_event if its post_header_len
> +      array does not include the event type.
> +    */
> +    DBUG_PRINT("error", ("event type %d found, but the current "
> +                         "Format_description_log_event supports only %d event "
> +                         "types", event_type,
> +                         description_event->number_of_event_types));
> +    ev= NULL;
> +  }
> +  else
> +  {
> +    switch(event_type) {
> +    case QUERY_EVENT:
> +      ev  = new Query_log_event(buf, event_len, description_event, QUERY_EVENT);
> +      break;
> +    case LOAD_EVENT:
> +      ev = new Load_log_event(buf, event_len, description_event);
> +      break;
> +    case NEW_LOAD_EVENT:
> +      ev = new Load_log_event(buf, event_len, description_event);
> +      break;
> +    case ROTATE_EVENT:
> +      ev = new Rotate_log_event(buf, event_len, description_event);
> +      break;
>  #ifdef HAVE_REPLICATION
> -  case SLAVE_EVENT: /* can never happen (unused event) */
> -    ev = new Slave_log_event(buf, event_len);
> -    break;
> +    case SLAVE_EVENT: /* can never happen (unused event) */
> +      ev = new Slave_log_event(buf, event_len);
> +      break;
>  #endif /* HAVE_REPLICATION */
> -  case CREATE_FILE_EVENT:
> -    ev = new Create_file_log_event(buf, event_len, description_event);
> -    break;
> -  case APPEND_BLOCK_EVENT:
> -    ev = new Append_block_log_event(buf, event_len, description_event);
> -    break;
> -  case DELETE_FILE_EVENT:
> -    ev = new Delete_file_log_event(buf, event_len, description_event);
> -    break;
> -  case EXEC_LOAD_EVENT:
> -    ev = new Execute_load_log_event(buf, event_len, description_event);
> -    break;
> -  case START_EVENT_V3: /* this is sent only by MySQL <=4.x */
> -    ev = new Start_log_event_v3(buf, description_event);
> -    break;
> -  case STOP_EVENT:
> -    ev = new Stop_log_event(buf, description_event);
> -    break;
> -  case INTVAR_EVENT:
> -    ev = new Intvar_log_event(buf, description_event);
> -    break;
> -  case XID_EVENT:
> -    ev = new Xid_log_event(buf, description_event);
> -    break;
> -  case RAND_EVENT:
> -    ev = new Rand_log_event(buf, description_event);
> -    break;
> -  case USER_VAR_EVENT:
> -    ev = new User_var_log_event(buf, description_event);
> -    break;
> -  case FORMAT_DESCRIPTION_EVENT:
> -    ev = new Format_description_log_event(buf, event_len, description_event); 
> -    break;
> +    case CREATE_FILE_EVENT:
> +      ev = new Create_file_log_event(buf, event_len, description_event);
> +      break;
> +    case APPEND_BLOCK_EVENT:
> +      ev = new Append_block_log_event(buf, event_len, description_event);
> +      break;
> +    case DELETE_FILE_EVENT:
> +      ev = new Delete_file_log_event(buf, event_len, description_event);
> +      break;
> +    case EXEC_LOAD_EVENT:
> +      ev = new Execute_load_log_event(buf, event_len, description_event);
> +      break;
> +    case START_EVENT_V3: /* this is sent only by MySQL <=4.x */
> +      ev = new Start_log_event_v3(buf, description_event);
> +      break;
> +    case STOP_EVENT:
> +      ev = new Stop_log_event(buf, description_event);
> +      break;
> +    case INTVAR_EVENT:
> +      ev = new Intvar_log_event(buf, description_event);
> +      break;
> +    case XID_EVENT:
> +      ev = new Xid_log_event(buf, description_event);
> +      break;
> +    case RAND_EVENT:
> +      ev = new Rand_log_event(buf, description_event);
> +      break;
> +    case USER_VAR_EVENT:
> +      ev = new User_var_log_event(buf, description_event);
> +      break;
> +    case FORMAT_DESCRIPTION_EVENT:
> +      ev = new Format_description_log_event(buf, event_len, description_event);
> +      break;
>  #if defined(HAVE_REPLICATION) 
> -  case PRE_GA_WRITE_ROWS_EVENT:
> -    ev = new Write_rows_log_event_old(buf, event_len, description_event);
> -    break;
> -  case PRE_GA_UPDATE_ROWS_EVENT:
> -    ev = new Update_rows_log_event_old(buf, event_len, description_event);
> -    break;
> -  case PRE_GA_DELETE_ROWS_EVENT:
> -    ev = new Delete_rows_log_event_old(buf, event_len, description_event);
> -    break;
> -  case WRITE_ROWS_EVENT:
> -    ev = new Write_rows_log_event(buf, event_len, description_event);
> -    break;
> -  case UPDATE_ROWS_EVENT:
> -    ev = new Update_rows_log_event(buf, event_len, description_event);
> -    break;
> -  case DELETE_ROWS_EVENT:
> -    ev = new Delete_rows_log_event(buf, event_len, description_event);
> -    break;
> -  case TABLE_MAP_EVENT:
> -    ev = new Table_map_log_event(buf, event_len, description_event);
> -    break;
> +    case PRE_GA_WRITE_ROWS_EVENT:
> +      ev = new Write_rows_log_event_old(buf, event_len, description_event);
> +      break;
> +    case PRE_GA_UPDATE_ROWS_EVENT:
> +      ev = new Update_rows_log_event_old(buf, event_len, description_event);
> +      break;
> +    case PRE_GA_DELETE_ROWS_EVENT:
> +      ev = new Delete_rows_log_event_old(buf, event_len, description_event);
> +      break;
> +    case WRITE_ROWS_EVENT:
> +      ev = new Write_rows_log_event(buf, event_len, description_event);
> +      break;
> +    case UPDATE_ROWS_EVENT:
> +      ev = new Update_rows_log_event(buf, event_len, description_event);
> +      break;
> +    case DELETE_ROWS_EVENT:
> +      ev = new Delete_rows_log_event(buf, event_len, description_event);
> +      break;
> +    case TABLE_MAP_EVENT:
> +      ev = new Table_map_log_event(buf, event_len, description_event);
> +      break;
>  #endif
> -  case BEGIN_LOAD_QUERY_EVENT:
> -    ev = new Begin_load_query_log_event(buf, event_len, description_event);
> -    break;
> -  case EXECUTE_LOAD_QUERY_EVENT:
> -    ev= new Execute_load_query_log_event(buf, event_len, description_event);
> -    break;
> -  case INCIDENT_EVENT:
> -    ev = new Incident_log_event(buf, event_len, description_event);
> -    break;
> -  default:
> -    DBUG_PRINT("error",("Unknown event code: %d",
> -                        (int) buf[EVENT_TYPE_OFFSET]));
> -    ev= NULL;
> -    break;
> +    case BEGIN_LOAD_QUERY_EVENT:
> +      ev = new Begin_load_query_log_event(buf, event_len, description_event);
> +      break;
> +    case EXECUTE_LOAD_QUERY_EVENT:
> +      ev= new Execute_load_query_log_event(buf, event_len, description_event);
> +      break;
> +    case INCIDENT_EVENT:
> +      ev = new Incident_log_event(buf, event_len, description_event);
> +      break;
> +    default:
> +      DBUG_PRINT("error",("Unknown event code: %d",
> +                          (int) buf[EVENT_TYPE_OFFSET]));
> +      ev= NULL;
> +      break;
> +    }
>    }
>  
>    DBUG_PRINT("read_event", ("%s(type_code: %d; event_len: %d)",
> @@ -2577,6 +2603,14 @@ void Start_log_event_v3::print(FILE* fil
>      my_b_printf(&cache,"ROLLBACK%s\n", print_event_info->delimiter);
>  #endif
>    }
> +  if (temp_buf &&
> +      print_event_info->base64_output_mode != BASE64_OUTPUT_NEVER &&
> +      !print_event_info->short_form)
> +  {
> +    my_b_printf(&cache, "BINLOG '\n");
> +    print_base64(&cache, print_event_info, FALSE);
> +    print_event_info->printed_fd_event= TRUE;
> +  }
>    DBUG_VOID_RETURN;
>  }
>  #endif /* MYSQL_CLIENT */
> @@ -5022,7 +5056,7 @@ Create_file_log_event::Create_file_log_e
>                     Load_log_event::get_data_size() +
>                     create_file_header_len + 1);
>      if (len < block_offset)
> -      return;
> +      DBUG_VOID_RETURN;
>      block = (char*)buf + block_offset;
>      block_len = len - block_offset;
>    }
> diff -Nrup a/sql/log_event.h b/sql/log_event.h
> --- a/sql/log_event.h	2007-10-30 09:51:08 +01:00
> +++ b/sql/log_event.h	2007-12-10 10:42:42 +01:00
> @@ -567,6 +567,15 @@ class Format_description_log_event;
>  class Relay_log_info;
>  
>  #ifdef MYSQL_CLIENT
> +enum enum_base64_output_mode {
> +  BASE64_OUTPUT_NEVER= 0,
> +  BASE64_OUTPUT_AUTO= 1,
> +  BASE64_OUTPUT_ALWAYS= 2,
> +  BASE64_OUTPUT_UNSPEC= 3,
> +  /* insert new output modes here */
> +  BASE64_OUTPUT_LAST_TYPE
> +};
>   

Since variables declared with this enum defaults to 0 (unless auto, of 
course), I would suggest to place the "UNSPEC" as 0, because that allows 
you to get a sensible and detectable value if you forget to assign a 
value to a variable of this type.

Then, since the enumeration only needs a sequence of number, and it does 
not matter which ones, I would say that you can remove the explicit  
values for the enumeration constants.

Also, LAST_TYPE is not the last type, it is *one more* than the value of 
the last *enumeration constant*, and the closest description to what it 
is would be a count of the number of enumeration constants. (Assuming 
that you don't have holes in the sequence above.)

I suggest to use the name BASE64_OUTPUT_MODE_COUNT instead. (I have the 
convention for enums that I add the constant <enum-name>_COUNT last.)
 
> +
>  /*
>    A structure for mysqlbinlog to know how to print events
>  
> @@ -600,7 +609,8 @@ typedef struct st_print_event_info
>    st_print_event_info()
>      :flags2_inited(0), sql_mode_inited(0),
>       auto_increment_increment(1),auto_increment_offset(1), charset_inited(0),
> -     lc_time_names_number(0), charset_database_number(0)
> +     lc_time_names_number(0), charset_database_number(0),
> +     base64_output_mode(BASE64_OUTPUT_UNSPEC), printed_fd_event(FALSE)
>   

Note: "UNSPEC" is the default.

>      {
>        /*
>          Currently we only use static PRINT_EVENT_INFO objects, so zeroed at
> @@ -627,7 +637,14 @@ typedef struct st_print_event_info
>  
>    /* Settings on how to print the events */
>    bool short_form;
> -  bool base64_output;
> +  enum_base64_output_mode base64_output_mode;
> +  /*
> +    This is set whenever a Format_description_event is printed.
> +    Later, when an event is printed in base64, this flag is tested: if
> +    no Format_description_event has been seen, it is unsafe to print
> +    the base64 event, so an error message is generated.
> +  */
> +  bool printed_fd_event;
>    my_off_t hexdump_from;
>    uint8 common_header_len;
>    char delimiter[16];
> @@ -930,7 +947,13 @@ public:
>  				   const char **error,
>                                     const Format_description_log_event
>                                     *description_event);
> -  /* returns the human readable name of the event's type */
> +  /**
> +    Returns the human readable name of the given event type.
> +  */
> +  static const char* get_type_str(Log_event_type type);
> +  /**
> +    Returns the human readable name of this event's type.
> +  */
>    const char* get_type_str();
>  
>    /* Return start of query time or current time */
> diff -Nrup a/sql/share/errmsg.txt b/sql/share/errmsg.txt
> --- a/sql/share/errmsg.txt	2007-11-14 14:28:21 +01:00
> +++ b/sql/share/errmsg.txt	2007-12-10 10:42:43 +01:00
> @@ -6114,3 +6114,6 @@ ER_TRG_CANT_OPEN_TABLE
>  
>  ER_CANT_CREATE_SROUTINE
>    eng "Cannot create stored routine `%-.64s`. Check warnings"
> +
> +ER_NO_FORMAT_DESCRIPTION_EVENT_BEFORE_BINLOG_STATEMENT
> +  eng "The BINLOG statement of type `%s` was not preceded by a format description
> BINLOG statement."
> diff -Nrup a/sql/slave.cc b/sql/slave.cc
> --- a/sql/slave.cc	2007-11-21 21:09:23 +01:00
> +++ b/sql/slave.cc	2007-12-10 10:42:42 +01:00
> @@ -13,6 +13,17 @@
>     along with this program; if not, write to the Free Software
>     Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA */
>  
> +
> +/**
> +  @addtogroup Replication
>   

We should have two groups. One for internals and one for externals. I am 
undecided on what group names we should use, so you can stick with this 
for the time being.

> +  @{
> +
> +  @file
> +
> +  @brief Code to run the io thread and the sql thread on the
> +  replication slave.
> +*/
> +
>  #include "mysql_priv.h"
>  
>  #include <mysql.h>
> @@ -33,10 +44,6 @@
>  
>  #include "rpl_tblmap.h"
>  
> -int queue_event(Master_info* mi,const char* buf,ulong event_len);
> -static Log_event* next_event(Relay_log_info* rli);
> -
> -
>  #define FLAGSTR(V,F) ((V)&(F)?#F" ":"")
>  
>  #define MAX_SLAVE_RETRY_PAUSE 5
> @@ -132,6 +139,7 @@ static int create_table_from_dump(THD* t
>                                    const char* table_name, bool overwrite);
>  static int get_master_version_and_clock(MYSQL* mysql, Master_info* mi);
>  static Log_event* next_event(Relay_log_info* rli);
> +static int queue_event(Master_info* mi,const char* buf,ulong event_len);
>  static int terminate_slave_thread(THD *thd,
>                                    pthread_mutex_t* term_lock,
>                                    pthread_cond_t* term_cond,
> @@ -1757,6 +1765,181 @@ static int has_temporary_error(THD *thd)
>    DBUG_RETURN(0);
>  }
>  
> +
> +/**
> +  Applies the given event and advances the relay log position.
> +
> +  In essence, this function does:
> +
> +  <pre>
> +    ev->apply_event(rli);
> +    ev->update_pos(rli);
> +  </pre>
>   

You have @code and @endcode for this purpose.

> +
> +  But it also does some maintainance, such as skipping events if
> +  needed and reporting errors.
> +
> +  If the @c skip flag is set, then it is tested whether the event
> +  should be skipped, by looking at the slave_skip_counter and the
> +  server id.  The skip flag should be set when calling this from a
> +  replication thread but not set when executing an explicit BINLOG
> +  statement.
> +
> +  @retval 0 OK.
> +
> +  @retval 1 Error calling ev->apply_event().
> +
> +  @retval 2 No error calling ev->apply_event(), but error calling
> +  ev->update_pos().
>   

Skip the periods last in the sentences, they are not needed.

> +*/
> +int apply_event_and_update_pos(Log_event* ev, THD* thd, Relay_log_info* rli,
> +                               bool skip)
> +{
> +  int exec_res= 0;
> +
> +  DBUG_ENTER("apply_event_and_update_pos");
> +
> +  DBUG_PRINT("exec_event",("%s(type_code: %d; server_id: %d)",
> +                           ev->get_type_str(), ev->get_type_code(),
> +                           ev->server_id));
> +  DBUG_PRINT("info", ("thd->options: %s%s; rli->last_event_start_time: %lu",
> +                      FLAGSTR(thd->options, OPTION_NOT_AUTOCOMMIT),
> +                      FLAGSTR(thd->options, OPTION_BEGIN),
> +                      rli->last_event_start_time));
> +
> +  /*
> +    Execute the event to change the database and update the binary
> +    log coordinates, but first we set some data that is needed for
> +    the thread.
> +
> +    The event will be executed unless it is supposed to be skipped.
> +
> +    Queries originating from this server must be skipped.  Low-level
> +    events (Format_description_log_event, Rotate_log_event,
> +    Stop_log_event) from this server must also be skipped. But for
> +    those we don't want to modify 'group_master_log_pos', because
> +    these events did not exist on the master.
> +    Format_description_log_event is not completely skipped.
> +
> +    Skip queries specified by the user in 'slave_skip_counter'.  We
> +    can't however skip events that has something to do with the log
> +    files themselves.
> +
> +    Filtering on own server id is extremely important, to ignore
> +    execution of events created by the creation/rotation of the relay
> +    log (remember that now the relay log starts with its Format_desc,
> +    has a Rotate etc).
> +  */
> +
> +  thd->server_id = ev->server_id; // use the original server id for logging
> +  thd->set_time();                            // time the query
> +  thd->lex->current_select= 0;
> +  if (!ev->when)
> +    ev->when= my_time(0);
> +  ev->thd = thd; // because up to this point, ev->thd == 0
> +
> +  if (skip)
> +  {
> +    int reason= ev->shall_skip(rli);
> +    if (reason == Log_event::EVENT_SKIP_COUNT)
> +      --rli->slave_skip_counter;
> +    pthread_mutex_unlock(&rli->data_lock);
> +    if (reason == Log_event::EVENT_SKIP_NOT)
> +      exec_res= ev->apply_event(rli);
> +#ifndef DBUG_OFF
> +    /*
> +      This only prints information to the debug trace.
> +
> +      TODO: Print an informational message to the error log?
> +    */
> +    static const char *const explain[] = {
> +      // EVENT_SKIP_NOT,
> +      "not skipped",
> +      // EVENT_SKIP_IGNORE,
> +      "skipped because event should be ignored",
> +      // EVENT_SKIP_COUNT
> +      "skipped because event skip counter was non-zero"
> +    };
> +    DBUG_PRINT("info", ("OPTION_BEGIN: %d; IN_STMT: %d",
> +                        thd->options & OPTION_BEGIN ? 1 : 0,
> +                        rli->get_flag(Relay_log_info::IN_STMT)));
> +    DBUG_PRINT("skip_event", ("%s event was %s",
> +                              ev->get_type_str(), explain[reason]));
> +#endif
> +  }
> +  else
> +    exec_res= ev->apply_event(rli);
> +
> +  DBUG_PRINT("info", ("apply_event error = %d", exec_res));
> +  if (exec_res == 0)
> +  {
> +    int error= ev->update_pos(rli);
> +    char buf[22];
> +    DBUG_PRINT("info", ("update_pos error = %d", error));
> +    DBUG_PRINT("info", ("group %s %s",
> +                        llstr(rli->group_relay_log_pos, buf),
> +                        rli->group_relay_log_name));
> +    DBUG_PRINT("info", ("event %s %s",
> +                        llstr(rli->event_relay_log_pos, buf),
> +                        rli->event_relay_log_name));
> +    /*
> +      The update should not fail, so print an error message and
> +      return an error code.
> +
> +      TODO: Replace this with a decent error message when merged
> +      with BUG#24954 (which adds several new error message).
> +    */
> +    if (error)
> +    {
> +      rli->report(ERROR_LEVEL, ER_UNKNOWN_ERROR,
> +                  "It was not possible to update the positions"
> +                  " of the relay log information: the slave may"
> +                  " be in an inconsistent state."
> +                  " Stopped in %s position %s",
> +                  rli->group_relay_log_name,
> +                  llstr(rli->group_relay_log_pos, buf));
> +      DBUG_RETURN(2);
> +    }
> +  }
> +
> +  DBUG_RETURN(exec_res ? 1 : 0);
> +}
> +
> +
> +/**
> +  Top-level function for executing the next event from the relay log.
> +
> +  This function reads the event from the relay log, executes it, and
> +  advances the relay log position.  It also handles errors, etc.
> +
> +  @retval 0 OK.
> +
> +  @retval nonzero The event was not applied, for one of the following
> +  reasons:
> +
> +  <ul>
> +
> +  <li>The position specfied by the UNTIL condition of the START SLAVE
> +  command is reached.</li>
> +
> +  <li>It was not possible to read the event from the log.</li>
> +
> +  <li>The slave is killed.</li>
> +
> +  <li>An error occurred when applying the event, and the event has
> +  been tried slave_trans_retries times.  If the event has been retried
> +  fewer times, 0 is returned.</li>
> +
> +  <li>init_master_info or init_relay_log_pos failed. (These are called
> +  if a failure occurs when applying the event.)</li>
> +
> +  <li>An error occurred when updating the binlog position.</li>
> +
> +  </ul>
>   

The text associated with a return value should be brief. This text is 
better to put in the body of the description.

Also, Doxygen handles normal lists well, so there is no need to use HTML 
here. You can type:

    If the return value is non-zero, the event was not applied for one
    of the following reasons:

    - The position specfied by the UNTIL condition of the START SLAVE
      command is reached

    - It was not possible to read the event from the log

    - The slave is killed
      

> +
> +  The exact value returned is not meaningful; check the errors in the
> +  thd and rli objects for details.
> +*/
>   

I assume this is just a cut and paste of the code removed below. If that 
is not the case, please tell me and I will take a more thorough look.

>  static int exec_relay_log_event(THD* thd, Relay_log_info* rli)
>  {
>    DBUG_ENTER("exec_relay_log_event");
> @@ -1787,7 +1970,7 @@ static int exec_relay_log_event(THD* thd
>      */
>      rli->abort_slave= 1;
>      pthread_mutex_unlock(&rli->data_lock);
> -    DBUG_RETURN(1);
> +    DBUG_RETURN(-1);
>   

Please add this return value (-1) to the @retval list in the function 
comment.

>    }
>  
>    Log_event * ev = next_event(rli);
> @@ -1798,121 +1981,30 @@ static int exec_relay_log_event(THD* thd
>    {
>      pthread_mutex_unlock(&rli->data_lock);
>      delete ev;
> -    DBUG_RETURN(1);
> +    DBUG_RETURN(-1);
>   

See above.

>    }
>    if (ev)
>    {
> -    int const type_code= ev->get_type_code();
> -    int exec_res= 0;
> -
> -    DBUG_PRINT("exec_event",("%s(type_code: %d; server_id: %d)",
> -                       ev->get_type_str(), type_code, ev->server_id));
> -    DBUG_PRINT("info", ("thd->options: %s%s; rli->last_event_start_time:
> %lu",
> -                        FLAGSTR(thd->options, OPTION_NOT_AUTOCOMMIT),
> -                        FLAGSTR(thd->options, OPTION_BEGIN),
> -                        rli->last_event_start_time));
> +    int exec_res= apply_event_and_update_pos(ev, thd, rli, TRUE);
>  
> -
> -    /*
> -      Execute the event to change the database and update the binary
> -      log coordinates, but first we set some data that is needed for
> -      the thread.
> -
> -      The event will be executed unless it is supposed to be skipped.
> -
> -      Queries originating from this server must be skipped.  Low-level
> -      events (Format_description_log_event, Rotate_log_event,
> -      Stop_log_event) from this server must also be skipped. But for
> -      those we don't want to modify 'group_master_log_pos', because
> -      these events did not exist on the master.
> -      Format_description_log_event is not completely skipped.
> -
> -      Skip queries specified by the user in 'slave_skip_counter'.  We
> -      can't however skip events that has something to do with the log
> -      files themselves.
> -
> -      Filtering on own server id is extremely important, to ignore
> -      execution of events created by the creation/rotation of the relay
> -      log (remember that now the relay log starts with its Format_desc,
> -      has a Rotate etc).
> -    */
> -
> -    thd->server_id = ev->server_id; // use the original server id for logging
> -    thd->set_time();                            // time the query
> -    thd->lex->current_select= 0;
> -    if (!ev->when)
> -      ev->when= my_time(0);
> -    ev->thd = thd; // because up to this point, ev->thd == 0
> -
> -    int reason= ev->shall_skip(rli);
> -    if (reason == Log_event::EVENT_SKIP_COUNT)
> -      --rli->slave_skip_counter;
> -    pthread_mutex_unlock(&rli->data_lock);
> -    if (reason == Log_event::EVENT_SKIP_NOT)
> -      exec_res= ev->apply_event(rli);
> -#ifndef DBUG_OFF
>      /*
> -      This only prints information to the debug trace.
> -
> -      TODO: Print an informational message to the error log?
> +      Format_description_log_event should not be deleted because it will be
> +      used to read info about the relay log's format; it will be deleted when
> +      the SQL thread does not need it, i.e. when this thread terminates.
>      */
> -    static const char *const explain[] = {
> -      // EVENT_SKIP_NOT,
> -      "not skipped",
> -      // EVENT_SKIP_IGNORE,
> -      "skipped because event should be ignored",
> -      // EVENT_SKIP_COUNT
> -      "skipped because event skip counter was non-zero"
> -    };
> -    DBUG_PRINT("info", ("OPTION_BEGIN: %d; IN_STMT: %d",
> -                        thd->options & OPTION_BEGIN ? 1 : 0,
> -                        rli->get_flag(Relay_log_info::IN_STMT)));
> -    DBUG_PRINT("skip_event", ("%s event was %s",
> -                              ev->get_type_str(), explain[reason]));
> -#endif
> -
> -    DBUG_PRINT("info", ("apply_event error = %d", exec_res));
> -    if (exec_res == 0)
> +    if (ev->get_type_code() != FORMAT_DESCRIPTION_EVENT)
>      {
> -      int error= ev->update_pos(rli);
> -      char buf[22];
> -      DBUG_PRINT("info", ("update_pos error = %d", error));
> -      DBUG_PRINT("info", ("group %s %s",
> -                          llstr(rli->group_relay_log_pos, buf),
> -                          rli->group_relay_log_name));
> -      DBUG_PRINT("info", ("event %s %s",
> -                          llstr(rli->event_relay_log_pos, buf),
> -                          rli->event_relay_log_name));
> -      /*
> -        The update should not fail, so print an error message and
> -        return an error code.
> -
> -        TODO: Replace this with a decent error message when merged
> -        with BUG#24954 (which adds several new error message).
> -      */
> -      if (error)
> -      {
> -        rli->report(ERROR_LEVEL, ER_UNKNOWN_ERROR,
> -                    "It was not possible to update the positions"
> -                    " of the relay log information: the slave may"
> -                    " be in an inconsistent state."
> -                    " Stopped in %s position %s",
> -                    rli->group_relay_log_name,
> -                    llstr(rli->group_relay_log_pos, buf));
> -        DBUG_RETURN(1);
> -      }
> +      DBUG_PRINT("info", ("Deleting the event after it has been executed"));
> +      delete ev;
>      }
>  
>      /*
> -       Format_description_log_event should not be deleted because it will be
> -       used to read info about the relay log's format; it will be deleted when
> -       the SQL thread does not need it, i.e. when this thread terminates.
> +      update_log_pos failed: this should not happen, so we don't
> +      retry.
>      */
> -    if (type_code != FORMAT_DESCRIPTION_EVENT)
> -    {
> -      DBUG_PRINT("info", ("Deleting the event after it has been executed"));
> -      delete ev;
> -    }
> +    if (exec_res == 2)
> +      DBUG_RETURN(-1);
> +
>      if (slave_trans_retries)
>      {
>        int temp_err;
> @@ -1992,7 +2084,7 @@ or slave's MySQL code. If you want to ch
>  relay log, you will be able to know their names by issuing 'SHOW SLAVE STATUS' \
>  on this slave.\
>  ");
> -  DBUG_RETURN(1);
> +  DBUG_RETURN(-1);
>  }
>   

If you add a prototype (declaration) for a new function early in the 
file, and add the new function definition *last* in the file, you will 
have significantly smaller and more readable diffs.

> @@ -3051,7 +3143,7 @@ static int queue_old_event(Master_info *
>    any >=5.0.0 format.
>  */
>  
> -int queue_event(Master_info* mi,const char* buf, ulong event_len)
> +static int queue_event(Master_info* mi,const char* buf, ulong event_len)
>  {
>    int error= 0;
>    ulong inc_pos;
> @@ -3936,5 +4028,9 @@ bool rpl_master_has_bug(Relay_log_info *
>  template class I_List_iterator<i_string>;
>  template class I_List_iterator<i_string_pair>;
>  #endif
> +
> +/**
> +  @} (end of group Replication)
> +*/
>  
>  #endif /* HAVE_REPLICATION */
> diff -Nrup a/sql/slave.h b/sql/slave.h
> --- a/sql/slave.h	2007-09-10 13:16:00 +02:00
> +++ b/sql/slave.h	2007-12-10 10:42:43 +01:00
> @@ -187,6 +187,8 @@ int purge_relay_logs(Relay_log_info* rli
>  void set_slave_thread_options(THD* thd);
>  void set_slave_thread_default_charset(THD *thd, Relay_log_info const *rli);
>  void rotate_relay_log(Master_info* mi);
> +int apply_event_and_update_pos(Log_event* ev, THD* thd, Relay_log_info* rli,
> +                               bool skip);
>  
>  pthread_handler_t handle_slave_io(void *arg);
>  pthread_handler_t handle_slave_sql(void *arg);
> diff -Nrup a/sql/sql_binlog.cc b/sql/sql_binlog.cc
> --- a/sql/sql_binlog.cc	2007-11-08 08:54:08 +01:00
> +++ b/sql/sql_binlog.cc	2007-12-10 10:42:43 +01:00
> @@ -17,15 +17,14 @@
>  #include "rpl_rli.h"
>  #include "base64.h"
>  
> -/*
> +/**
>    Execute a BINLOG statement
>  
> -  TODO: This currently assumes a MySQL 5.x binlog.
> -  When we'll have binlog with a different format, to execute the
> -  BINLOG command properly the server will need to know which format
> -  the BINLOG command's event is in.  mysqlbinlog should then send
> -  the Format_description_log_event of the binlog it reads and the
> -  server thread should cache this format into
> +  To execute the BINLOG command properly the server needs to know
> +  which format the BINLOG command's event is in.  Therefore, the first
> +  BINLOG statement seen must be a base64 encoding of the
> +  Format_description_log_event, as outputted by mysqlbinlog.  This
> +  Format_description_log_event is cached in
>    rli->description_event_for_exec.
>  */
>  
> @@ -54,11 +53,24 @@ void mysql_client_binlog_statement(THD* 
>    /*
>      Allocation
>    */
> +
> +  /*
> +    If we do not have a Format_description_event, we create a dummy
> +    one here.  In this case, the first event we read must be a
> +    Format_description_event.
> +  */
> +  my_bool have_fd_event= TRUE;
>    if (!thd->rli_fake)
> +  {
>      thd->rli_fake= new Relay_log_info;
> -
> -  const Format_description_log_event *desc=
> -    new Format_description_log_event(4);
> +    have_fd_event= FALSE;
> +  }
> +  if (thd->rli_fake &&
> !thd->rli_fake->relay_log.description_event_for_exec)
> +  {
> +    thd->rli_fake->relay_log.description_event_for_exec=
> +      new Format_description_log_event(4);
> +    have_fd_event= FALSE;
> +  }
>  
>    const char *error= 0;
>    char *buf= (char *) my_malloc(decoded_len, MYF(MY_WME));
> @@ -67,7 +79,9 @@ void mysql_client_binlog_statement(THD* 
>    /*
>      Out of memory check
>    */
> -  if (!(thd->rli_fake && desc && buf))
> +  if (!(thd->rli_fake &&
> +        thd->rli_fake->relay_log.description_event_for_exec &&
> +        buf))
>    {
>      my_error(ER_OUTOFMEMORY, MYF(0), 1);  /* needed 1 bytes */
>      goto end;
> @@ -138,7 +152,28 @@ void mysql_client_binlog_statement(THD* 
>          goto end;
>        }
>  
> -      ev= Log_event::read_log_event(bufptr, event_len, &error, desc);
> +      /*
> +        If we have not seen any Format_description_event, then we must
> +        see one; it is the only statement that can be read in base64
> +        without a prior Format_description_event.
> +      */
> +      if (!have_fd_event)
> +      {
> +        if (bufptr[EVENT_TYPE_OFFSET] == FORMAT_DESCRIPTION_EVENT)
>   

Note regarding our discussion above: here you are treating the value 
from the buffer as an int and not as a Log_event_type, which IMHO you 
should do.

> +          have_fd_event= TRUE;
> +        else
> +        {
> +          my_error(ER_NO_FORMAT_DESCRIPTION_EVENT_BEFORE_BINLOG_STATEMENT,
> +                   MYF(0),
> +                   Log_event::get_type_str(
> +                     (Log_event_type)bufptr[EVENT_TYPE_OFFSET]));
> +          goto end;
> +        }
> +      }
> +
> +      ev= Log_event::read_log_event(bufptr, event_len, &error,
> +                                    thd->rli_fake->relay_log.
> +                                      description_event_for_exec);
>  
>        DBUG_PRINT("info",("binlog base64 err=%s", error));
>        if (!ev)
> @@ -174,11 +209,10 @@ void mysql_client_binlog_statement(THD* 
>          Neither do we have to update the log positions, since that is
>          not used at all: the rli_fake instance is used only for error
>          reporting.
> -       */
> +      */
>  #if !defined(MYSQL_CLIENT) && defined(HAVE_REPLICATION)
> -      if (IF_DBUG(int err= ) ev->apply_event(thd->rli_fake))
> +      if (apply_event_and_update_pos(ev, thd, thd->rli_fake, FALSE))
>        {
> -        DBUG_PRINT("info", ("apply_event() returned: %d", err));
>          /*
>            TODO: Maybe a better error message since the BINLOG statement
>            now contains several events.
> @@ -188,7 +222,14 @@ void mysql_client_binlog_statement(THD* 
>        }
>  #endif
>  
> -      delete ev;
> +      /*
> +        Format_description_log_event should not be deleted because it
> +        will be used to read info about the relay log's format; it
> +        will be deleted when the SQL thread does not need it,
> +        i.e. when this thread terminates.
> +      */
> +      if (ev->get_type_code() != FORMAT_DESCRIPTION_EVENT)
> +        delete ev;
>        ev= 0;
>      }
>    }
> @@ -207,7 +248,6 @@ end:
>    */
>    thd->net.no_send_ok= nsok;
>  
> -  delete desc;
>    my_free(buf, MYF(MY_ALLOW_ZERO_PTR));
>    DBUG_VOID_RETURN;
>  }
>
>   


-- 
Mats Kindahl
Lead Software Developer
Replication Team
MySQL AB, www.mysql.com


Thread
bk commit into 5.1 tree (sven:1.2651) BUG#32407Sven Sandberg10 Dec
  • Re: bk commit into 5.1 tree (sven:1.2651) BUG#32407Mats Kindahl13 Dec
    • Re: bk commit into 5.1 tree (sven:1.2651) BUG#32407Sven Sandberg13 Dec
      • Re: bk commit into 5.1 tree (sven:1.2651) BUG#32407Mats Kindahl13 Dec