Re: bk commit into 5.1 tree (mats:1.2652) BUG#31502

[Andrei Elkin]

Mats, hej.


We discussed some issues on #rep regarding to the 2nd patch.

We are facing -

  slave needs to handle pre-metadata master's extra columns
  with sometimes stopping gracefully (mostly solved by the 1st patch)

  as a side-effect of that solution that relies on table_map's column
  type info we got a problem of distinguishing between enum, set from
  the very string.

My point was that replacement in storing real_type() instead of
type().
With wl#3228 we actually have access to the real_type when it's
different from type.

To avoid the mentioned side-effect
it'd be enough to refine logics of the first patch new function
check_and_set_length(). When it executes

   switch (type) ...

   case MYSQL_TYPE_STRING:

to test additionally if there is a metadata info and if it is the
first byte must be the value of the real_type.
if the real_type is in MYSQL_TYPE_{ENUM,SET} then the length is
set basing on their rules not the MYSQL_TYPE_STRING rule
(can be implemented with introducing a new get_real_type(col), see
further).
With doing such refinement of logics of check_and_set_length() we
won't be taxed with an unnecessary replication stop\footnote{
because of the first arg of the if yielded true
+    if (table->field[col]->real_type() != type(col) ||
+        (m_field_metadata[col] == 0 &&
+           table->field[col]->real_type() != type(col)))
}
when old metadata-aware master updates a set or enum columns.

Answering one of your questions:

<mats> andrei, The real type is not stored for all fields, so I think
	      we should correct the coltype array to hold the right data.

We actually have all necessary info for the slave in the table_map.

As to an example 

<mats> andrei, There is also another scenario that could cause a
	      segfault...
<mats> master> CREATE TABLE t1 (a INT, b ENUM('RED','GREEN','BLUE'));
<mats> slave> ALTER TABLE t1 MODIFY b CHAR(10));
<mats> master> INSERT INTO t1 VALUES(1,'RED'), (2, 'GREEN');
<mats> *see slave crash*

we need indeed a similar to the 2nd patch refinement of logics in
table_def::compatible_with().
Something like this:


+    if (table->field[col]->type() != type(col) ||
+        (m_field_metadata[col] != 0 &&
+           table->field[col]->real_type() != get_real_type(col)))

where a new function get_real_type(col) returns
table->field[col]->real_type() if 
metadata does not carry it otherwise the master's real_type for the
col.

All in all, we should not have the side-effect plus your crash example
ceases.

I have some more comments as well but let us come to the conclusion on
this principal part.

regards,

Andrei



