Re: bk commit into 5.1 tree (sven:1.2650) BUG#32407

[He Zhenxing]

Hi Sven,

Just one suggestion, see below

On 2007-11-28 Wed 19:13 +0100，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-11-28 19:13:44+01:00, sven@riska.(none) +3 -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.
>   
>   I made FD able to print themselves, and made mysqlbinlog tell FD events
>   to print themselves.
> 
>   client/mysqlbinlog.cc@stripped, 2007-11-28 19:13:43+01:00, sven@riska.(none) +77 -11
>     
>      - mysqlbinlog uses this and prints FD in base64, even if the user
>        specified a --start-position that is after the FD.  The reason is
>        that it is not safe to read other base64-printed events unless the
>        FD is known.
>     
>      - Added a --no-base64-output flag to mysqlbinlog, which suppresses
>        this printing of extra out-of-range base64 FD.  This can be used
>        with binlogs that are known not to contain any base64 events.  If
>        mysqlbinlog later finds an event that must be printed in base64, it
>        generates an error.
>     
>     While I was here, I fixed some small things in the code:
>     
>      - replace hard-coded 4 by EVENT_TYPE_OFFSET in 3 places
>     
>      - 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.
> 
>   sql/log_event.cc@stripped, 2007-11-28 19:13:43+01:00, sven@riska.(none) +9 -1
>     Added base64 printing to Format_description_log_events.
>     
>     While I was here, I fixed two small issues:
>     
>      - Log_event::get_type_str() did not return anything for old row events.
>     
>      - DBUG_ENTER was not matched by DBUG_VOID_RETURN in one place.
> 
>   sql/log_event.h@stripped, 2007-11-28 19:13:43+01:00, sven@riska.(none) +5 -2
>     Better comments.
> 
> 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-11-28 19:13:43 +01:00
> @@ -63,7 +63,7 @@ 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;
> +static bool opt_base64_output= 0, opt_no_base64_output= 0;
>  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 +96,14 @@ 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;
> +/*
> +  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.
> +*/
> +static bool printed_fd_event= FALSE;
>  
>  static int dump_local_log_entries(PRINT_EVENT_INFO *print_event_info,
>                                    const char* logname);
> @@ -632,11 +639,19 @@ Create_file event for file_id: %u\n",exv
>        break;
>      }
>      case FORMAT_DESCRIPTION_EVENT:
> -      delete glob_description_event;
> -      glob_description_event= (Format_description_log_event*) ev;
> +      /*
> +        glob_description_event == ev when this function is called from
> +        check_header.
> +      */
> +      if (glob_description_event != ev)
> +      {
> +        delete glob_description_event;
> +        glob_description_event= (Format_description_log_event*) ev;
> +      }
>        print_event_info->common_header_len=
>          glob_description_event->common_header_len;
>        ev->print(result_file, print_event_info);
> +      printed_fd_event= TRUE;
>        ev->temp_buf= 0; // as the event ref is zeroed
>        /*
>          We don't want this event to be deleted now, so let's hide it (I
> @@ -678,6 +693,26 @@ 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:
> +      if (!printed_fd_event)
> +      {
> +        if (opt_no_base64_output)
> +          die("Error: --no-base64-output specified, but binlog contains a %s "
> +              "event which cannot be printed without base64.",
> +              ev->get_type_str());
> +        else
> +          die("Error: 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.",
> +              ev->get_type_str());
> +      }
> +      /* FALL THROUGH */
>      default:
>        ev->print(result_file, print_event_info);
>      }
> @@ -713,6 +748,11 @@ static struct my_option my_long_options[
>     "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},
> +  {"no-base64-output", OPT_BASE64_OUTPUT,
> +   "Do not print any binlog entries using base64 encoding. "
> +   "If there are any row-based events, an error message will be printed.",
> +   (uchar**) &opt_no_base64_output, (uchar**) &opt_no_base64_output, 0,
> +   GET_BOOL, NO_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
> @@ -1305,8 +1345,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.
> +*/
>  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];
> @@ -1371,8 +1424,9 @@ Could not read entry at offset %lu : Err
>      {
>        DBUG_PRINT("info",("buf[4]=%d", buf[4]));

Since you are changing hard coded numbers into macros, I think the upper
line could be changed too, it would make the code and the debug message
more informative, though the value printed in the debug message will
still be a number.

>        /* 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 +1438,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*) 
> @@ -1399,9 +1454,11 @@ at offset %lu ; this could be a log form
>          }
>          delete *description_event;
>          *description_event= new_description_event;
> +        if (!opt_no_base64_output)
> +          process_event(print_event_info, *description_event, tmp_pos);
>          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 +1487,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 +1497,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 +1519,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 */
> @@ -1550,6 +1607,15 @@ int main(int argc, char** argv)
>    if (!argc)
>    {
>      usage();
> +    free_defaults(defaults_argv);
> +    exit(1);
> +  }
> +
> +  if (opt_base64_output && opt_no_base64_output)
> +  {
> +    usage();
> +    printf("--base64-output and --no-base64-output cannot be used "
> +           "together.\n");
>      free_defaults(defaults_argv);
>      exit(1);
>    }
> 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-11-28 19:13:43 +01:00
> @@ -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";
> @@ -2576,6 +2579,11 @@ void Start_log_event_v3::print(FILE* fil
>  #else
>      my_b_printf(&cache,"ROLLBACK%s\n", print_event_info->delimiter);
>  #endif
> +    if (temp_buf)
> +    {
> +      my_b_printf(&cache, "BINLOG '\n");
> +      print_base64(&cache, print_event_info, FALSE);
> +    }
>    }
>    DBUG_VOID_RETURN;
>  }
> @@ -5022,7 +5030,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-11-28 19:13:43 +01:00
> @@ -810,6 +810,10 @@ public:
>    bool cache_stmt;
>  
>  #ifndef MYSQL_CLIENT
> +  /*
> +    this is ifndef'ed to avoid having to link mysqlbinlog against
> +    libpthread
> +  */
>    THD* thd;
>  
>    Log_event();
> @@ -852,9 +856,8 @@ public:
>    {
>      return thd ? thd->db : 0;
>    }
> -#else
> +#else /* ifndef MYSQL_CLIENT */
>    Log_event() : temp_buf(0) {}
> -    /* avoid having to link mysqlbinlog against libpthread */
>    static Log_event* read_log_event(IO_CACHE* file,
>                                     const Format_description_log_event
>                                     *description_event);
> 
> -- 
> MySQL Code Commits Mailing List
> For list archives: http://lists.mysql.com/commits
> To unsubscribe:    http://lists.mysql.com/commits?unsub=1
-- 
Turbolinux China
A: A12 Jianguomenwai Avenue Chaoyang District Beijing PRC
Z: 100022
E: hezx@stripped
T: 86-010-65054020-318
F: 86-010-65054017