Re: bk commit into 5.1 tree (mats:1.2571) BUG#29549

[Mats Kindahl]

Andrei Elkin wrote:
> Mats, hello.
>
> Thanks for considering my questions.
>
>   
>> Hi Andrei!
>>
>> Summary of comments below:
>>
>>    * I'll add Field_blob to the cset comment.
>>    * I'll reuse Field::pack() and Field::unpack() where appropriate
>>     

I made a slight rewrite of the Field::unpack() function (to move a 
variable definition to just before it was used, and then call 
Field::unpack() instead inside Field_new_decimal::unpack().

>>    * I will change the code in pack_row() to take the maximum field
>>      size into consideration instead of assuming a maximum.
>>     

I moved the max_data_length() function upwards from Field_blob and made 
it virtual.

>>    * I'll add an assertion to the locations where the WORDS_BIGENDIAN
>>      removes code.
>>     

There is no assertion to add here. The reason for removing the code is 
not anything about assuming that table->s->db_low_byte_first is true, it 
is about the fact that for little-endian machines, native format and 
low_byte_first formats are identical.

Just my few cents,
Mats Kindahl

>>
>> Could you verify that this suffices?
>>     
>
> Sure. don't be shy to ping me as soon as it will be committed.
>
> cheers,
>
> Andrei
>
>
>   
>> Just my few cents,
>> Mats Kindahl
>>
>> Andrei Elkin wrote:
>>     
>>> Mats, hej.
>>>
>>> Hopefully i have my final set of questions which do not undermine
>>> the value of your work.
>>>
>>>
>>>   
>>>       
>>>> 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, 2007-10-05 18:16:11+02:00,
> mats@stripped +20 -0
>>>>   BUG#29549 (Endians: test failures on Solaris):
>>>>     Refactoring code to add parameter to pack() and unpack()
>>>> functions with
>>>>   purpose of indicating if data should be packed in little-endian or
>>>>   native order. Using new functions to always pack data for binary log
>>>>   in little-endian order. The purpose of this refactoring is to allow
>>>>   proper implementation of endian-agnostic pack() and unpack()
> functions.
>>>>     Eliminating several versions of virtual pack() and unpack()
>>>> functions
>>>>   in favor for one single virtual function which is overridden in
>>>>   subclasses.
>>>>     Implementing pack() and unpack() functions for some field types
>>>> that
>>>>   packed data in native format regardless of the value of the
>>>>   st_table_share::db_low_byte_first flag.
>>>>     The field types that were packed in native format regardless
>>>> are:
>>>>   Field_real, Field_decimal, Field_tiny, Field_short, Field_medium,
>>>>   Field_long, and Field_longlong.
>>>>       
>>>>         
>>> Could you please either add Field_blob or refuse with an explanation
>>> to me. There is a hunk which makes me thinking Field_blob packing has been
>>> corrected too.
>>>   
>>>       
>> I'll add it.
>>
>> [snip]
>>
>>     
>>>>     Using pad_char for the field instead of spaces (0x20) when
>>>>     unpacking.
>>>>     
>>>>         
>>> You answered that the change is "obvious" and thereafter you were not
>>> going to expand the comments.
>>> After such answer I indeed stopped to find any other subttle reason. Thanks.
>>>
>>>   
>>>       
>> OK.
>>
>> [snip]
>>
>>     
>>>   
>>>       
>>>> -const uchar *Field_new_decimal::unpack(uchar* to, -
>>>> const uchar *from, -                                       uint
>>>> param_data)
>>>> -{
>>>> +const uchar *
>>>> +Field_new_decimal::unpack(uchar* to,
>>>> +                          const uchar *from,
>>>> +                          uint param_data,
>>>> +                          bool low_byte_first __attribute__((unused)))
>>>> +{
>>>> +  if (param_data == 0)
>>>> +  {
>>>> +    uint const length= pack_length();
>>>> +    memcpy(to, from, pack_length());
>>>> +    return from + length;
>>>> +  }
>>>> +
>>>>     
>>>>         
>>> The early return block corresponds to a branch of Field::unpack().
>>> To call the parent method is better instead of
>>> two maintenan two equivalent but separate pieces of code.
>>>   
>>>       
>> OK.
>>
>>     
>>>   
>>>       
>>>>    uint from_precision= (param_data & 0xff00) >> 8U;
>>>>    uint from_decimal= param_data & 0x00ff;
>>>>    uint length=pack_length();
>>>> @@ -3857,6 +3926,49 @@ void Field_longlong::sql_type(String &re
>>>>  }
>>>>   +/*
>>>> +  Floating-point numbers
>>>> + */
>>>> +
>>>> +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);
>>>> +  }
>>>> +  else
>>>> +#endif
>>>>     
>>>>         
>>> Obviously, there will be change in behaviour for packing in the native format
> on
>>> the big endian.
>>> There is a change for swapping whereas previously the two arg
>>> Field_real::pack method never did not do that.
>>>
>>> Might it to be an issue?
>>>   
>>>       
>> It was an issue in the previous code: replication for NDB failed on
>> big endian machines, that is why this code was added.
>>
>> [snip]
>>
>>     
>>>> +
>>>> +  /*
>>>> +    Store max length, which will occupy packlength bytes. If the max
>>>> +    length given is smaller than the actual length of the blob, we
>>>> +    just store the initial bytes of the blob.
>>>>     
>>>>         
>>>   Naturallly, a question pops up:
>>>   and what will happend with the rest
>>>   lenth - max_length bytes of the field?  Will it be ever packed
>>>   (replicated)?
>>>   
>>>       
>> No, they will be truncated. Actually, it seems to be a problem in the
>> old code in that the max length was given by a constant (UINT_MAX),
>> and not dependent on the max field size for the given field.
>>
>> I'm adding a fix for that one as well, but that requires a little more work.
>>
>>     
>>>   
>>>       
>>>> +  */
>>>> +  store_length(to, packlength, min(length, max_length),
> low_byte_first);
>>>> +
>>>> +  /*
>>>> +    Store the actual blob data, which will occupy 'length' bytes.
>>>> +   */
>>>> +  if (length > 0)
>>>>    {
>>>>      get_ptr((uchar**) &from);
>>>>      memcpy(to+packlength, from,length);
>>>>    }
>>>>    ptr=save;					// Restore org row pointer
>>>> -  return to+packlength+length;
>>>> +  DBUG_DUMP("packed", to, packlength + length);
>>>> +  DBUG_RETURN(to+packlength+length);
>>>>  }
>>>>   @@ -7644,28 +7756,26 @@ uchar *Field_blob::pack(uchar *to, const
>>>>      @param   to         Destination of the data
>>>>     @param   from       Source of the data
>>>> -   @param   param_data <not used>
>>>>      @return  New pointer into memory based on from + length of the
>>>> data
>>>>  */
>>>>  const uchar *Field_blob::unpack(uchar *to,
>>>> const uchar *from,
>>>> -                                uint param_data)
>>>> +                                uint param_data,
>>>> +                                bool low_byte_first)
>>>>  {
>>>> -  return unpack(to, from);
>>>> -}
>>>> -
>>>> -
>>>> -const uchar *Field_blob::unpack(uchar *to, const uchar *from)
>>>> -{
>>>> -  memcpy(to,from,packlength);
>>>> -  uint32 length=get_length(from);
>>>> -  from+=packlength;
>>>> -  if (length)
>>>> -    memcpy_fixed(to+packlength, &from, sizeof(from));
>>>> -  else
>>>> -    bzero(to+packlength,sizeof(from));
>>>> -  return from+length;
>>>> +  DBUG_ENTER("Field_blob::unpack");
>>>> +  DBUG_PRINT("enter", ("to: 0x%lx; from: 0x%lx;"
>>>> +                       " param_data: %u; low_byte_first: %d",
>>>> +                       to, from, param_data, low_byte_first));
>>>> +  uint const master_packlength=
>>>> +    param_data > 0 ? param_data & 0xFF : packlength;
>>>> +  uint32 const length= get_length(from, master_packlength,
> low_byte_first);
>>>> +  DBUG_DUMP("packed", from, length + master_packlength);
>>>> +  store(reinterpret_cast<const char*>(from) + master_packlength,
>>>> +        length, field_charset);
>>>> +  DBUG_DUMP("record", to, table->s->reclength);
>>>> +  DBUG_RETURN(from + master_packlength + length);
>>>>  }
>>>>   /* Keys for blobs are like keys on varchars */
>>>>     
>>>>         
>>> You need to update the comments in 4-args version of
>>> Field_blob::unpack (perhaps in some other as well, please check it
>>> too) continuing to say the method is replication-related.
>>>   
>>>       
>> The methods are not pure replication. They are used by filesort as well.
>>
>>     
>>>   
>>>       
>>>> @@ -7715,7 +7825,9 @@ int Field_blob::pack_cmp(const uchar *b,
>>>>   /* Create a packed key that will be used for storage from a MySQL
>>>> row */
>>>>  -uchar *Field_blob::pack_key(uchar *to, const uchar *from, uint
>>>> max_length)
>>>> +uchar *
>>>> +Field_blob::pack_key(uchar *to, const uchar *from, uint max_length,
>>>> +                     bool low_byte_first __attribute__((unused)))
>>>>  {
>>>>    uchar *save= ptr;
>>>>    ptr= (uchar*) from;
>>>> @@ -7760,8 +7872,9 @@ uchar *Field_blob::pack_key(uchar *to, c
>>>>      Pointer into 'from' past the last byte copied from packed key.
>>>>  */
>>>>  -const uchar *Field_blob::unpack_key(uchar *to, const uchar *from,
>>>> -                                    uint max_length)
>>>> +const uchar *
>>>> +Field_blob::unpack_key(uchar *to, const uchar *from, uint max_length,
>>>> +                       bool low_byte_first __attribute__((unused)))
>>>>  {
>>>>    /* get length of the blob key */
>>>>    uint32 length= *from++;
>>>> @@ -7784,8 +7897,9 @@ const uchar *Field_blob::unpack_key(ucha
>>>>   /* Create a packed key that will be used for storage from a MySQL
>>>> key */
>>>>  -uchar *Field_blob::pack_key_from_key_image(uchar *to, const uchar
>>>> *from,
>>>> -                                           uint max_length)
>>>> +uchar *
>>>> +Field_blob::pack_key_from_key_image(uchar *to, const uchar *from, uint
> max_length,
>>>> +                                    bool low_byte_first
> __attribute__((unused)))
>>>>  {
>>>>    uint length=uint2korr(from);
>>>>    if (length > max_length)
>>>> @@ -8672,9 +8786,11 @@ void Field_bit::sql_type(String &res) co
>>>>  }
>>>>   -uchar *Field_bit::pack(uchar *to, const uchar *from, uint
>>>> max_length)
>>>> +uchar *
>>>> +Field_bit::pack(uchar *to, const uchar *from, uint max_length,
>>>> +                bool low_byte_first __attribute__((unused)))
>>>>  {
>>>> -  DBUG_ASSERT(max_length);
>>>> +  DBUG_ASSERT(max_length > 0);
>>>>    uint length;
>>>>    if (bit_len > 0)
>>>>    {
>>>> @@ -8709,28 +8825,44 @@ uchar *Field_bit::pack(uchar *to, const
>>>> /**
>>>>     Unpack a bit field from row data.
>>>>  -   This method is used to unpack a bit field from a master whose
>>>> size +   This method is used to unpack a bit field from a master
>>>> whose size
>>>>     of the field is less than that of the slave.
>>>> -  
>>>> +
>>>>     @param   to         Destination of the data
>>>>     @param   from       Source of the data
>>>>     @param   param_data Bit length (upper) and length (lower) values
>>>>      @return  New pointer into memory based on from + length of the
>>>> data
>>>>  */
>>>> -const uchar *Field_bit::unpack(uchar *to,
>>>> -                               const uchar *from,
>>>> -                               uint param_data)
>>>> +const uchar *
>>>> +Field_bit::unpack(uchar *to, const uchar *from, uint param_data,
>>>> +                  bool low_byte_first __attribute__((unused)))
>>>>  {
>>>>    uint const from_len= (param_data >> 8U) & 0x00ff;
>>>>    uint const from_bit_len= param_data & 0x00ff;
>>>>    /*
>>>> -    If the master and slave have the same sizes, then use the old
>>>> -    unpack() method.
>>>> +    If the parameter data is zero (i.e., undefined), or if the master
>>>> +    and slave have the same sizes, then use the old unpack() method.
>>>>    */
>>>> -  if ((from_bit_len == bit_len) &&
>>>> -      (from_len == bytes_in_rec)) -    return(unpack(to, from));
>>>> +  if (param_data == 0 ||
>>>> +      (from_bit_len == bit_len) && (from_len == bytes_in_rec))
>>>> +  {
>>>> +    if (bit_len > 0)
>>>> +    {
>>>> +      /*
>>>> +        set_rec_bits is a macro, don't put the post-increment in the
>>>> +        argument since that might cause strange side-effects.
>>>> +
>>>> +        For the choice of the second argument, see the explanation for
>>>> +        Field_bit::pack().
>>>> +      */
>>>> +      set_rec_bits(*from, bit_ptr + (to - ptr), bit_ofs, bit_len);
>>>> +      from++;
>>>> +    }
>>>> +    memcpy(to, from, bytes_in_rec);
>>>> +    return from + bytes_in_rec;
>>>> +  }
>>>> +
>>>>    /*
>>>>      We are converting a smaller bit field to a larger one here.
>>>>      To do that, we first need to construct a raw value for the original
>>>> @@ -8755,25 +8887,6 @@ const uchar *Field_bit::unpack(uchar *to
>>>>    store(value, new_len, system_charset_info);
>>>>    my_afree(value);
>>>>    return from + len;
>>>> -}
>>>> -
>>>> -
>>>> -const uchar *Field_bit::unpack(uchar *to, const uchar *from)
>>>> -{
>>>> -  if (bit_len > 0)
>>>> -  {
>>>> -    /*
>>>> -      set_rec_bits is a macro, don't put the post-increment in the
>>>> -      argument since that might cause strange side-effects.
>>>> -
>>>> -      For the choice of the second argument, see the explanation for
>>>> -      Field_bit::pack().
>>>> -    */
>>>> -    set_rec_bits(*from, bit_ptr + (to - ptr), bit_ofs, bit_len);
>>>> -    from++;
>>>> -  }
>>>> -  memcpy(to, from, bytes_in_rec);
>>>> -  return from + bytes_in_rec;
>>>>  }
>>>>   diff -Nrup a/sql/field.h b/sql/field.h
>>>> --- a/sql/field.h	2007-08-04 11:08:07 +02:00
>>>> +++ b/sql/field.h	2007-10-05 18:16:00 +02:00
>>>> @@ -339,32 +339,45 @@ public:
>>>>      return str;
>>>>    }
>>>>    virtual bool send_binary(Protocol *protocol);
>>>> -  virtual uchar *pack(uchar *to, const uchar *from, uint
> max_length=~(uint) 0)
>>>> +
>>>> +  virtual uchar *pack(uchar *to, const uchar *from,
>>>> +                      uint max_length, bool low_byte_first);
>>>> +  /**
>>>> +     @overload Field::pack(uchar*, const uchar*, uint, bool)
>>>> +  */
>>>> +  uchar *pack(uchar *to, const uchar *from)
>>>>    {
>>>> -    uint32 length=pack_length();
>>>> -    memcpy(to,from,length);
>>>> -    return to+length;
>>>> +    DBUG_ENTER("Field::pack");
>>>> +    uchar *result= this->pack(to, from, UINT_MAX,
> table->s->db_low_byte_first);
>>>> +    DBUG_RETURN(result);
>>>>    }
>>>> -  virtual const uchar *unpack(uchar* to, const uchar *from, uint
> param_data);
>>>> -  virtual const uchar *unpack(uchar* to, const uchar *from)
>>>> +
>>>> +  virtual const uchar *unpack(uchar* to, const uchar *from,
>>>> +                              uint param_data, bool low_byte_first);
>>>> +  /**
>>>> +     @overload Field::unpack(uchar*, const uchar*, uint, bool)
>>>> +  */
>>>> +  const uchar *unpack(uchar* to, const uchar *from)
>>>>    {
>>>> -    uint length=pack_length();
>>>> -    memcpy(to,from,length);
>>>> -    return from+length;
>>>> +    DBUG_ENTER("Field::unpack");
>>>> +    const uchar *result= unpack(to, from, 0U,
> table->s->db_low_byte_first);
>>>> +    DBUG_RETURN(result);
>>>>    }
>>>>     
>>>>         
>>>   
>>>       
>>>> -  virtual uchar *pack_key(uchar* to, const uchar *from, uint
> max_length)
>>>> +
>>>> +  virtual uchar *pack_key(uchar* to, const uchar *from,
>>>> +                          uint max_length, bool low_byte_first)
>>>>    {
>>>> -    return pack(to,from,max_length);
>>>> +    return pack(to, from, max_length, low_byte_first);
>>>>    }
>>>>     
>>>>         
>>> Previously, pack_key was defined to work in native format.
>>> Why not   
>>>       
>>>> +    return pack(to, from, max_length, 0);
>>>>     
>>>>         
>>> and to leave intact the signature?
>>> The question is asked also because all overloaded definitions does not
>>> use the last new arg.
>>>
>>> Similar applies to pack_key_from_key_image.
>>>   
>>>       
>> For symmetry reasons, I have elected to use the same signature for all
>> the functions. The fact that they are not used currently does not
>> preclude that they will be used in the future. Since this patch
>> simplifies the pack()/unpack() functionality and base it on a single
>> function, then by symmetry, I should follow the same pattern for all
>> similar functions. This makes it easier to use the interface. The fact
>> that it uses pack() internally, I view as an implementation issue.
>>
>>     
>>>   
>>>       
>>>>    virtual uchar *pack_key_from_key_image(uchar* to, const uchar *from,
>>>> -					uint max_length)
>>>> +					uint max_length, bool low_byte_first)
>>>>    {
>>>> -    return pack(to,from,max_length);
>>>> +    return pack(to, from, max_length, low_byte_first);
>>>>    }
>>>>    virtual const uchar *unpack_key(uchar* to, const uchar *from,
>>>> -                                  uint max_length)
>>>> +                                  uint max_length, bool low_byte_first)
>>>>    {
>>>> -    return unpack(to,from);
>>>> +    return unpack(to, from, max_length, low_byte_first);
>>>>    }
>>>>    virtual uint packed_col_length(const uchar *to, uint length)
>>>>    { return length;}
>>>>     
>>>>         
>>>   
>>>       
>>>> @@ -556,6 +569,10 @@ public:
>>>>    int truncate(double *nr, double max_length);
>>>>    uint32 max_display_length() { return field_length; }
>>>>    uint size_of() const { return sizeof(*this); }
>>>> +  virtual const uchar *unpack(uchar* to, const uchar *from,
>>>> +                              uint param_data, bool low_byte_first);
>>>> +  virtual uchar *pack(uchar* to, const uchar *from,
>>>> +                      uint max_length, bool low_byte_first);
>>>>  };
>>>>       
>>>>         
>>>   
>>>       
>>>> @@ -584,6 +601,16 @@ public:
>>>>    void overflow(bool negative);
>>>>    bool zero_pack() const { return 0; }
>>>>    void sql_type(String &str) const;
>>>> +  virtual const uchar *unpack(uchar* to, const uchar *from,
>>>> +                              uint param_data, bool low_byte_first)
>>>> +  {
>>>> +    return Field::unpack(to, from, param_data, low_byte_first);
>>>> +  }
>>>> +  virtual uchar *pack(uchar* to, const uchar *from,
>>>> +                      uint max_length, bool low_byte_first)
>>>> +  {
>>>> +    return Field::pack(to, from, max_length, low_byte_first);
>>>> +  }
>>>>  };
>>>>      
>>>>         
>>> What's a point not to use automatic overloading?
>>>   
>>>       
>> Because there is a class in between that does not do the correct
>> thing. This class is not directly derived from Field.
>>
>>     
>>>   
>>>       
>>>>  @@ -629,7 +656,8 @@ public:
>>>>    uint size_of() const { return sizeof(*this); }    uint32
>>>> pack_length() const { return (uint32) bin_size; }
>>>>    uint is_equal(Create_field *new_field);
>>>> -  virtual const uchar *unpack(uchar* to, const uchar *from, uint
> param_data);
>>>> +  virtual const uchar *unpack(uchar* to, const uchar *from,
>>>> +                              uint param_data, bool low_byte_first);
>>>>  };
>>>>       
>>>>         
>>> The same as before.
>>>   
>>>       
>> ... and the same answer as before.
>>
>>     
>>>   
>>>       
>>>> @@ -660,6 +688,20 @@ public:
>>>>    uint32 pack_length() const { return 1; }
>>>>    void sql_type(String &str) const;
>>>>    uint32 max_display_length() { return 4; }
>>>> +
>>>> +  virtual uchar *pack(uchar* to, const uchar *from,
>>>> +                      uint max_length, bool low_byte_first)
>>>> +  {
>>>> +    *to= *from;
>>>> +    return to + 1;
>>>> +  }
>>>> +
>>>> +  virtual const uchar *unpack(uchar* to, const uchar *from,
>>>> +                              uint param_data, bool low_byte_first)
>>>> +  {
>>>> +    *to= *from;
>>>> +    return from + 1;
>>>> +  }
>>>>  };
>>>>   @@ -695,8 +737,47 @@ public:
>>>>    uint32 pack_length() const { return 2; }
>>>>    void sql_type(String &str) const;
>>>>    uint32 max_display_length() { return 6; }
>>>> -};
>>>>  +  virtual uchar *pack(uchar* to, const uchar *from,
>>>> +                      uint max_length, bool low_byte_first)
>>>> +  {
>>>> +    int16 val;
>>>> +#ifdef WORDS_BIGENDIAN
>>>> +    if (table->s->db_low_byte_first)
>>>> +      val = sint2korr(from);
>>>> +    else
>>>> +#endif
>>>> +      shortget(val, from);
>>>> +
>>>> +#ifdef WORDS_BIGENDIAN
>>>> +    if (low_byte_first)
>>>> +      int2store(to, val);
>>>> +    else
>>>> +#endif
>>>> +      shortstore(to, val);
>>>> +    return to + sizeof(val);
>>>> +  }
>>>> +
>>>> +  virtual const uchar *unpack(uchar* to, const uchar *from,
>>>> +                              uint param_data, bool low_byte_first)
>>>> +  {
>>>> +    int16 val;
>>>> +#ifdef WORDS_BIGENDIAN
>>>> +    if (low_byte_first)
>>>> +      val = sint2korr(from);
>>>> +    else
>>>> +#endif
>>>> +      shortget(val, from);
>>>> +
>>>> +#ifdef WORDS_BIGENDIAN
>>>> +    if (table->s->db_low_byte_first)
>>>> +      int2store(to, val);
>>>> +    else
>>>> +#endif
>>>> +      shortstore(to, val);
>>>> +    return from + sizeof(val);
>>>> +  }
>>>> +};
>>>>   class Field_medium :public Field_num {
>>>>  public:
>>>>     
>>>>         
>>>   
>>>       
>>>> @@ -725,6 +806,18 @@ public:
>>>>    uint32 pack_length() const { return 3; }
>>>>    void sql_type(String &str) const;
>>>>    uint32 max_display_length() { return 8; }
>>>> +
>>>> +  virtual uchar *pack(uchar* to, const uchar *from,
>>>> +                      uint max_length, bool low_byte_first)
>>>> +  {
>>>> +    return Field::pack(to, from, max_length, low_byte_first);
>>>> +  }
>>>> +
>>>> +  virtual const uchar *unpack(uchar* to, const uchar *from,
>>>> +                              uint param_data, bool low_byte_first)
>>>> +  {
>>>> +    return Field::unpack(to, from, param_data, low_byte_first);
>>>> +  }
>>>>  };
>>>>      
>>>>         
>>> the same as before.
>>>   
>>>       
>> ... and the same answer as before.
>>
>>     
>>>   
>>>       
>>>> @@ -760,6 +853,45 @@ public:
>>>>    uint32 pack_length() const { return 4; }
>>>>    void sql_type(String &str) const;
>>>>    uint32 max_display_length() { return MY_INT32_NUM_DECIMAL_DIGITS; }
>>>> +  virtual uchar *pack(uchar* to, const uchar *from,
>>>> +                      uint max_length, bool low_byte_first)
>>>> +  {
>>>> +    int32 val;
>>>> +#ifdef WORDS_BIGENDIAN
>>>> +    if (table->s->db_low_byte_first)
>>>> +      val = sint4korr(from);
>>>> +    else
>>>> +#endif
>>>> +      longget(val, from);
>>>> +
>>>> +#ifdef WORDS_BIGENDIAN
>>>> +    if (low_byte_first)
>>>> +      int4store(to, val);
>>>> +    else
>>>> +#endif
>>>> +      longstore(to, val);
>>>> +    return to + sizeof(val);
>>>> +  }
>>>>     
>>>>         
>>> Obviously, the modified since the last version of the patch definition
>>> assumes that on the little endian
>>>
>>>   table->s->db_low_byte_first is true
>>>
>>> Not to disagree I think we are better to be safe to assert that
>>> explicitly at all places where you oprimized out
>>> `if (table->s->db_low_byte_first)' check.
>>>   
>>>       
>> I can add assertion for that, yes. It is actually a good idea.
>>
>>     
>>>   
>>>       
>>>> +
>>>> +  virtual const uchar *unpack(uchar* to, const uchar *from,
>>>> +                              uint param_data, bool low_byte_first)
>>>> +  {
>>>> +    int32 val;
>>>> +#ifdef WORDS_BIGENDIAN
>>>> +    if (low_byte_first)
>>>> +      val = sint4korr(from);
>>>> +    else
>>>> +#endif
>>>> +      longget(val, from);
>>>> +
>>>> +#ifdef WORDS_BIGENDIAN
>>>> +    if (table->s->db_low_byte_first)
>>>> +      int4store(to, val);
>>>> +    else
>>>> +#endif
>>>> +      longstore(to, val);
>>>> +    return from + sizeof(val);
>>>> +  }
>>>>  };
>>>>   @@ -802,6 +934,45 @@ public:
>>>>    void sql_type(String &str) const;
>>>>    bool can_be_compared_as_longlong() const { return TRUE; }
>>>>    uint32 max_display_length() { return 20; }
>>>> +  virtual uchar *pack(uchar* to, const uchar *from,
>>>> +                      uint max_length, bool low_byte_first)
>>>> +  {
>>>> +    int64 val;
>>>> +#ifdef WORDS_BIGENDIAN
>>>> +    if (table->s->db_low_byte_first)
>>>> +      val = sint8korr(from);
>>>> +    else
>>>> +#endif
>>>> +      longlongget(val, from);
>>>> +
>>>> +#ifdef WORDS_BIGENDIAN
>>>> +    if (low_byte_first)
>>>> +      int8store(to, val);
>>>> +    else
>>>> +#endif
>>>> +      longlongstore(to, val);
>>>> +    return to + sizeof(val);
>>>> +  }
>>>> +
>>>> +  virtual const uchar *unpack(uchar* to, const uchar *from,
>>>> +                              uint param_data, bool low_byte_first)
>>>> +  {
>>>> +    int64 val;
>>>> +#ifdef WORDS_BIGENDIAN
>>>> +    if (low_byte_first)
>>>> +      val = sint8korr(from);
>>>> +    else
>>>> +#endif
>>>> +      longlongget(val, from);
>>>> +
>>>> +#ifdef WORDS_BIGENDIAN
>>>> +    if (table->s->db_low_byte_first)
>>>> +      int8store(to, val);
>>>> +    else
>>>> +#endif
>>>> +      longlongstore(to, val);
>>>> +    return from + sizeof(val);
>>>> +  }
>>>>  };
>>>>  #endif
>>>>  @@ -1176,9 +1347,10 @@ public:
>>>>    int cmp(const uchar *,const uchar *);
>>>>    void sort_string(uchar *buff,uint length);
>>>>    void sql_type(String &str) const;
>>>> -  uchar *pack(uchar *to, const uchar *from, uint max_length=~(uint) 0);
>>>> -  virtual const uchar *unpack(uchar* to, const uchar *from, uint
> param_data);
>>>> -  const uchar *unpack(uchar* to, const uchar *from);
>>>> +  virtual uchar *pack(uchar *to, const uchar *from,
>>>> +                      uint max_length, bool low_byte_first);
>>>> +  virtual const uchar *unpack(uchar* to, const uchar *from,
>>>> +                              uint param_data, bool low_byte_first);
>>>>    int pack_cmp(const uchar *a,const uchar *b,uint key_length,
>>>>                 my_bool insert_or_update);
>>>>    int pack_cmp(const uchar *b,uint key_length,my_bool
> insert_or_update);
>>>> @@ -1250,13 +1422,15 @@ public:
>>>>    uint get_key_image(uchar *buff,uint length, imagetype type);
>>>>    void set_key_image(const uchar *buff,uint length);
>>>>    void sql_type(String &str) const;
>>>> -  uchar *pack(uchar *to, const uchar *from, uint max_length=~(uint) 0);
>>>> -  uchar *pack_key(uchar *to, const uchar *from, uint max_length);
>>>> +  virtual uchar *pack(uchar *to, const uchar *from,
>>>> +                      uint max_length, bool low_byte_first);
>>>> +  uchar *pack_key(uchar *to, const uchar *from, uint max_length, bool
> low_byte_first);
>>>>    uchar *pack_key_from_key_image(uchar* to, const uchar *from,
>>>> -                                 uint max_length);
>>>> -  virtual const uchar *unpack(uchar* to, const uchar *from, uint
> param_data);
>>>> -  const uchar *unpack(uchar* to, const uchar *from);
>>>> -  const uchar *unpack_key(uchar* to, const uchar *from, uint
> max_length);
>>>> +                                 uint max_length, bool low_byte_first);
>>>> +  virtual const uchar *unpack(uchar* to, const uchar *from,
>>>> +                              uint param_data, bool low_byte_first);
>>>> +  const uchar *unpack_key(uchar* to, const uchar *from,
>>>> +                          uint max_length, bool low_byte_first);
>>>>    int pack_cmp(const uchar *a, const uchar *b, uint key_length,
>>>>                 my_bool insert_or_update);
>>>>    int pack_cmp(const uchar *b, uint key_length,my_bool
> insert_or_update);
>>>> @@ -1374,13 +1548,13 @@ public:
>>>>       @retval The length in the row plus the size of the data.
>>>>    */
>>>>    uint32 get_packed_size(const uchar *ptr_arg, bool low_byte_first)
>>>> -    {return packlength + get_length(ptr_arg, low_byte_first);}
>>>> +    {return packlength + get_length(ptr_arg, packlength,
> low_byte_first);}
>>>>     inline uint32 get_length(uint row_offset= 0)
>>>> -  { return get_length(ptr+row_offset,
> table->s->db_low_byte_first); }
>>>> -  uint32 get_length(const uchar *ptr, bool low_byte_first);
>>>> +  { return get_length(ptr+row_offset, this->packlength,
> table->s->db_low_byte_first); }
>>>> +  uint32 get_length(const uchar *ptr, uint packlength, bool
> low_byte_first);
>>>>    uint32 get_length(const uchar *ptr_arg)
>>>> -  { return get_length(ptr_arg, table->s->db_low_byte_first); }
>>>> +  { return get_length(ptr_arg, this->packlength,
> table->s->db_low_byte_first); }
>>>>    void put_length(uchar *pos, uint32 length);
>>>>    inline void get_ptr(uchar **str)
>>>>      {
>>>> @@ -1421,13 +1595,16 @@ public:
>>>>      memcpy_fixed(ptr+packlength,&tmp,sizeof(char*));
>>>>      return 0;
>>>>    }
>>>> -  uchar *pack(uchar *to, const uchar *from, uint max_length= ~(uint)
> 0);
>>>> -  uchar *pack_key(uchar *to, const uchar *from, uint max_length);
>>>> +  virtual uchar *pack(uchar *to, const uchar *from,
>>>> +                      uint max_length, bool low_byte_first);
>>>> +  uchar *pack_key(uchar *to, const uchar *from,
>>>> +                  uint max_length, bool low_byte_first);
>>>>    uchar *pack_key_from_key_image(uchar* to, const uchar *from,
>>>> -                                 uint max_length);
>>>> -  virtual const uchar *unpack(uchar *to, const uchar *from, uint
> param_data);
>>>> -  const uchar *unpack(uchar *to, const uchar *from);
>>>> -  const uchar *unpack_key(uchar* to, const uchar *from, uint
> max_length);
>>>> +                                 uint max_length, bool low_byte_first);
>>>> +  virtual const uchar *unpack(uchar *to, const uchar *from,
>>>> +                              uint param_data, bool low_byte_first);
>>>> +  const uchar *unpack_key(uchar* to, const uchar *from,
>>>> +                          uint max_length, bool low_byte_first);
>>>>    int pack_cmp(const uchar *a, const uchar *b, uint key_length,
>>>>                 my_bool insert_or_update);
>>>>    int pack_cmp(const uchar *b, uint key_length,my_bool
> insert_or_update);
>>>> @@ -1602,9 +1779,10 @@ public:
>>>>    uint32 pack_length() const { return (uint32) (field_length + 7) / 8;
> }
>>>>    uint32 pack_length_in_rec() const { return bytes_in_rec; }
>>>>    void sql_type(String &str) const;
>>>> -  uchar *pack(uchar *to, const uchar *from, uint max_length=~(uint) 0);
>>>> -  virtual const uchar *unpack(uchar *to, const uchar *from, uint
> param_data);
>>>> -  const uchar *unpack(uchar* to, const uchar *from);
>>>> +  virtual uchar *pack(uchar *to, const uchar *from,
>>>> +                      uint max_length, bool low_byte_first);
>>>> +  virtual const uchar *unpack(uchar *to, const uchar *from,
>>>> +                              uint param_data, bool low_byte_first);
>>>>    virtual void set_default();
>>>>     Field *new_key_field(MEM_ROOT *root, struct st_table
>>>> *new_table,
>>>> diff -Nrup a/sql/log.cc b/sql/log.cc
>>>> --- a/sql/log.cc	2007-08-14 14:20:02 +02:00
>>>> +++ b/sql/log.cc	2007-10-05 18:16:01 +02:00
>>>> @@ -3571,9 +3571,6 @@ bool MYSQL_BIN_LOG::write(Log_event *eve
>>>>  	(!binlog_filter->db_ok(local_db)))
>>>>      {
>>>>        VOID(pthread_mutex_unlock(&LOCK_log));
>>>> -      DBUG_PRINT("info",("OPTION_BIN_LOG is %s, db_ok('%s') == %d",
>>>> -                         (thd->options & OPTION_BIN_LOG) ? "set"
> : "clear",
>>>> -                         local_db, binlog_filter->db_ok(local_db)));
>>>>        DBUG_RETURN(0);
>>>>      }
>>>>  #endif /* HAVE_REPLICATION */
>>>> diff -Nrup a/sql/log_event.cc b/sql/log_event.cc
>>>> --- a/sql/log_event.cc	2007-08-27 20:22:01 +02:00
>>>> +++ b/sql/log_event.cc	2007-10-05 18:16:01 +02:00
>>>> @@ -5697,7 +5697,9 @@ Rows_log_event::Rows_log_event(const cha
>>>>                                 *description_event)
>>>>    : Log_event(buf, description_event),
>>>>      m_row_count(0),
>>>> +#ifndef MYSQL_CLIENT
>>>>      m_table(NULL),
>>>> +#endif
>>>>      m_rows_buf(0), m_rows_cur(0), m_rows_end(0),
>>>>      m_curr_row(NULL), m_curr_row_end(NULL),
>>>>      m_key(NULL)
>>>> @@ -6168,6 +6170,8 @@ int Rows_log_event::do_apply_event(RELAY
>>>>          unpack_current_row(rli);
>>>>           // at this moment m_curr_row_end should be set
>>>> +      DBUG_PRINT("debug", ("m_curr_row_end: 0x%lx; m_curr_row: 0x%lx;
> m_rows_end: 0x%lx",
>>>> +                           m_curr_row_end, m_curr_row, m_rows_end));
>>>>        DBUG_ASSERT(error || m_curr_row_end != NULL);
>>>> DBUG_ASSERT(error || m_curr_row < m_curr_row_end);
>>>>        DBUG_ASSERT(error || m_curr_row_end <= m_rows_end);
>>>> diff -Nrup a/sql/rpl_record.cc b/sql/rpl_record.cc
>>>> --- a/sql/rpl_record.cc	2007-08-26 14:31:03 +02:00
>>>> +++ b/sql/rpl_record.cc	2007-10-05 18:16:01 +02:00
>>>> @@ -65,6 +65,8 @@ pack_row(TABLE *table, MY_BITMAP const*
>>>> my_ptrdiff_t const rec_offset= record - table->record[0];
>>>>    my_ptrdiff_t const def_offset= table->s->default_values -
> table->record[0];
>>>>  +  DBUG_ENTER("pack_row");
>>>> +
>>>>    /*
>>>>      We write the null bits and the packed records using one pass
>>>>      through all the fields. The null bytes are written little-endian,
>>>> @@ -96,26 +98,11 @@ pack_row(TABLE *table, MY_BITMAP const*
>>>> For big-endian machines, we have to make sure that the
>>>>            length is stored in little-endian format, since this is the
>>>>            format used for the binlog.
>>>> -
>>>> -          We do this by setting the db_low_byte_first, which is used
>>>> -          inside some store_length() to decide what order to write the
>>>> -          bytes in.
>>>> -
>>>> -          In reality, db_log_byte_first is only set for legacy table
>>>> -          type Isam, but in the event of a bug, we need to guarantee
>>>> -          the endianess when writing to the binlog.
>>>> -
>>>> -          This is currently broken for NDB due to BUG#29549, so we
>>>> -          will fix it when NDB has fixed their way of handling BLOBs.
>>>>          */
>>>> -#if 0
>>>> -        bool save= table->s->db_low_byte_first;
>>>> -        table->s->db_low_byte_first= TRUE;
>>>> -#endif
>>>> -        pack_ptr= field->pack(pack_ptr, field->ptr + offset);
>>>> -#if 0
>>>> -        table->s->db_low_byte_first= save;
>>>> -#endif
>>>> +        const uchar *old_pack_ptr= pack_ptr;
>>>> +        pack_ptr= field->pack(pack_ptr, field->ptr + offset,
> UINT_MAX, TRUE);
>>>> +        DBUG_PRINT("debug", ("field: %s; pack_ptr: 0x%lx;
> pack_ptr':0x%lx; bytes: %d",
>>>> +                             field->field_name, old_pack_ptr,
> pack_ptr, pack_ptr - old_pack_ptr));
>>>>        }
>>>>         null_mask <<= 1;
>>>> @@ -143,8 +130,8 @@ pack_row(TABLE *table, MY_BITMAP const*
>>>> packed data. If it doesn't, something is very wrong.
>>>>    */
>>>>    DBUG_ASSERT(null_ptr == row_data + null_byte_count);
>>>> -
>>>> -  return static_cast<size_t>(pack_ptr - row_data);
>>>> +  DBUG_DUMP("row_data", row_data, pack_ptr - row_data);
>>>> +  DBUG_RETURN(static_cast<size_t>(pack_ptr - row_data));
>>>>  }
>>>>  #endif
>>>>  @@ -242,18 +229,11 @@ unpack_row(RELAY_LOG_INFO const *rli,
>>>>            Use the master's size information if available else call
>>>>            normal unpack operation.
>>>>          */
>>>> -#if 0
>>>> -        bool save= table->s->db_low_byte_first;
>>>> -        table->s->db_low_byte_first= TRUE;
>>>> -#endif
>>>>          uint16 const metadata= tabledef->field_metadata(i);
>>>> -        if (tabledef && metadata)
>>>> -          pack_ptr= f->unpack(f->ptr, pack_ptr, metadata);
>>>> -        else
>>>> -          pack_ptr= f->unpack(f->ptr, pack_ptr);
>>>> -#if 0
>>>> -        table->s->db_low_byte_first= save;
>>>> -#endif
>>>> +        uchar const *const old_pack_ptr= pack_ptr;
>>>> +        pack_ptr= f->unpack(f->ptr, pack_ptr, metadata, TRUE);
>>>> +	DBUG_PRINT("debug", ("field: %s; metadata: 0x%x; pack_ptr: 0x%lx;
> pack_ptr': 0x%lx; bytes: %d",
>>>> +                             f->field_name, metadata, old_pack_ptr,
> pack_ptr, pack_ptr - old_pack_ptr));
>>>>        }
>>>>         null_mask <<= 1;
>>>> @@ -265,6 +245,7 @@ unpack_row(RELAY_LOG_INFO const *rli,
>>>>      throw away master's extra fields
>>>>    */
>>>>    uint max_cols= min(tabledef->size(), cols->n_bits);
>>>> +  DBUG_PRINT("debug", ("Master has %u fields, slave %u",
> tabledef->size(), cols->n_bits));
>>>>    for (; i < max_cols; i++)
>>>>    {
>>>>      if (bitmap_is_set(cols, i))
>>>> @@ -279,6 +260,7 @@ unpack_row(RELAY_LOG_INFO const *rli,
>>>>         if (!((null_bits & null_mask) &&
> tabledef->maybe_null(i)))
>>>>          pack_ptr+= tabledef->calc_field_size(i, (uchar *) pack_ptr);
>>>> +      DBUG_PRINT("debug", ("pack_ptr: 0x%lx", pack_ptr));
>>>>        null_mask <<= 1;
>>>>      }
>>>>    }
>>>> @@ -288,6 +270,8 @@ unpack_row(RELAY_LOG_INFO const *rli,
>>>>      really wrong.
>>>>     */
>>>>    DBUG_ASSERT(null_ptr == row_data + master_null_byte_count);
>>>> +
>>>> +  DBUG_DUMP("row_data", row_data, pack_ptr - row_data);
>>>>     *row_end = pack_ptr;
>>>>    if (master_reclength)
>>>> diff -Nrup a/sql/sql_show.cc b/sql/sql_show.cc
>>>> --- a/sql/sql_show.cc	2007-08-16 18:04:11 +02:00
>>>> +++ b/sql/sql_show.cc	2007-10-05 18:16:02 +02:00
>>>> @@ -29,6 +29,8 @@
>>>>  #include "event_data_objects.h"
>>>>  #include <my_dir.h>
>>>>  +#define STR_OR_NIL(S) ((S) ? (S) : "<nil>")
>>>> +
>>>>  #ifdef WITH_PARTITION_STORAGE_ENGINE
>>>>  #include "ha_partition.h"
>>>>  #endif
>>>> @@ -3096,8 +3098,8 @@ int get_all_tables(THD *thd, TABLE_LIST
>>>> schema_table_idx= get_schema_table_idx(schema_table);
>>>>    get_lookup_field_values(thd, cond, tables, &lookup_field_vals);
>>>>    DBUG_PRINT("INDEX VALUES",("db_name='%s', table_name='%s'",
>>>> -                             lookup_field_vals.db_value.str,
>>>> -                             lookup_field_vals.table_value.str));
>>>> +                            
> STR_OR_NIL(lookup_field_vals.db_value.str),
>>>> +                            
> STR_OR_NIL(lookup_field_vals.table_value.str)));
>>>>     if (!lookup_field_vals.wild_db_value &&
>>>> !lookup_field_vals.wild_table_value)
>>>>    {
>>>>
>>>> -- 
>>>> MySQL Code Commits Mailing List
>>>> For list archives: http://lists.mysql.com/commits
>>>> To unsubscribe:    http://lists.mysql.com/commits?unsub=1
>>>>
>>>>     
>>>>         
>>> regards,
>>>
>>> Andrei
>>>   
>>>       
>> -- 
>> Mats Kindahl
>> Lead Software Developer
>> Replication Team
>> MySQL AB, www.mysql.com
>>
>>
>>
>> -- 
>> MySQL Code Commits Mailing List
>> For list archives: http://lists.mysql.com/commits
>> To unsubscribe:    http://lists.mysql.com/commits?unsub=1
>>     


-- 
Mats Kindahl
Lead Software Developer
Replication Team
MySQL AB, www.mysql.com