From: Olav Sandstaa
Date: January 12 2009 11:52am
Subject: Re: Falcon and timestamps on big-endian machines
List-Archive: http://lists.mysql.com/falcon/361
Message-Id: <496B2F0B.9070601@sun.com>
MIME-Version: 1.0
Content-Type: text/plain; format=flowed; charset=ISO-8859-1
Content-Transfer-Encoding: 7BIT

Ann and all,

I have started to work on bug #37281 "Time stamps in Falcon on 
big-endian machine give wrong results" and this email contains some 
responses to an email Ann wrote several months ago when I first reported 
the error with timestamps.

A short summary of bug #37281: The code for special handling of 
TIMESTAMPs on big-endian machines in StorageInterface::decodeRecord() 
was removed since it was believed this did not have any function 
(everything within the following #ifdef was removed):


                case MYSQL_TYPE_TIMESTAMP:
                    {
                    int value = (int) (dataStream->value.integer64 / 1000);
#ifdef _BIG_ENDIAN
                    if (table->s->db_low_byte_first)
                        int4store(field->ptr, value);
                    else
#endif
                    longstore(field->ptr, value);
                    }
                    break;

This lead to timestamps no longer working correctly on big-endian machines.


Ann W. Harrison wrote:
>   I still worry about reintroducing the code that was removed without
> understanding exactly why the timestamp should be inverted only on 
> decode.  MySQL can TIMESTAMP fields have different behaviors - some
> are automatically updated.  I think the code that was remove fixes
> an endian problem on automatically updated timestamps, but probably
> messes it up for others.

I agree with you that we should know why we the timestamp should be 
inverted only on decode before we reintroduces the the removed code - 
and I think I now understand why it is needed (see comments further down).

>
>   When a timestamp field with the TIMESTAMP_AUTO_SET_ON_INSERT flag
> set is inserted, ha_falcon.cpp appears to call back into the item
> to get a timestamp value.  That value is little endian, I think.
> Similarly, when a timestamp field update, ha_falcon checks the
> state of the auto update flag, and makes the same call.

You are right about this. In both these cases Falcon calls 
Field_timestamp::set_time() which again calls 
Field_timestamp::store_timestamp() with the following implementation:

  inline void store_timestamp(my_time_t timestamp)
  {
#ifdef WORDS_BIGENDIAN
    if (table && table->s->db_low_byte_first)
    {
      int4store(ptr,timestamp);
    }
    else
#endif
      longstore(ptr,(uint32) timestamp);
  }

This code will store the generated timestamps in little endian format 
both on little endian and big endian machines.

So initially both timetamps sent down from the server and timestamps 
generated by Falcon (when TIMESTAMP_AUTO_SET_ON_INSERT and 
TIMESTAMP_AUTO_SET_ON_UPDATE) are on little-endian format.

>
>   I think that the code that was removed rearranged the automatically
> generated values.  If I'm right, then it should not be used on
> timestamp fields that are not automatically set.  However, a better
> solution, if that is the case, would be to invert the values when
> they are set, so Falcon actually stores all values in big endian
> format on a big endian machine.

I think this is actually the case already (if I understood your 
suggestion correctly) since I believe Falcon stores the timestamps in 
big endian format on big endian machines. For both timestamps sent down 
by the server and the timestamps generated by Falcon, Falcon calls 
StorageInterface::encodeRecord() which has the following code for 
timestamps:

  case MYSQL_TYPE_TIMESTAMP:
                    {
                    my_bool nullValue;
                    int64 value = ((Field_timestamp*) 
field)->get_timestamp(&nullValue);
                    dataStream->encodeDate(value * 1000);
                    }

and the code for the Field_timestamp::get_timestamp() has the following 
implementation:

     inline long get_timestamp(my_bool *null_value)
  {
    if ((*null_value= is_null()))
      return 0;
#ifdef WORDS_BIGENDIAN
    if (table && table->s->db_low_byte_first)
      return sint4korr(ptr);
#endif
    long tmp;
    longget(tmp,ptr);
    return tmp;
  }

So if I have understood the code correctly, when Falcon has finished 
StorageInterface::encodeRecord() the timestamp value is stored in little 
endian format on little endian machines and big endian format on big 
endian machines. And as a further comment to your question about that we 
should understand by Falcon only does the endian conversion on 
::decodeRecord it seems like this is not the case. The same conversion 
takes place also in StorageInterface::encodeRecord() but is less visible 
since it is done in the call-back to Field_timestamp::get_timestamp().

The server wants to have timestamp values returned on a little-endian 
format. So it seems like we need to do the conversion from big-endian to 
little-endian in StorageInterface::decodeRecord() when running on a big 
endian machine. And thus the removed code is actually both needed and 
correct (as far as I can see).

Still, re-introducing the removed code might not be best solution. I 
think a better solution is to use a call-back to 
Field_timestamp::store_timestamp() (see implemention of it above). Thus 
the code in StorageInterface::decodeRecord() could look a like this:

  case MYSQL_TYPE_TIMESTAMP:
    int value = (int) (dataStream->value.integer64 / 1000);
    ((Field_timestamp*)field)->store_timestamp(value);
    break;

This would make the code for decode similar to what we do in 
StorageInterface::encodeRecord(), the endian convertion would be hidden 
in the Field_timestamp;:store_timestamp() implementation and overall 
make the code simpler.

I plan to submit a patch for this shortly which seems to solve all the 
failing test cases related to timestamps on big-endian machines.

Please let me know if anyone have comments or questions to the above or 
to the patch.

Best regards,
Olav