WL#3259 (RBR with more columns on slave than master):

Incorporating changes from review.
Fixing one bug that surfaced.


mysql-test/extra/rpl_tests/rpl_row_tabledefs.test:
  Adding tests that UPDATE and DELETE does not generate an error.
mysql-test/r/rpl_row_tabledefs_2myisam.result:
  Result change.
mysql-test/r/rpl_row_tabledefs_3innodb.result:
  Result change.
mysql-test/t/disabled.def:
  Enabling rpl_sp_effects (even though it gives a result mismatch currently).
sql/field.cc:
  Using constant to denote undefined last null byte.
sql/field.h:
  Using constant to denote undefined last null byte.
  Adding documentation.
sql/log_event.cc:
  Not generating error for non-NULL no-DEFAULT columns when updating or deleting row.
  Better documentation and comments.
sql/rpl_utility.cc:
  Moving documentation to header file.
sql/rpl_utility.h:
  Documenting class and members.

5 files changed, 179 insertions, 53 deletions

diff --git a/sql/field.cc b/sql/field.cc
index 340f33f1e01..479a562aaac 100644
--- a/sql/field.cc
+++ b/sql/field.cc
@@ -1261,7 +1261,10 @@ my_size_t
 Field::do_last_null_byte() const
 {
   DBUG_ASSERT(null_ptr == NULL || (byte*) null_ptr >= table->record[0]);
-  return null_ptr ? (byte*) null_ptr - table->record[0] + 1 : 0;
+  if (null_ptr)
+    return (byte*) null_ptr - table->record[0] + 1;
+  else
+    return LAST_NULL_BYTE_UNDEF;
 }
 
 
@@ -8196,7 +8199,10 @@ Field_bit::do_last_null_byte() const
   else
     result= bit_ptr;
 
