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

[Andrei Elkin]

Mats, hej.

You asked me to review it. I have not got any new commit mails but
found a new patch on the bug page:

Subject: bk commit into 5.1 tree (mats:1.2572) BUG#29549
ChangeSet@stripped, 2007-10-09 16:08:00+02:00, mats@stripped

I checked it and it is good.

I have two minor concerns from the last discussion and one new
question listed below.


cheers,

Andrei

>>>    * I'll add Field_blob to the cset comment.

The newest patch succeeds the older patch where we agreed about this
minor comment to add.
Are you going to recommit the older cset?


>>>    * I'll reuse Field::pack() and Field::unpack() where appropriate
>>>     

I found that in the newest.

>
> 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.
>

There is only the question. What did motivate you for this change?
What was wrong without having this new method?



>>>    * 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.
>

That was discussed, and I'd like you to document a part of our
discussion where you provided the definition of a pretty subtle
table->s->db_low_byte_first.
Could you please write it down somewhere in the code?
Esp. that specific part which claims on the little endian
db_low_byte_first is irrelevant.



> 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
>
>