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

[Sven Sandberg]

Hi Zhenxing,

Sorry for my late reply, I somehow missed to click send...

Thank you for spotting this one more place to change
4->EVENT_TYPE_OFFSET, I will fix that too.

I had to do more changes which I will commit later today, so please
review again!

/Sven

He Zhenxing wrote:
> 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