-  return result ? (byte*) result - table->record[0] + 1 : 0;
+  if (result)
+    return (byte*) result - table->record[0] + 1;
+  else
+    return LAST_NULL_BYTE_UNDEF;
 }
 
 Field *Field_bit::new_key_field(MEM_ROOT *root,
diff --git a/sql/field.h b/sql/field.h
index 51b624b799f..26a9cd0b465 100644
--- a/sql/field.h
+++ b/sql/field.h
@@ -208,10 +208,24 @@ public:
   inline bool maybe_null(void) { return null_ptr != 0 || table->maybe_null; }
   inline bool real_maybe_null(void) { return null_ptr != 0; }
 
+  enum {
+    LAST_NULL_BYTE_UNDEF= 0
+  };
+
   /*
-    Return a pointer to the last byte of the null bytes where the
-    field conceptually is placed.  In the case that the field does not
-    use any bits of the null bytes, a null pointer is returned.
+    Find the position of the last null byte for the field.
+
+    SYNOPSIS
+      last_null_byte()
+
+    DESCRIPTION
+      Return a pointer to the last byte of the null bytes where the
+      field conceptually is placed.
+
+    RETURN VALUE
+      The position of the last null byte relative to the beginning of
+      the record. If the field does not use any bits of the null
+      bytes, the value 0 (LAST_NULL_BYTE_UNDEF) is returned.
    */
   my_size_t last_null_byte() const {
     my_size_t bytes= do_last_null_byte();
@@ -384,6 +398,17 @@ public:
   friend class Item_func_group_concat;
 
 private:
+  /*
+    Primitive for implementing last_null_byte().
+
+    SYNOPSIS
+      do_last_null_byte()
+
+    DESCRIPTION
+      Primitive for the implementation of the last_null_byte()
+      function. This represents the inheritance interface and can be
+      overridden by subclasses.
+   */
   virtual my_size_t do_last_null_byte() const;
 };
 
diff --git a/sql/log_event.cc b/sql/log_event.cc
index ceacc1eade7..ebfaa4bfff3 100644
--- a/sql/log_event.cc
+++ b/sql/log_event.cc
@@ -5304,7 +5304,7 @@ int Rows_log_event::do_add_row_data(byte *const row_data,
     row_end Pointer to variable that will hold the value of the
             one-after-end position for the row
     master_reclength
-            Pointer to variable that will hold the length of the
+            Pointer to variable that will be set to the length of the
             record on the master side
     rw_set  Pointer to bitmap that holds either the read_set or the
             write_set of the table
@@ -5317,13 +5317,22 @@ int Rows_log_event::do_add_row_data(byte *const row_data,
 
       At most 'colcnt' columns are read: if the table is larger than
       that, the remaining fields are not filled in.
+
+  RETURN VALUE
+
+      Error code, or zero if no error. The following error codes can
+      be returned:
+
+      ER_NO_DEFAULT_FOR_FIELD
+        Returned if one of the fields existing on the slave but not on
+        the master does not have a default value (and isn't nullable)
  */
 static int
 unpack_row(RELAY_LOG_INFO *rli,
            TABLE *table, uint const colcnt, byte *record,
            char const *row, MY_BITMAP const *cols,
            char const **row_end, ulong *master_reclength,
-           MY_BITMAP* const rw_set)
+           MY_BITMAP* const rw_set, Log_event_type const event_type)
 {
   DBUG_ASSERT(record && row);
   my_ptrdiff_t const offset= record - (byte*) table->record[0];
@@ -5334,10 +5343,20 @@ unpack_row(RELAY_LOG_INFO *rli,
     Field **fptr= &table->field[colcnt-1];
     do
       master_null_bytes= (*fptr)->last_null_byte();
-    while (master_null_bytes == 0 && fptr-- > table->field);
+    while (master_null_bytes == Field::LAST_NULL_BYTE_UNDEF &&
+           fptr-- > table->field);
+
+    /*
+      If master_null_bytes is LAST_NULL_BYTE_UNDEF (0) at this time,
+      there were no nullable fields nor BIT fields at all in the
+      columns that are common to the master and the slave. In that
+      case, there is only one null byte holding the X bit.
 
-    if (master_null_bytes == 0)
-      master_null_bytes= table->s->null_bytes;
+      OBSERVE! There might still be nullable columns following the
+      common columns, so table->s->null_bytes might be greater than 1.
+     */
+    if (master_null_bytes == Field::LAST_NULL_BYTE_UNDEF)
+      master_null_bytes= 1;
   }
 
   DBUG_ASSERT(master_null_bytes <= table->s->null_bytes);
@@ -5348,31 +5367,29 @@ unpack_row(RELAY_LOG_INFO *rli,
 
   Field **const begin_ptr = table->field;
   Field **field_ptr;
+  char const *ptr= row + master_null_bytes;
+  Field **const end_ptr= begin_ptr + colcnt;
+  for (field_ptr= begin_ptr ; field_ptr < end_ptr ; ++field_ptr)
   {
-    char const *ptr= row + master_null_bytes;
-    Field **const end_ptr= begin_ptr + colcnt;
-    for (field_ptr= begin_ptr ; field_ptr < end_ptr ; ++field_ptr)
-    {
-      Field *const f= *field_ptr;
-
-      if (bitmap_is_set(cols, field_ptr -  begin_ptr))
-      {
-        ptr= f->unpack(f->ptr + offset, ptr);
-        /* Field...::unpack() cannot return 0 */
-        DBUG_ASSERT(ptr != NULL);
-      }
-      else
-        bitmap_clear_bit(rw_set, field_ptr - begin_ptr);
-    }
+    Field *const f= *field_ptr;
 
-    *row_end = ptr;
-    if (master_reclength)
+    if (bitmap_is_set(cols, field_ptr -  begin_ptr))
     {
-      if (*field_ptr)
-        *master_reclength = (*field_ptr)->ptr - (char*) table->record[0];
-      else
-        *master_reclength = table->s->reclength;
+      ptr= f->unpack(f->ptr + offset, ptr);
+      /* Field...::unpack() cannot return 0 */
+      DBUG_ASSERT(ptr != NULL);
     }
+    else
+      bitmap_clear_bit(rw_set, field_ptr - begin_ptr);
+  }
+
+  *row_end = ptr;
+  if (master_reclength)
+  {
+    if (*field_ptr)
+      *master_reclength = (*field_ptr)->ptr - (char*) table->record[0];
+    else
+      *master_reclength = table->s->reclength;
   }
 
   /*
@@ -5381,16 +5398,26 @@ unpack_row(RELAY_LOG_INFO *rli,
     it was not there already. We iterate over all remaining columns,
     even if there were an error, to get as many error messages as
     possible.  We are still able to return a pointer to the next row,
-    so wedo that.
+    so redo that.
+
+    This generation of error messages is only relevant when inserting
+    new rows.
    */
   for ( ; *field_ptr ; ++field_ptr)
   {
-    if ((*field_ptr)->flags & (NOT_NULL_FLAG | NO_DEFAULT_VALUE_FLAG))
+    uint32 const mask= NOT_NULL_FLAG | NO_DEFAULT_VALUE_FLAG;
+
+    DBUG_PRINT("debug", ("flags = 0x%x, mask = 0x%x, flags & mask = 0x%x",
+                         (*field_ptr)->flags, mask,
+                         (*field_ptr)->flags & mask));
+
+    if (event_type == WRITE_ROWS_EVENT &&
+        ((*field_ptr)->flags & mask) == mask)
     {
-      slave_print_msg(ERROR_LEVEL, rli, ER_NO_DEFAULT_FOR_FIELD, 
+      slave_print_msg(ERROR_LEVEL, rli, ER_NO_DEFAULT_FOR_FIELD,
                       "Field `%s` of table `%s`.`%s` "
                       "has no default value and cannot be NULL",
-                      (*field_ptr)->field_name, table->s->db.str, 
+                      (*field_ptr)->field_name, table->s->db.str,
                       table->s->table_name.str);
       error = ER_NO_DEFAULT_FOR_FIELD;
     }
@@ -5562,8 +5589,8 @@ int Rows_log_event::exec_event(st_relay_log_info *rli)
     {
       char const *row_end= NULL;
       if ((error= do_prepare_row(thd, rli, table, row_start, &row_end)))
-        break; // We should to the after-row operation even in the
-               // case of error
+        break; // We should perform the after-row operation even in
+               // the case of error
       
       DBUG_ASSERT(row_end != NULL); // cannot happen
       DBUG_ASSERT(row_end <= (const char*)m_rows_end);
@@ -6224,7 +6251,7 @@ int Write_rows_log_event::do_prepare_row(THD *thd, RELAY_LOG_INFO *rli,
   error= unpack_row(rli,
                     table, m_width, table->record[0],
                     row_start, &m_cols, row_end, &m_master_reclength,
-                    table->write_set);
+                    table->write_set, WRITE_ROWS_EVENT);
   bitmap_copy(table->read_set, table->write_set);
   return error;
 }
@@ -6285,10 +6312,17 @@ copy_extra_record_fields(TABLE *table,
                          my_size_t master_reclength,
                          my_ptrdiff_t master_fields)
 {
-  DBUG_PRINT("info", ("Copying to %p from field %d at offset %u to field %d at offset %u", 
-                      table->record[0], 
+  DBUG_PRINT("info", ("Copying to %p "
+                      "from field %d at offset %u "
+                      "to field %d at offset %u",
+                      table->record[0],
                       master_fields, master_reclength,
                       table->s->fields, table->s->reclength));
+  /*
+    Copying the extra fields of the slave that does not exist on
+    master into record[0] (which are basically the default values).
+  */
+  DBUG_ASSERT(master_reclength <= table->s->reclength);
   if (master_reclength < table->s->reclength)
     bmove_align(table->record[0] + master_reclength,
                 table->record[1] + master_reclength,
@@ -6771,7 +6805,7 @@ int Delete_rows_log_event::do_prepare_row(THD *thd, RELAY_LOG_INFO *rli,
   error= unpack_row(rli,
                     table, m_width, table->record[0], 
                     row_start, &m_cols, row_end, &m_master_reclength,
-                    table->read_set);
+                    table->read_set, DELETE_ROWS_EVENT);
   /*
     If we will access rows using the random access method, m_key will
     be set to NULL, so we do not need to make a key copy in that case.
@@ -6914,13 +6948,13 @@ int Update_rows_log_event::do_prepare_row(THD *thd, RELAY_LOG_INFO *rli,
   error= unpack_row(rli,
                     table, m_width, table->record[0],
                     row_start, &m_cols, row_end, &m_master_reclength,
-                    table->read_set);
+                    table->read_set, UPDATE_ROWS_EVENT);
   row_start = *row_end;
   /* m_after_image is the after image for the update */
   error= unpack_row(rli,
                     table, m_width, m_after_image,
                     row_start, &m_cols, row_end, &m_master_reclength,
-                    table->write_set);
+                    table->write_set, UPDATE_ROWS_EVENT);
 
   /*
     If we will access rows using the random access method, m_key will
diff --git a/sql/rpl_utility.cc b/sql/rpl_utility.cc
index 5405d022223..c80b6dc3f69 100644
--- a/sql/rpl_utility.cc
+++ b/sql/rpl_utility.cc
@@ -107,11 +107,6 @@ field_length_from_packed(enum_field_types const field_type,
 /*
   Is the definition compatible with a table?
 
-  Compare the definition with a table to see if it is compatible with
-  it.  A table definition is compatible with a table if
-  - the columns types of the table definition is a (not necessarily
-    proper) prefix of the column type of the table, or
-  - the other way around
 */
 int
 table_def::compatible_with(RELAY_LOG_INFO *rli, TABLE *table)
diff --git a/sql/rpl_utility.h b/sql/rpl_utility.h
index 0ac3c10eec6..df0b0cd2ee1 100644
--- a/sql/rpl_utility.h
+++ b/sql/rpl_utility.h
@@ -32,29 +32,95 @@ field_length_from_packed(enum_field_types const field_type,
 
   RESPONSIBILITIES
 
-  - Extract table definition data from the table map event
+  - Extract and decode table definition data from the table map event
   - Check if table definition in table map is compatible with table
     definition on slave
+
+  DESCRIPTION
+
+    Currently, the only field type data available is an array of the
+    type operators that are present in the table map event.
+
+  TODO
+
+    Add type operands to this structure to allow detection of
+    difference between, e.g., BIT(5) and BIT(10).
  */
 
 class table_def
 {
 public:
+  /*
+    Convenience declaration of the type of the field type data in a
+    table map event.
+  */
   typedef unsigned char field_type;
 
-  table_def(field_type *t, my_size_t s)
-    : m_type(t), m_size(s)
+  /*
+    Constructor.
+
+    SYNOPSIS
+      table_def()
+      types Array of types
+      size  Number of elements in array 'types'
+   */
+  table_def(field_type *types, my_size_t size)
+    : m_type(types), m_size(size)
   {
   }
 
+  /*
+    Return the number of fields there is type data for.
+
+    SYNOPSIS
+      size()
+
+    RETURN VALUE
+      The number of fields that there is type data for.
+   */
   my_size_t size() const { return m_size; }
+
+  /*
+    Return a representation of the type data for one field.
+
+    SYNOPSIS
+      type()
+      i   Field index to return data for
+
+    RETURN VALUE
+
+      Will return a representation of the type data for field
+      'i'. Currently, only the type identifier is returned.
+   */
   field_type type(my_ptrdiff_t i) const { return m_type[i]; }
 
+  /*
+    Decide if the table definition is compatible with a table.
+
+    SYNOPSIS
+      compatible_with()
+      rli   Pointer to relay log info
+      table Pointer to table to compare with.
+
+    DESCRIPTION
+
+      Compare the definition with a table to see if it is compatible
+      with it.  A table definition is compatible with a table if:
+
+      - the columns types of the table definition is a (not
+        necessarily proper) prefix of the column type of the table, or
+
+      - the other way around
+
+    RETURN VALUE
+      1  if the table definition is not compatible with 'table'
+      0  if the table definition is compatible with 'table'
+  */
   int compatible_with(RELAY_LOG_INFO *rli, TABLE *table) const;
 
 private:
-  my_size_t m_size;
-  field_type *m_type;
+  my_size_t m_size;           // Number of elements in the types array
+  field_type *m_type;                     // Array of type descriptors
 };
 
 #endif /* RPL_UTILITY_H */