> Below is the list of changes that have just been committed into a local
> 5.1 repository of mats. When mats 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, 2008-02-22 15:25:56+01:00, mats@stripped +8 -0
>   BUG#31502 (5.1.20 -> 5.1.22 Slave crashes if it gets an event w/ data 
>              for non-exist column):
>   
>   Complimentary patch to fix the fact that type() instead of real_type()
>   was sent in the table map event. This cause a crash when using ENUM
>   extra columns since type() is MYSQL_TYPE_STRING and real_type() is
>   MYSQL_TYPE_ENUM. The packed format for MYSQL_TYPE_STRING is (len,
>   byte, ...) while the pack format for MYSQL_TYPE_ENUM is just the
>   integer value as a single byte (which will then be interpreted as the
>   length of a string by the slave).
>
>   sql/field.cc@stripped, 2008-02-22 15:25:43+01:00, mats@stripped +8
> -11
>     Removing debug printouts that are not needed any more.
>     In Field_string::unpack(), only 'min(field_length,length)' bytes
>     were read from the log, although 'length' bytes should be read,
>     since this is what is stored in the log. Changing the code so
>     that the function advances the point 'length' bytes, but just
>     copy 'min(field_length,lenght)' bytes from the buffer. This way
>     there is no risk of buffer overflow when length > field_length.
>
>   sql/field.h@stripped, 2008-02-22 15:25:44+01:00, mats@stripped +8
> -2
>     Removing gratuitous virtual for save_field_metadata(): the function
>     is never overridden. Adding debug printout to save_field_metadata().
>     
>
>   sql/log_event.cc@stripped, 2008-02-22 15:25:45+01:00, mats@stripped
> +1 -1
>     Storing real_type() instead of type() in the coltype array.
>
>   sql/log_event.h@stripped, 2008-02-22 15:25:45+01:00, mats@stripped
> +2 -1
>     Removing gratuitous virtual on save_field_metadata(): the
>     function is never overridden. Adding end of all rows to
>     unpack_row() to allow for checking if reading out of bounds.
>
>   sql/log_event_old.h@stripped, 2008-02-22 15:25:45+01:00,
> mats@stripped +1 -0
>     Adding end of all rows to unpack_row() to allow for checking
>     if reading out of bounds.
>
>   sql/rpl_record.cc@stripped, 2008-02-22 15:25:46+01:00, mats@stripped
> +20 -13
>     Adding end of all rows to unpack_row() to allow for checking
>     if reading out of bounds. Returning error if reading out of
>     bounds. Some changes to debug printout to get good information
>     for debugging.
>
>   sql/rpl_record.h@stripped, 2008-02-22 15:25:46+01:00, mats@stripped
> +1 -0
>     Adding parameter for end of all rows to allow check for
>     reading out of bounds data.
>
>   sql/rpl_utility.cc@stripped, 2008-02-22 15:25:46+01:00,
> mats@stripped +22 -6
>     Not printing an error for field types that require metadata
>     when we have a length value, i.e., when there were metadata
>     available for the field.
>
> diff -Nrup a/sql/field.cc b/sql/field.cc
> --- a/sql/field.cc	2007-11-21 21:09:21 +01:00
> +++ b/sql/field.cc	2008-02-22 15:25:43 +01:00
> @@ -4036,39 +4036,35 @@ uchar *
>  Field_real::pack(uchar *to, const uchar *from,
>                   uint max_length, bool low_byte_first)
>  {
> -  DBUG_ENTER("Field_real::pack");
>    DBUG_ASSERT(max_length >= pack_length());
> -  DBUG_PRINT("debug", ("pack_length(): %u", pack_length()));
>  #ifdef WORDS_BIGENDIAN
>    if (low_byte_first != table->s->db_low_byte_first)
>    {
>      const uchar *dptr= from + pack_length();
>      while (dptr-- > from)
>        *to++ = *dptr;
> -    DBUG_RETURN(to);
> +    return to;
>    }
>    else
>  #endif
> -    DBUG_RETURN(Field::pack(to, from, max_length, low_byte_first));
> +    return Field::pack(to, from, max_length, low_byte_first);
>  }
>  
>  const uchar *
>  Field_real::unpack(uchar *to, const uchar *from,
>                     uint param_data, bool low_byte_first)
>  {
> -  DBUG_ENTER("Field_real::unpack");
> -  DBUG_PRINT("debug", ("pack_length(): %u", pack_length()));
>  #ifdef WORDS_BIGENDIAN
>    if (low_byte_first != table->s->db_low_byte_first)
>    {
>      const uchar *dptr= from + pack_length();
>      while (dptr-- > from)
>        *to++ = *dptr;
> -    DBUG_RETURN(from + pack_length());
> +    return from + pack_length();
>    }
>    else
>  #endif
> -    DBUG_RETURN(Field::unpack(to, from, param_data, low_byte_first));
> +    return Field::unpack(to, from, param_data, low_byte_first);
>  }
>  
>  /****************************************************************************
> @@ -6675,7 +6671,7 @@ Field_string::unpack(uchar *to,
>                       bool low_byte_first __attribute__((unused)))
>  {
>    uint from_length=
> -    param_data ? min(param_data & 0x00ff, field_length) : field_length;
> +    param_data ? (param_data & 0x00ff) : field_length;
>    uint length;
>  
>    if (from_length > 255)
> @@ -6686,9 +6682,10 @@ Field_string::unpack(uchar *to,
>    else
>      length= (uint) *from++;
>  
> -  memcpy(to, from, length);
> +  memcpy(to, from, min(length, field_length));
>    // Pad the string with the pad character of the fields charset
> -  bfill(to + length, field_length - length, field_charset->pad_char);
> +  if (field_length > length)
> +    bfill(to + length, field_length - length, field_charset->pad_char);
>    return from+length;
>  }
>  
> diff -Nrup a/sql/field.h b/sql/field.h
> --- a/sql/field.h	2007-10-30 08:19:22 +01:00
> +++ b/sql/field.h	2008-02-22 15:25:44 +01:00
> @@ -167,8 +167,14 @@ public:
>      field_length + 1, field_length, and pack_length_no_ptr() respectfully.
>    */
>    virtual uint row_pack_length() { return 0; }
> -  virtual int save_field_metadata(uchar *first_byte)
> -  { return do_save_field_metadata(first_byte); }
> +  int save_field_metadata(uchar *first_byte)
> +  {
> +    int result= do_save_field_metadata(first_byte);
> +    DBUG_PRINT("info", ("field: %s; type: %3d; count: %2d, data: %02x %02x",
> +                        field_name, real_type(), result,
> +                        first_byte[0], first_byte[1]));
> +    return result;
> +  }
>  
>    /*
>      data_length() return the "real size" of the data in memory.
> diff -Nrup a/sql/log_event.cc b/sql/log_event.cc
> --- a/sql/log_event.cc	2007-12-13 21:27:23 +01:00
> +++ b/sql/log_event.cc	2008-02-22 15:25:45 +01:00
> @@ -6852,7 +6852,7 @@ Table_map_log_event::Table_map_log_event
>    {
>      m_coltype= reinterpret_cast<uchar*>(m_memory);
>      for (unsigned int i= 0 ; i < m_table->s->fields ; ++i)
> -      m_coltype[i]= m_table->field[i]->type();
> +      m_coltype[i]= m_table->field[i]->real_type();
>    }
>  
>    /*
> diff -Nrup a/sql/log_event.h b/sql/log_event.h
> --- a/sql/log_event.h	2007-12-13 21:27:23 +01:00
> +++ b/sql/log_event.h	2008-02-22 15:25:45 +01:00
> @@ -2874,7 +2874,7 @@ public:
>  
>    virtual int get_data_size() { return m_data_size; } 
>  #ifndef MYSQL_CLIENT
> -  virtual int save_field_metadata();
> +  int save_field_metadata();
>    virtual bool write_data_header(IO_CACHE *file);
>    virtual bool write_data_body(IO_CACHE *file);
>    virtual const char *get_db() { return m_dbnam; }
> @@ -3097,6 +3097,7 @@ protected:
>      DBUG_ASSERT(m_table);
>      ASSERT_OR_RETURN_ERROR(m_curr_row < m_rows_end, HA_ERR_CORRUPT_EVENT);
>      int const result= ::unpack_row(rli, m_table, m_width, m_curr_row, &m_cols,
> +                                   m_rows_end,
>                                     &m_curr_row_end, &m_master_reclength);
>      ASSERT_OR_RETURN_ERROR(m_curr_row_end <= m_rows_end, HA_ERR_CORRUPT_EVENT);
>      return result;
> diff -Nrup a/sql/log_event_old.h b/sql/log_event_old.h
> --- a/sql/log_event_old.h	2007-11-20 19:49:34 +01:00
> +++ b/sql/log_event_old.h	2008-02-22 15:25:45 +01:00
> @@ -212,6 +212,7 @@ protected:
>      DBUG_ASSERT(m_table);
>      ASSERT_OR_RETURN_ERROR(m_curr_row < m_rows_end, HA_ERR_CORRUPT_EVENT);
>      int const result= ::unpack_row(rli, m_table, m_width, m_curr_row, &m_cols,
> +                                   m_rows_end,
>                                     &m_curr_row_end, &m_master_reclength);
>      ASSERT_OR_RETURN_ERROR(m_curr_row_end <= m_rows_end, HA_ERR_CORRUPT_EVENT);
>      return result;
> diff -Nrup a/sql/rpl_record.cc b/sql/rpl_record.cc
> --- a/sql/rpl_record.cc	2007-12-13 21:27:24 +01:00
> +++ b/sql/rpl_record.cc	2008-02-22 15:25:46 +01:00
> @@ -77,8 +77,10 @@ pack_row(TABLE *table, MY_BITMAP const* 
>    unsigned int null_mask= 1U;
>    for ( ; (field= *p_field) ; p_field++)
>    {
> -    DBUG_PRINT("debug", ("null_mask=%d; null_ptr=%p; row_data=%p;
> null_byte_count=%d",
> -                         null_mask, null_ptr, row_data, null_byte_count));
> +    DBUG_PRINT("debug", ("field: %s; type: %d",
> +                         field->field_name, field->real_type()));
> +    DBUG_PRINT("debug", ("null_mask=%d; null_ptr=%p; null_byte_count=%d",
> +                         null_mask, null_ptr, null_byte_count));
>      if (bitmap_is_set(cols, p_field - table->field))
>      {
>        my_ptrdiff_t offset;
> @@ -104,11 +106,10 @@ pack_row(TABLE *table, MY_BITMAP const* 
>  #endif
>          pack_ptr= field->pack(pack_ptr, field->ptr + offset,
>                                field->max_data_length(), TRUE);
> -        DBUG_PRINT("debug", ("field: %s; pack_ptr: 0x%lx;"
> -                             " pack_ptr':0x%lx; bytes: %d",
> -                             field->field_name, (ulong) old_pack_ptr,
> -                             (ulong) pack_ptr,
> +        DBUG_PRINT("debug", ("pack_ptr: 0x%lx; pack_ptr':0x%lx; bytes: %d",
> +                             (ulong) old_pack_ptr, (ulong) pack_ptr,
>                               (int) (pack_ptr - old_pack_ptr)));
> +        DBUG_DUMP("row_data", old_pack_ptr, pack_ptr - old_pack_ptr);
>        }
>  
>        null_mask <<= 1;
> @@ -186,6 +187,7 @@ int
>  unpack_row(Relay_log_info const *rli,
>             TABLE *table, uint const colcnt,
>             uchar const *const row_data, MY_BITMAP const *cols,
> +           uchar const *const data_end,
>             uchar const **const row_end, ulong *const master_reclength)
>  {
>    DBUG_ENTER("unpack_row");
> @@ -247,11 +249,12 @@ unpack_row(Relay_log_info const *rli,
>          uchar const *const old_pack_ptr= pack_ptr;
>  #endif
>          pack_ptr= f->unpack(f->ptr, pack_ptr, metadata, TRUE);
> -	DBUG_PRINT("debug", ("field: %s; type: %d; metadata: 0x%x;"
> -                             " pack_ptr: 0x%lx; pack_ptr': 0x%lx; bytes: %d",
> -                             f->field_name, f->type(), metadata,
> +	DBUG_PRINT("debug", ("field: %s; type: %d; metadata: 0x%x",
> +                             f->field_name, f->type(), metadata));
> +	DBUG_PRINT("debug", ("pack_ptr: 0x%lx; pack_ptr': 0x%lx; bytes: %d",
>                               (ulong) old_pack_ptr, (ulong) pack_ptr,
>                               (int) (pack_ptr - old_pack_ptr)));
> +        DBUG_DUMP("row_data", old_pack_ptr, pack_ptr - old_pack_ptr);
>        }
>  
>        null_mask <<= 1;
> @@ -262,8 +265,8 @@ unpack_row(Relay_log_info const *rli,
>    /*
>      throw away master's extra fields
>    */
> -  DBUG_PRINT("info", ("i: %u; tabledef->size: %lu; cols->n_bits: %u",
> -                      i, tabledef->size(), cols->n_bits));
> +  DBUG_PRINT("info", ("tabledef->size: %lu; cols->n_bits: %u",
> +                      tabledef->size(), cols->n_bits));
>    uint max_cols= min(tabledef->size(), cols->n_bits);
>    for (; i < max_cols; i++)
>    {
> @@ -282,9 +285,13 @@ unpack_row(Relay_log_info const *rli,
>          if ((error= tabledef->calc_field_size(i, (uchar *) pack_ptr, &length,
> rli)))
>            DBUG_RETURN(error);
>          DBUG_PRINT("info",
> -                   ("type: %d; pack_ptr: 0x%lu; pack_ptr': 0x%lu",
> -                    tabledef->type(i),
> +                   ("type: %d; metadata: 0x%04x; bytes: %d",
> +                    tabledef->type(i), tabledef->field_metadata(i), length));
> +        DBUG_PRINT("info",
> +                   ("pack_ptr: 0x%lu; pack_ptr': 0x%lu",
>                      (ulong) pack_ptr, (ulong) pack_ptr + length));
> +        DBUG_DUMP("row_data", pack_ptr, min(length, 128));
> +        ASSERT_OR_RETURN_ERROR(pack_ptr + length <= data_end,
> HA_ERR_CORRUPT_EVENT);
>          pack_ptr += length;
>        }
>        null_mask <<= 1;
> diff -Nrup a/sql/rpl_record.h b/sql/rpl_record.h
> --- a/sql/rpl_record.h	2007-09-10 13:15:59 +02:00
> +++ b/sql/rpl_record.h	2008-02-22 15:25:46 +01:00
> @@ -27,6 +27,7 @@ size_t pack_row(TABLE* table, MY_BITMAP 
>  int unpack_row(Relay_log_info const *rli,
>                 TABLE *table, uint const colcnt,
>                 uchar const *const row_data, MY_BITMAP const *cols,
> +               uchar const *const data_end,
>                 uchar const **const row_end, ulong *const master_reclength);
>  
>  // Fill table's record[0] with default values.
> diff -Nrup a/sql/rpl_utility.cc b/sql/rpl_utility.cc
> --- a/sql/rpl_utility.cc	2007-12-13 21:27:24 +01:00
> +++ b/sql/rpl_utility.cc	2008-02-22 15:25:46 +01:00
> @@ -77,9 +77,14 @@ check_and_set_length(enum_field_types fi
>    case MYSQL_TYPE_VARCHAR:
>    case MYSQL_TYPE_BIT:
>    case MYSQL_TYPE_DECIMAL:                      // ???
> -    log->report(ERROR_LEVEL, ER_SLAVE_MISSING_METADATA,
> -                ER(ER_SLAVE_MISSING_METDATA), field_type);
> -    error= HA_ERR_SLAVE_MISSING_METADATA;
> +    if (length)
> +      *out_length = length;
> +    else
> +    {
> +      log->report(ERROR_LEVEL, ER_SLAVE_MISSING_METADATA,
> +                ER(ER_SLAVE_MISSING_METADATA), field_type);
> +      error= HA_ERR_SLAVE_MISSING_METADATA;
> +    }
>      break;
>    }
>    return error;
> @@ -192,7 +197,7 @@ table_def::calc_field_size(uint col, uch
>    case MYSQL_TYPE_BLOB:
>    case MYSQL_TYPE_GEOMETRY:
>    {
> -#if 1
> +#if 0
>      /*
>        BUG#29549: 
>        This is currently broken for NDB, which is using big-endian
> @@ -232,7 +237,7 @@ table_def::calc_field_size(uint col, uch
>      break;
>    }
>    default:
> -    length= -1;
> +    length= 0;
>    }
>  
>    int const error= check_and_set_length(type(col), length, log, len_arg);
> @@ -258,7 +263,18 @@ table_def::compatible_with(Relay_log_inf
>  
>    for (uint col= 0 ; col < cols_to_check ; ++col)
>    {
> -    if (table->field[col]->type() != type(col))
> +    /*
> +      Here we check if the type we received is different from the real
> +      type of the column. If it is, there is a problem and an error is
> +      reported.
> +
> +      To be able to replication from old servers not having metadata,
> +      we check the type (instead of the real type) since this is what
> +      was stored instead of the real type.
> +     */
> +    if (table->field[col]->real_type() != type(col) ||
> +        (m_field_metadata[col] == 0 &&
> +           table->field[col]->real_type() != type(col)))
>      {
>        DBUG_ASSERT(col < size() && col < tsh->fields);
>        DBUG_ASSERT(tsh->db.str && tsh->table_name.str);
>
> -- 
> MySQL Code Commits Mailing List
> For list archives: http://lists.mysql.com/commits
> To unsubscribe:    http://lists.mysql.com/commits?unsub=1