Bug #37466 Adding field to Log_event header not backward compatible
Submitted: 17 Jun 2008 22:48 Modified: 17 Oct 2008 14:50
Reporter: Justin Tolmer Email Updates:
Status: In progress Impact on me:
None 
Category:MySQL Server: Replication Severity:S3 (Non-critical)
Version:5.1.23-rc OS:Any
Assigned to: Assigned Account CPU Architecture:Any

[17 Jun 2008 22:48] Justin Tolmer
Description:
Much support has been added to sql/log_event.cc so that new fields can be added to the event header without needing to change the file format. Format_description_log_event plays a critical role because it conveys information on the size of the header of events which follow it. This support should allow a server with the current header format containing 19 bytes to replicate from a server with a newer format which has a header with a larger size. The intended functioning of this is outlined in detail in worklog #3610.

Unfortunately, there are 2 bugs which prevent this from working correctly.

Log_event::Log_event(const char* buf,
                     const Format_description_log_event* description_event)

contains this code snippet:

  if ((buf[EVENT_TYPE_OFFSET] == FORMAT_DESCRIPTION_EVENT) ||
      (buf[EVENT_TYPE_OFFSET] == ROTATE_EVENT))
  {
    /*
      These events always have a header which stops here (i.e. their
      header is FROZEN).
    */

meaning that their headers will forever by 19 bytes. However both 

Start_log_event_v3::Start_log_event_v3(const char* buf,
                                       const Format_description_log_event
                                       *description_event)

and

Rotate_log_event::Rotate_log_event(const char* buf, uint event_len,
                                   const Format_description_log_event*
                                   description_event)

contain code that does:

  buf+= description_event->common_header_len;

which is not correct.

How to repeat:
Reproducing the error is involved because it requires adding a new field to the Log_event header and then setting up replication using the new header format.

Suggested fix:
In the 2 contructors mentioned, change the code to

  buf+= LOG_EVENT_MINIMAL_HEADER_LEN;
[18 Jun 2008 10:28] Susanne Ebrecht
Many thanks for writing a bug report. We will analyse this.
[24 Jun 2008 20:47] Justin Tolmer
Found another instance of this problem in the code in fake_rotate_event. This function generates a Rotate_Log_event, which is supposed to have a header frozen at 19 bytes and so the function should be using LOG_EVENT_MINIMAL_HEADER_LEN instead of LOG_EVENT_HEADER_LEN.
[12 Aug 2008 13:38] Bugs System
A patch for this bug has been committed. After review, it may
be pushed to the relevant source trees for release in the next
version. You can access the patch from:

  http://lists.mysql.com/commits/51416

2669 Mats Kindahl	2008-08-12
      Bug #37466  	Adding field to Log_event header not backward compatible
      
      In order to be able to extend the common header, the length of the
      common header is given in the format description log event. This will
      allow any server reading an event to find the location of the post-
      header and data even if the common header is longer than expected by
      just adding the size of the common header given in the format
      description log event.
      
      However, in order to be read correctly, the common header of both
      the format description log event and rotate event has to be fixed
      at LOG_EVENT_MINIMAL_HEADER_LEN, which is 19. The value of LOG_
      EVENT_MINIMAL_HEADER_LEN may not change.
      
      If the common header is extended with new data, the header will grow
      beyond LOG_EVENT_MINIMAL_HEADER_LEN. However, the code for the format
      description and rotate event reads the size from the format description
      log event instead of just using the value LOG_EVENT_MINIMAL_HEADER_LEN,
      this subsequently will cause them to read bad data, leading to a crash or
      data corruption of some other form.
      
      This patch replace the read of the common header length from the format
      description log event with the constant LOG_EVENT_MINIMAL_HEADER_LEN in
      order to allow the common header to be extended with new fields.
[12 Aug 2008 13:39] Mats Kindahl
Patch committed towards 5.1 even though it will be merged with 6.0 and not pushed to 5.1.
[6 Oct 2008 19:58] Justin Tolmer
The patch doesn't cover the change needed to fake_rotate_event in file sql/sql_repl.cc.
[7 Oct 2008 10:34] Andrei Elkin
Justin, hello.

According to fake_rotate_event() definition the fake rotate
is created having LOG_EVENT_MINIMAL_HEADER_LEN bytes in the common header and
then no changes are needed in this part.

Could you please explain what is the change you meant for the fake rotate?

Regards,

Andrei
[7 Oct 2008 11:16] Bugs System
A patch for this bug has been committed. After review, it may
be pushed to the relevant source trees for release in the next
version. You can access the patch from:

  http://lists.mysql.com/commits/51416
[7 Oct 2008 16:23] Justin Tolmer
Andrei,

In the mysql-5.1.24-rc source, and the copy of the 6.0 sources I have, fake_rotate_event has this code:

  DBUG_ENTER("fake_rotate_event");
  char header[LOG_EVENT_HEADER_LEN], buf[ROTATE_HEADER_LEN+100];

...

  packet->append(header, sizeof(header));

Thus, the header being written for the event is too large since rotate events are supposed to be frozen at size LOG_EVENT_MINIMAL_HEADER_LEN.

I continually get errors when trying to view the source on launchpad.net and so don't know if the head revisions of the files still contain the error or have perhaps subsequently been fixed.