Mats, hello.
Could not find your patch in commits@; pasting the content of the mail
from the bug page.
The patch looks safe but I have questions.
From: Mats Kindahl <mats@stripped>
Date: August 12 2008 3:37pm
Subject: bzr commit into mysql-5.1 branch (mats:2669) Bug#37466
List-Archive: http://lists.mysql.com/commits/51416
X-Bug: 37466
Message-Id: <20080812133757.BF4F85BF30@mats-laptop>
#At file:///home/bzr/b37466-mysql-5.1-bugteam/
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.
As I see, the problem makes sense only when we consider new -> old
replication withing an assumption that the old header if fully
inherited by a new format and new attributes only are appended to the
old header.
The least it should be noted.
If we are going to do this change we shall confess to ourselves that
really would start supporting this replication direction.
Personally I think it should not be our target in any perceivable
future.
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.
modified:
Notice there is nothing about Start_log_event_v3 event but it changes
further in the patch.
sql/log_event.cc
per-file messages:
sql/log_event.cc
Replacing the read of the common header length from the format description
log events with the constant value LOG_EVENT_MINIMAL_HEADER_LEN.
=== modified file 'sql/log_event.cc'
--- a/sql/log_event.cc 2008-05-12 17:50:53 +0000
+++ b/sql/log_event.cc 2008-08-12 13:37:51 +0000
@@ -2696,7 +2696,13 @@ Start_log_event_v3::Start_log_event_v3(c
*description_event)
:Log_event(buf, description_event)
{
- buf+= description_event->common_header_len;
+ /*
+ The size of the common header for the Rotate_log_event,
+ Start_log_event_v3, and the Format_description_log_event has to be
+ fixed at LOG_EVENT_MINIMAL_HEADER_LEN and should not be read from
+ the description log event supplied.
+ */
+ buf+= LOG_EVENT_MINIMAL_HEADER_LEN;
binlog_version= uint2korr(buf+ST_BINLOG_VER_OFFSET);
memcpy(server_version, buf+ST_SERVER_VER_OFFSET,
ST_SERVER_VER_LEN);
I think this part should be accompanied with adding a line to the
Log_event::Log_event(const char* buf,
const Format_description_log_event*
description_event)
if ((buf[EVENT_TYPE_OFFSET] == FORMAT_DESCRIPTION_EVENT) ||
+ buf[EVENT_TYPE_OFFSET] == START_EVENT_V3 ||
(buf[EVENT_TYPE_OFFSET] == ROTATE_EVENT))
to enforce START_EVENT_V3 will never ever have anything after
LOG_EVENT_MINIMAL_HEADER_LEN position.
@@ -2947,7 +2953,13 @@ Format_description_log_event(const char*
description_event)
:Start_log_event_v3(buf, description_event), event_type_permutation(0)
{
- DBUG_ENTER("Format_description_log_event::Format_description_log_event(char*,...)");
+ DBUG_ENTER("Format_description_log_event(char*,...)");
+ /*
+ The size of the common header for the Rotate_log_event,
+ Start_log_event_v3, and the Format_description_log_event has to be
+ fixed at LOG_EVENT_MINIMAL_HEADER_LEN and should not be read from
+ the description log event supplied.
+ */
Maybe it's better to refer to the first appearance of these comments
to avoid repeating? Like /* see comments in Start_log_event_v3::Start_log_event_v3() */
buf+= LOG_EVENT_MINIMAL_HEADER_LEN;
if ((common_header_len=buf[ST_COMMON_HEADER_LEN_OFFSET]) < OLD_HEADER_LEN)
DBUG_VOID_RETURN; /* sanity check */
@@ -4052,10 +4064,16 @@ Rotate_log_event::Rotate_log_event(const
:Log_event(buf, description_event) ,new_log_ident(0), flags(DUP_NAME)
{
DBUG_ENTER("Rotate_log_event::Rotate_log_event(char*,...)");
- // The caller will ensure that event_len is what we have at EVENT_LEN_OFFSET
- uint8 header_size= description_event->common_header_len;
+ /*
+ The size of the common header for the Rotate_log_event,
+ Start_log_event_v3, and the Format_description_log_event has to be
+ fixed at LOG_EVENT_MINIMAL_HEADER_LEN and should not be read from
+ the description log event supplied.
+ */
same as above
+ uint8 header_size= LOG_EVENT_MINIMAL_HEADER_LEN;
uint8 post_header_len= description_event->post_header_len[ROTATE_EVENT-1];
uint ident_offset;
+ // The caller will ensure that event_len is what we have at EVENT_LEN_OFFSET
if (event_len < header_size)
DBUG_VOID_RETURN;
buf += header_size;
regards,
Andrei
